-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Part of issue #5996: Fix IDE warnings in ContributionsListFragment #6542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Part of issue #5996: Fix IDE warnings in ContributionsListFragment #6542
Conversation
...Fragment (naming, null-safety, deprecations)
...ame alignment, remove redundant toggle)
✅ Generated APK variants!
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt
Show resolved
Hide resolved
WalkthroughThe file Changes
Sequence Diagram(s)This change set consists primarily of internal refactoring and modernization of existing functionality without introducing new features or altering primary control flow patterns, so sequence diagram generation is not applicable. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt (6)
418-439: Simplify and correct FAB state toggle; remove redundant early flipthis.isFabOpen is flipped before verifying visibility, which can desync state when fabPlus isn’t shown. Flip once after animations only.
Apply:
- private fun animateFAB(isFabOpen: Boolean) { - this.isFabOpen = !isFabOpen + private fun animateFAB(isFabOpen: Boolean) { if (binding!!.fabPlus.isShown) { if (isFabOpen) { binding!!.fabPlus.startAnimation(rotateBackward) binding!!.fabCamera.startAnimation(fabClose) binding!!.fabGallery.startAnimation(fabClose) binding!!.fabCustomGallery.startAnimation(fabClose) binding!!.fabCamera.hide() binding!!.fabGallery.hide() binding!!.fabCustomGallery.hide() } else { binding!!.fabPlus.startAnimation(rotateForward) binding!!.fabCamera.startAnimation(fabOpen) binding!!.fabGallery.startAnimation(fabOpen) binding!!.fabCustomGallery.startAnimation(fabOpen) binding!!.fabCamera.show() binding!!.fabGallery.show() binding!!.fabCustomGallery.show() } this.isFabOpen = !isFabOpen } }
466-468: Make RV state save null-safePrevent potential NPE if layoutManager is momentarily null.
Apply:
- val layoutManager = rvContributionsList?.layoutManager as GridLayoutManager? - outState.putParcelable(RV_STATE, layoutManager!!.onSaveInstanceState()) + (rvContributionsList?.layoutManager as? GridLayoutManager) + ?.onSaveInstanceState() + ?.let { outState.putParcelable(RV_STATE, it) }
473-476: Guard restore with null-safe callAvoid passing null to onRestoreInstanceState and skip if state missing.
Apply:
- val savedRecyclerLayoutState = - BundleCompat.getParcelable(savedInstanceState, RV_STATE, Parcelable::class.java) - rvContributionsList!!.layoutManager!!.onRestoreInstanceState(savedRecyclerLayoutState) + val savedRecyclerLayoutState = + BundleCompat.getParcelable(savedInstanceState, RV_STATE, Parcelable::class.java) + savedRecyclerLayoutState?.let { state -> + rvContributionsList?.layoutManager?.onRestoreInstanceState(state) + }
99-101: Tiny polish: make span counts compile-time constantsNot urgent, but consider moving spanCountLandscape/spanCountPortrait to companion object as const val for clarity.
Example:
- private val spanCountLandscape = 3 - private val spanCountPortrait = 1 + companion object { + private const val SPAN_COUNT_LANDSCAPE = 3 + private const val SPAN_COUNT_PORTRAIT = 1 + }Then use SPAN_* in getSpanCount/onConfigurationChanged.
Also applies to: 348-352, 358-366
543-547: Optional guard: avoid opening "/wiki/null"If getWikipediaPageTitle() can be null/blank, guard and inform the user before launching.
Apply:
- val url = - languageWikipediaSite!!.mobileUrl() + "/wiki/" + (contribution!!.wikidataPlace - ?.getWikipediaPageTitle()) - handleWebUrl(requireContext(), url.toUri()) + val title = contribution!!.wikidataPlace?.getWikipediaPageTitle() + if (title.isNullOrBlank()) { + showShortToast(context, R.string.no_wikipedia_title_found) + return + } + val url = "${languageWikipediaSite!!.mobileUrl()}/wiki/$title" + handleWebUrl(requireContext(), url.toUri())
444-447: Add KDoc or plan interface rename to clarify misleading parameter nameThe parameter
numberOfUploads: Booleanis misleading—it reads like a count but is actually a boolean flag controlling visibility. Since there is only one implementation (inContributionsListFragment.kt), renaming at the interface level inContributionsListContract.ktwould be straightforward. Add a KDoc to clarify thatnumberOfUploadscontrols whether to show the welcome message, or rename the parameter (e.g.,shouldShowWelcomeorisNewUser) in a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt (2)
app/src/main/java/fr/free/nrw/commons/contributions/WikipediaInstructionsDialogFragment.kt (1)
newInstance(56-60)app/src/main/java/fr/free/nrw/commons/utils/UrlUtils.kt (1)
handleWebUrl(16-33)
🔇 Additional comments (3)
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt (3)
22-24: Good modernization: BundleCompat + toUri
- Using BundleCompat.getParcelable avoids Tiramisu warnings.
- Switching to url.toUri() keeps Uri creation idiomatic.
Also applies to: 473-476, 543-547
479-482: LGTM: clearer names for openMediaDetail and callback forwardingRenamed parameters improve readability; forwarding remains unchanged in behavior.
508-520: Good: parentFragmentManager and typed dialog callbackUsing parentFragmentManager is the correct modern API; callback lambda signature is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Avoid double-bangs on injected deps; verify adapter ctor matches
Use requireNotNull or early null checks to prevent NPEs from mediaClient!!/mediaDataExtractor!!, and confirm the ContributionsListAdapter signature matches these args.
Run to confirm constructor and usages:
🏁 Script executed:
#!/bin/bash # Find adapter constructor and its params/usages rg -nP -C3 'class\s+ContributionsListAdapter\b|new\s+ContributionsListAdapter|ContributionsListAdapter\(' --type=kt
Length of output: 26
Let me also try a more direct approach:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ContributionsListAdapter class without file type restriction
# Try broader search pattern
rg -nP -C3 'class\s+ContributionsListAdapter'
# Also search for the file separately
echo "---"
echo "Looking for Kotlin files containing ContributionsListAdapter:"
fd -e kt -e kt
x -exec rg -l 'ContributionsListAdapter' {} \;
Length of output: 55568
🏁 Script executed:
#!/bin/bash # Search for ContributionsListAdapter definition rg 'class ContributionsListAdapter' -A 10
Length of output: 1405
---
🏁 Script executed:
```shell
#!/bin/bash
# Find mediaClient and mediaDataExtractor field declarations in ContributionsListFragment
rg -n 'mediaClient|mediaDataExtractor' app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt -B 2 -A 2
Length of output: 398
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where initAdapter() is called and check for null guards or context
rg -n 'initAdapter' app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt -B 5 -A 2
Length of output: 560
Replace double-bangs with requireNotNull() for injected dependencies
Fields mediaClient and mediaDataExtractor are nullable and injected via Dagger. Using !! on injected fields risks NPE if injection fails. Replace with requireNotNull() to provide explicit null safety and clearer error messages.
adapter = ContributionsListAdapter(this, requireNotNull(mediaClient) { "mediaClient must be injected" }, requireNotNull(mediaDataExtractor) { "mediaDataExtractor must be injected" }, compositeDisposable)
🤖 Prompt for AI Agents
In
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.kt
around lines 238 to 242, the adapter initialization uses force-unwraps
(mediaClient!! and mediaDataExtractor!!) which can throw NPE if Dagger injection
fails; replace these with requireNotNull(mediaClient) { "mediaClient must be
injected" } and requireNotNull(mediaDataExtractor) { "mediaDataExtractor must be
injected" } when passing to ContributionsListAdapter so null failures produce
explicit, descriptive exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test with zero-contributions user, but otherwise it seems to be working, thanks a lot!
e81f916
into
commons-app:main
Uh oh!
There was an error while loading. Please reload this page.
Description
Related to #5996
ContributionsListFragment.kt.numberOfUploads,contribution,isWikipediaPageExists) to improve consistency.Tests performed
Tested
ProdDebugbuild on Pixel 7 emulator with API level 34.Summary by CodeRabbit