Skip to content

Fix bookmarks rotation issue #6753

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.

Sign up for GitHub

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

Open
Roniscend wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix bookmarks rotation issue #6753

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/bookmarks/BookmarkFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,26 @@ class BookmarkFragment : CommonsDaggerSupportFragment() {
*/
fun setupTabLayout() {
binding!!.tabLayout.visibility = View.VISIBLE
context?.resources?.configuration?.orientation?.let { orientation ->
updateTabMode(orientation)
}
if (adapter!!.count == 1) {
binding!!.tabLayout.visibility = View.GONE
}
}
override fun onConfigurationChanged(newConfig: android.content.res.Configuration) {
Copy link
Collaborator

@RitikaPahwa4444 RitikaPahwa4444 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to handle this manually? Could you check if we've set configChanges for the activity?

Copy link
Contributor

@rovertrack rovertrack Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config changes are set in the android manifest for Mainactivity which is host for bookmarkfragment
i think this pr is focused on manipulating the tablayout.mode and tablayout.gravity based on the current device orientation

Copy link
Collaborator

@RitikaPahwa4444 RitikaPahwa4444 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not recommended to forcefully prevent activity recreation. My understanding is that this is when we need to manually handle orientation changes. I would recommend checking for alternatives once for this.

Copy link
Contributor

@rovertrack rovertrack Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed which would mess with other parts of the activity
alternatives would be best with using styles
something like this .

<style name="BookmarkTabStyle">
    <item name="app:tabMode">scrollable</item>
    <item name="app:tabGravity">fill</item>
</style>

Copy link
Contributor Author

@Roniscend Roniscend Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RitikaPahwa4444 You are right , Mainactivity file has configchanges set , this was a localized workaround without triggering a large refactor , If this approach is not satisfactory i can implement OnLayoutChangeListener on the Tablayout

Copy link
Contributor Author

@Roniscend Roniscend Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed which would mess with other parts of the activity alternatives would be best with using styles something like this .

<style name="BookmarkTabStyle">
    <item name="app:tabMode">scrollable</item>
    <item name="app:tabGravity">fill</item>
</style>

@rovertrack Thanks for the suggestion , However relying on XML styles won't actually work in this architectural scenario, since MainActivity uses configChanges="orientation|screenSize", the Fragment is never destroyed and the view hierarchy is never re inflated during rotation. This means Android won't dynamically swap between a values and values-land style the layout will just get permanently stuck in whichever style it was initially inflated with.
Also a quick heads up tabGravity is ignored by the framework whenever tabMode = "scrollable" is used

Copy link
Collaborator

@RitikaPahwa4444 RitikaPahwa4444 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No workarounds for config changes, please 🙂 I've pinned the issue where I've shared my thoughts, we've already patched a lot. So, any localised bug fixes might not get merged - there are some other long standing PRs too.

If, however, we're extending it to Main activity in general, we can discuss the approach.

Copy link
Contributor

@rovertrack rovertrack Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed #6538

super.onConfigurationChanged(newConfig)
updateTabMode(newConfig.orientation)
}
private fun updateTabMode(orientation: Int) {
if (orientation == android.content.res.Configuration.ORIENTATION_LANDSCAPE) {
binding?.tabLayout?.tabMode = com.google.android.material.tabs.TabLayout.MODE_FIXED
binding?.tabLayout?.tabGravity = com.google.android.material.tabs.TabLayout.GRAVITY_FILL
} else {
binding?.tabLayout?.tabMode = com.google.android.material.tabs.TabLayout.MODE_SCROLLABLE
binding?.tabLayout?.tabGravity = com.google.android.material.tabs.TabLayout.GRAVITY_FILL
}
}


fun onBackPressed() {
Expand Down
Loading