Fixing the initialisation issues of categoryImagesCallback due to CategoryDetailsActivity, WikidataItemDetailsActivity not using reformed fragments#6747
Open
mnlch wants to merge 5 commits intocommons-app:mainfrom
Conversation
…preventing uninitialised access to categoryImagesCallback (6358)
…enting uninitialised access to categoryImagesCallback (6358)
…enting uninitialised access to categoryImagesCallback (6358)
# Conflicts: # app/src/main/java/fr/free/nrw/commons/category/CategoryDetailsActivity.kt
neeldoshii
requested changes
Mar 18, 2026
| val parentCategoriesFragment = fragments | ||
| .filterIsInstance<ParentCategoriesFragment>() | ||
| .firstOrNull() | ||
| ?: ParentCategoriesFragment() |
Collaborator
There was a problem hiding this comment.
Can you explain this approach please? It seems the PR tries to do something else and not related to the issue likely LLM can you verify this solves the issue?.
Collaborator
|
@mnlch Can you check why the test cases are failing, I am making this PR as draft once its ready for review feel free to make it ready for review. |
…idataItemDetailsActivityUnitTests (6358)
Author
|
I: as for the CI failure: I had renamed variable II: I'll explain you more how I found this error, so you can understand why I chose this fix: As described in my comment, which I mention in the PR, I found it by tracing. Here is my last trace log, which I created by adding Take note:
// PageabkeMediaFragment.kt
override fun onAttach(context: Context) {
super.onAttach(context)
Timber.d("tr16-03 PageableMediaFragment.kt enters onAttach %d", hashCode())
if (parentFragment != null) {
categoryImagesCallback = (parentFragment as CategoryImagesCallback)
} else {
Timber.d("tr16-03 PageableMediaFragment.kt uses activity as callback %d -> %d", hashCode(), activity.hashCode())
categoryImagesCallback = (activity as CategoryImagesCallback)
}
if (this::categoryImagesCallback.isInitialized) {
Timber.d("> tr16-03 at this point, categoryImagesCallback IS initialised")
}
}
// MediaDetailPagerFragment.kt
private fun initProvider() {
if (parentFragment is MediaDetailProvider) {
Timber.d("tr16-03 initProvider chose parentFragment %d", hashCode())
mediaDetailProvider = parentFragment as MediaDetailProvider
} else if (activity is MediaDetailProvider) {
Timber.d("tr16-03 initProvider chose activity %s,%d inside %d", activity, activity.hashCode(), hashCode())
mediaDetailProvider = activity as MediaDetailProvider?
} else {
throw ClassCastException("Parent must implement MediaDetailProvider")
}
}
What can you see in the logs?
What do I conclude from these logs?
So what was my solution?
Ultimately, I think the real issue is the logical complexity of this code. A lot of objects reference each other through their supertypes, which makes finding relatively easy issues (sequential coupling) quite obtuse. |
Collaborator
|
@RitikaPahwa4444 Can you run the copilot on this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses crash #6358 by ensuring CategoryDetailsActivity and WikidataItemDetailsActivity reuse already-restored ViewPager fragments after configuration/theme changes, preventing fragment fields from pointing at “fresh” unattached instances that lack initialized callbacks.
Changes:
- Rework
setTabs()inCategoryDetailsActivityandWikidataItemDetailsActivityto prefer existing fragments fromFragmentManagerover always instantiating new ones. - Rename
depictionImagesListFragmenttodepictedImagesListFragmentfor consistency withDepictedImagesFragment. - Update the corresponding unit test to reflect the renamed field.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/src/main/java/fr/free/nrw/commons/explore/depictions/WikidataItemDetailsActivity.kt | Reuse restored fragments in setTabs() and rename the DepictedImages fragment field. |
| app/src/main/java/fr/free/nrw/commons/category/CategoryDetailsActivity.kt | Reuse restored fragments in setTabs() to avoid overwriting restored instances. |
| app/src/test/kotlin/fr/free/nrw/commons/explore/depictions/WikidataItemDetailsActivityUnitTests.kt | Update test to use the renamed depictedImagesListFragment field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+101
to
+105
| val fragments = supportFragmentManager!!.fragments | ||
| depictedImagesListFragment = fragments | ||
| .filterIsInstance<DepictedImagesFragment>() | ||
| .firstOrNull() | ||
| ?: DepictedImagesFragment() |
Comment on lines
+122
to
124
| depictedImagesListFragment!!.arguments = arguments | ||
| parentDepictionsFragment.arguments = arguments | ||
| childDepictionsFragment.arguments = arguments |
Comment on lines
+91
to
+95
| val fragments = supportFragmentManager.fragments | ||
| categoriesMediaFragment = fragments | ||
| .filterIsInstance<CategoriesMediaFragment>() | ||
| .firstOrNull() | ||
| ?: CategoriesMediaFragment() |
Comment on lines
+91
to
+95
| val fragments = supportFragmentManager.fragments | ||
| categoriesMediaFragment = fragments | ||
| .filterIsInstance<CategoriesMediaFragment>() | ||
| .firstOrNull() | ||
| ?: CategoriesMediaFragment() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #6358
"What changes did you make and why?":
Inside the
setTabsfunction (called throughonCreate) of both*DetailsActivityclasses, I switched from initialising the fragments directlyto only initialising if the fragment is currently not in the
supportFragmentManager, meaning that it was not yet created/loaded in the current lifecycle:This is because during the reformation of the fragments and activities after a theme switch, the fragments apparently get loaded first, meaning that in the default behaviour, the loaded (and initialised) instances would be overwritten by one which does not load before being queried for the
categoryImagesCallback. When this occurs, however, the fragments are already present as children inside thesupportFragmentManager, meaning we can pull them out. I'll explain further in the discussion of issue #6358 to leave a breadcrumb trail for anyone encountering this issue again.I deemed the
filterIsInstancemethod as the most elegant method of pulling these children out of the manager. Other methods would be to indexfragments[i](children seem to always be in the same order they were added) or use the find by tag function, looking for "android:switcher:{viewPage id}:{i}", in accordance to this undocumented behaviour.Furthermore, I also renamed the variable
depictionImagesListFragmenttodepictedImagesListFragment, in accordance to its class,DepictedImagesFragment.PS: that weird little merge in the commit history is because a typo I wanted to amend, but my IDE turned into into a merge hiccup :(
Tests performed
As per the issue description, I tested this non-automatically on the Pixel 9 Pro emulator, running API 36.
I have tested the following scenarios: