From eb0a5d60e1f274f8583fbb01faeb154eddfe3e83 Mon Sep 17 00:00:00 2001 From: rthak Date: Sat, 15 Aug 2026 14:54:14 -0500 Subject: [PATCH] Fix: multi-language captions not saved on upload (#6938) Language rows added via the "+" button in the media detail step only ever lived in UploadMediaDetailAdapter's own copy of the list. Nothing wrote them back into the UploadItem that the upload pipeline actually reads, so as soon as the screen was repopulated (e.g. navigating to another step and back), the extra rows were silently dropped and only the default-language caption made it into the uploaded file. Add an EventListener.onMediaDetailsChanged() callback fired whenever a row is added or removed, and have UploadMediaDetailFragment persist the adapter's current list into the UploadItem immediately. Co-Authored-By: Claude Sonnet 5 --- .../description/DescriptionEditActivity.kt | 2 ++ .../upload/UploadMediaDetailAdapter.kt | 6 ++++ .../mediaDetails/UploadMediaDetailFragment.kt | 34 ++++++++++++------- .../UploadMediaDetailAdapterUnitTest.kt | 28 +++++++++++++++ .../upload/UploadMediaPresenterTest.kt | 26 ++++++++++++++ .../UploadMediaDetailFragmentUnitTest.kt | 21 ++++++++++++ 6 files changed, 105 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/description/DescriptionEditActivity.kt b/app/src/main/java/fr/free/nrw/commons/description/DescriptionEditActivity.kt index b1f1b7f9b8d..164108349a9 100644 --- a/app/src/main/java/fr/free/nrw/commons/description/DescriptionEditActivity.kt +++ b/app/src/main/java/fr/free/nrw/commons/description/DescriptionEditActivity.kt @@ -157,6 +157,8 @@ class DescriptionEditActivity : override fun onPrimaryCaptionTextChange(isNotEmpty: Boolean) {} + override fun onMediaDetailsChanged() {} + private fun onVoiceInput(result: ActivityResult) { if (result.resultCode == RESULT_OK && result.data != null) { val resultData = result.data!!.getStringArrayListExtra(RecognizerIntent.EXTRA_RESULTS) diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadMediaDetailAdapter.kt b/app/src/main/java/fr/free/nrw/commons/upload/UploadMediaDetailAdapter.kt index b19da15e6b0..2ab3a42341b 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadMediaDetailAdapter.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadMediaDetailAdapter.kt @@ -123,6 +123,7 @@ class UploadMediaDetailAdapter : RecyclerView.Adapter= 0) { - presenter.setUploadMediaDetails(uploadMediaDetailAdapter.items, indexOfFragment) - Timber.d("Restored and set upload media details for index %d", indexOfFragment) - } else { - Timber.w("Invalid indexOfFragment %d, skipping setUploadMediaDetails", indexOfFragment) - } - } else { - Timber.w("fragmentCallback is null, skipping setUploadMediaDetails") - } + persistMediaDetails() } else { // initialize with a default UploadMediaDetail if saved state is empty or null uploadMediaDetailAdapter.items = mutableListOf(UploadMediaDetail()) @@ -486,6 +475,27 @@ class UploadMediaDetailFragment : UploadBaseFragment(), UploadMediaDetailsContra fragmentCallback!!.onNextButtonClicked(indexOfFragment) } + /** + * Fixes issue #6938: without this, rows added via "+" only existed in the adapter + * and were lost once the fragment was repopulated, so only the primary caption uploaded. + */ + private fun persistMediaDetails() { + // only call setUploadMediaDetails if indexOfFragment is valid + if (fragmentCallback != null) { + indexOfFragment = fragmentCallback!!.getIndexInViewFlipper(this) + if (indexOfFragment >= 0) { + presenter.setUploadMediaDetails(uploadMediaDetailAdapter.items, indexOfFragment) + Timber.d("Restored and set upload media details for index %d", indexOfFragment) + } else { + Timber.w("Invalid indexOfFragment %d, skipping setUploadMediaDetails", indexOfFragment) + } + } else { + Timber.w("fragmentCallback is null, skipping setUploadMediaDetails") + } + } + + override fun onMediaDetailsChanged() = persistMediaDetails() + /** * This method gets called whenever the next/previous button is pressed */ diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaDetailAdapterUnitTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaDetailAdapterUnitTest.kt index 7cc59b78dd1..6b8ce42abde 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaDetailAdapterUnitTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaDetailAdapterUnitTest.kt @@ -147,6 +147,33 @@ class UploadMediaDetailAdapterUnitTest { adapter.addDescription(uploadMediaDetail) val map: HashMap = selectedLanguages.get(adapter) as HashMap Assert.assertEquals(map[list.size], null) + // Regression test for issue #6938: added rows must propagate to the UploadItem. + verify(eventListener).onMediaDetailsChanged() + } + + @Test + @Throws(Exception::class) + fun testAddDescriptionSyncsToBackingListAndFormatCaptions() { + // Regression test for issue #6938: a caption added via "+" must reach + // Contribution.formatCaptions, not just the adapter's own copy of the list. + val realAdapter = + UploadMediaDetailAdapter(fragment, "", recentLanguagesDao, mockResultLauncher) + realAdapter.items = mutableListOf(UploadMediaDetail(languageCode = "en", captionText = "test")) + var uploadItemMediaDetails: List = emptyList() + realAdapter.eventListener = object : UploadMediaDetailAdapter.EventListener { + override fun onPrimaryCaptionTextChange(isNotEmpty: Boolean) = Unit + override fun addLanguage() = Unit + override fun onMediaDetailsChanged() { + uploadItemMediaDetails = realAdapter.items + } + } + + realAdapter.addDescription(UploadMediaDetail(languageCode = "ja", captionText = "テスト")) + + val captions = fr.free.nrw.commons.contributions.Contribution.formatCaptions(uploadItemMediaDetails) + Assert.assertEquals(2, captions.size) + Assert.assertEquals("test", captions["en"]) + Assert.assertEquals("テスト", captions["ja"]) } @Test @@ -164,6 +191,7 @@ class UploadMediaDetailAdapterUnitTest { adapter.removeDescription(uploadMediaDetail, list.size) val map: HashMap = selectedLanguages.get(adapter) as HashMap Assert.assertEquals(map[list.size], null) + verify(eventListener).onMediaDetailsChanged() } @Test diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt index da14438342b..54d2ce3166e 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt @@ -154,6 +154,32 @@ class UploadMediaPresenterTest { testScheduler.triggerActions() } + /** + * Regression test for #6938: with multiple images in one upload session, captions + * for one image must not be written onto another image's UploadItem. + */ + @Test + fun setUploadMediaDetailsOnlyUpdatesTargetedItemInMultiImageUpload() { + val firstItem = + UploadItem(Uri.EMPTY, null, null, null, 0, null, null, null) + val firstItemDetails = + listOf(UploadMediaDetail(languageCode = "en", captionText = "first")) + firstItem.uploadMediaDetails = firstItemDetails.toMutableList() + val secondItem = + UploadItem(Uri.EMPTY, null, null, null, 0, null, null, null) + whenever(repository.getUploads()).thenReturn(listOf(firstItem, secondItem)) + + val secondItemDetails = + listOf( + UploadMediaDetail(languageCode = "en", captionText = "second"), + UploadMediaDetail(languageCode = "de", captionText = "zweite"), + ) + uploadMediaPresenter.setUploadMediaDetails(secondItemDetails, 1) + + assertEquals(secondItemDetails, secondItem.uploadMediaDetails) + assertEquals(firstItemDetails, firstItem.uploadMediaDetails) + } + /** * Test for empty file name when the user presses the NEXT button */ diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragmentUnitTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragmentUnitTest.kt index 0cab47c67f0..0f8ad2cbec9 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragmentUnitTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragmentUnitTest.kt @@ -33,6 +33,7 @@ import fr.free.nrw.commons.nearby.Place import fr.free.nrw.commons.upload.ImageCoordinates import fr.free.nrw.commons.upload.UploadActivity import fr.free.nrw.commons.upload.UploadItem +import fr.free.nrw.commons.upload.UploadMediaDetail import fr.free.nrw.commons.upload.UploadMediaDetailAdapter import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailFragment.Companion.LAST_ZOOM import org.junit.Assert @@ -362,6 +363,26 @@ class UploadMediaDetailFragmentUnitTest { fragment.updateMediaDetails(mock()) } + @Test + @Throws(Exception::class) + fun testOnMediaDetailsChangedPersistsAdapterItemsToUploadItem() { + // Regression test for #6938: a language row added via "+" must be written back into the + // UploadItem backing this fragment to ensure it gets uploaded + Shadows.shadowOf(Looper.getMainLooper()).idle() + Whitebox.setInternalState(fragment, "fragmentCallback", callback) + Whitebox.setInternalState(fragment, "presenter", presenter) + val itemsWithSecondLanguage = mutableListOf( + UploadMediaDetail(languageCode = "en", captionText = "test"), + UploadMediaDetail(languageCode = "ja", captionText = "テスト") + ) + `when`(uploadMediaDetailAdapter.items).thenReturn(itemsWithSecondLanguage) + + fragment.onMediaDetailsChanged() + + Mockito.verify(presenter) + .setUploadMediaDetails(itemsWithSecondLanguage, 0) + } + @Test @Throws(Exception::class) fun testOnDestroyView() {