fix: library content hidden behind soft navigation bar#623
fix: library content hidden behind soft navigation bar#623jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughReplaces the boolean "hide status bar" preference with a tri-state StatusBarMode (EDGE_TO_EDGE, HIDDEN, INSET), migrates stored preference, updates settings UI to choose a mode (with restart flow), and applies status bar + navigation bar padding changes across LibraryScreen, PluviaMain, and padding utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Settings as Settings UI
participant Pref as PrefManager
participant App as App (restart)
participant Pluvia as PluviaMain
participant Library as LibraryScreen
Settings->>Pref: set statusBarMode(newMode)
Pref-->>Settings: persist ack
Settings->>App: request restart
App->>Pluvia: initialize
Pluvia->>Pref: read statusBarMode
Pluvia->>Pluvia: set system UI visibility (compare to HIDDEN)
Pluvia->>Library: render with top padding based on statusBarMode
Library->>Library: apply navigationBarsPadding() when INSET
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
The filters & custom game addition buttons constantly fight with the nav bar buttons on my Thor. I guess this could be optional if you disagree. In any case, I personally find this to be significantly better. |
|
|
||
| Box( | ||
| Modifier.background(MaterialTheme.colorScheme.background) | ||
| .navigationBarsPadding() |
There was a problem hiding this comment.
We have a setting in the app settings for making the app "fullscreen". Probably should not add the padding in that circumstance? have you checked?
There was a problem hiding this comment.
Somehow I missed that setting. So if we go with this new setting, I would change that setting to be a menu: show app behind status bar (current default), hide status bar (current "hide status bar" toggle behavior), hide app behind status bar (new thing).
I would probably use it, but it's arguably not necessary. What do you think?
jeremybernstein
force-pushed
the
jb/navbar-insets
branch
2 times, most recently
from
6629550 to
24cfbd8
Compare
|
@utkarshdalal ok, this is now reimagined as an extension to the existing "hide status bar" option (there's a third option now for "standard" status bar mode). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt (1)
670-693:⚠️ Potential issue | 🟡 MinorWrong restart message string — user sees language-specific text for a status bar mode change.
R.string.settings_language_restart_messageresolves to "Changing the language requires the app to restart. Do you want to continue?" — this is displayed in the status bar mode restart dialog, which is confusing. Thesettings_interface_statusbar_mode_subtitlestring added instrings.xmlis closer to correct, but a dedicated confirmation message string would be ideal.🐛 Proposed fix
Add a dedicated string (e.g., in
strings.xml):<string name="settings_interface_statusbar_mode_restart_message">Changing the status bar mode requires the app to restart. Do you want to continue?</string>Then update the dialog:
MessageDialog( visible = showStatusBarRestartDialog, title = stringResource(R.string.settings_interface_restart_required_title), - message = stringResource(R.string.settings_language_restart_message), + message = stringResource(R.string.settings_interface_statusbar_mode_restart_message), confirmBtnText = stringResource(R.string.settings_language_restart_confirm),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt` around lines 670 - 693, The status bar restart dialog is using the wrong string resource (R.string.settings_language_restart_message); add a dedicated confirmation string (e.g., settings_interface_statusbar_mode_restart_message with text "Changing the status bar mode requires the app to restart. Do you want to continue?") to strings.xml and update the MessageDialog call in SettingsGroupInterface (the visible = showStatusBarRestartDialog block) to use R.string.settings_interface_statusbar_mode_restart_message (or your new settings_interface_statusbar_mode_restart_message) for the message instead of the language string; leave the rest of the dialog handlers (onConfirmClick/onDismissRequest/onDismissClick, pendingStatusBarMode, showStatusBarLoadingDialog) unchanged.app/src/main/java/app/gamenative/utils/PaddingUtils.kt (1)
10-13:⚠️ Potential issue | 🟡 MinorStale docstring still references the old boolean semantics.
The KDoc still says "When
hideStatusBaris true", which no longer matches the enum-based check.📝 Suggested update
- * Creates padding values with conditional top padding based on status bar visibility. - * When hideStatusBar is true, top padding is 0.dp, otherwise it uses defaultPadding. - * All other sides use defaultPadding. + * Creates padding values with conditional top padding based on the current [StatusBarMode]. + * When the mode is [StatusBarMode.HIDDEN], top padding is 0.dp; otherwise uses defaultPadding. + * All other sides use defaultPadding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/PaddingUtils.kt` around lines 10 - 13, The KDoc in PaddingUtils still describes the old boolean behavior ("When hideStatusBar is true") but the implementation now uses an enum; update the comment to reference the enum-based parameter (e.g., StatusBarVisibility) and explain the actual semantics: when the enum value is StatusBarVisibility.HIDDEN the top padding is 0.dp, otherwise top uses defaultPadding and all other sides use defaultPadding; ensure the KDoc mentions the parameter name (hideStatusBar or the current parameter name) and the enum constants used so it matches the code.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/enums/StatusBarMode.kt (1)
3-7: Ordinal-based persistence is fragile — consider name-based storage.
PrefManagerstores and readsStatusBarModeby ordinal (0/1/2). Inserting or reordering enum entries in the future silently decodes stored values to the wrong mode (e.g., inserting a new entry beforeHIDDENshifts its ordinal and corrupts existing prefs). Storing bynameinstead ofordinalis immune to reordering.♻️ Name-based alternative in PrefManager
Change the backing key to a
stringPreferencesKeyand useStatusBarMode.valueOf():-private val STATUS_BAR_MODE = intPreferencesKey("status_bar_mode") +private val STATUS_BAR_MODE = stringPreferencesKey("status_bar_mode") var statusBarMode: StatusBarMode get() { - val value = getPref(STATUS_BAR_MODE, StatusBarMode.EDGE_TO_EDGE.ordinal) - return StatusBarMode.entries.getOrNull(value) ?: StatusBarMode.EDGE_TO_EDGE + val value = getPref(STATUS_BAR_MODE, StatusBarMode.EDGE_TO_EDGE.name) + return runCatching { StatusBarMode.valueOf(value) }.getOrDefault(StatusBarMode.EDGE_TO_EDGE) } set(value) { - setPref(STATUS_BAR_MODE, value.ordinal) + setPref(STATUS_BAR_MODE, value.name) }The migration block would similarly write
migrated.nameinstead ofmigrated.ordinal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/enums/StatusBarMode.kt` around lines 3 - 7, PrefManager currently persists StatusBarMode using ordinal which breaks if enum entries are reordered; change PrefManager to store the enum by name using a stringPreferencesKey and read/write via StatusBarMode.valueOf(storedString) (and write using mode.name), and update any migration logic to write migrated.name instead of migrated.ordinal so existing stored ordinals are converted to the correct enum name before switching the preference key.app/src/main/java/app/gamenative/PrefManager.kt (1)
720-733: Migration-in-getter adds permanent extra I/O overhead on everystatusBarModeaccess.Every call to
statusBarModealways executesrunBlocking { dataStore.data.first()[oldKey] }to check for the legacy key — even long after migration is complete (the old key is permanently gone after the first run). This means two blocking DataStore reads per access instead of one, which is particularly costly sincestatusBarAwarePaddinginPaddingUtils.ktcalls this getter during Compose recomposition.Additionally, because
removePrefandsetPrefare fire-and-forget (scope.launch), the migration block can execute multiple times before the async remove completes — though the result is idempotent, it generates redundant writes.The existing
init()method is the right place for one-time migration, consistent with howaccessTokenandrefreshTokenare migrated:♻️ Proposed refactor — move migration to init()
In
init():fun init(context: Context) { dataStore = context.datastore + + // Migrate old boolean status bar pref to StatusBarMode enum + val oldStatusBarKey = booleanPreferencesKey("hide_status_bar_when_not_in_game") + runBlocking { dataStore.data.first()[oldStatusBarKey] }?.let { oldValue -> + val migrated = if (oldValue) StatusBarMode.HIDDEN else StatusBarMode.EDGE_TO_EDGE + setPref(STATUS_BAR_MODE, migrated.ordinal) + removePref(oldStatusBarKey) + } + // Note: Should remove after a few release versions... val oldPassword = stringPreferencesKey("password") removePref(oldPassword)Then simplify the getter to a single read:
var statusBarMode: StatusBarMode get() { - val oldKey = booleanPreferencesKey("hide_status_bar_when_not_in_game") - val oldValue = runBlocking { dataStore.data.first()[oldKey] } - if (oldValue != null) { - val migrated = if (oldValue) StatusBarMode.HIDDEN else StatusBarMode.EDGE_TO_EDGE - setPref(STATUS_BAR_MODE, migrated.ordinal) - removePref(oldKey) - return migrated - } val value = getPref(STATUS_BAR_MODE, StatusBarMode.EDGE_TO_EDGE.ordinal) return StatusBarMode.entries.getOrNull(value) ?: StatusBarMode.EDGE_TO_EDGE }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/PrefManager.kt` around lines 720 - 733, The getter for statusBarMode currently performs a runBlocking DataStore read to migrate the old boolean key on every access; move that one-time migration into init() by checking booleanPreferencesKey("hide_status_bar_when_not_in_game") there (use runBlocking { dataStore.data.first()[oldKey] }) and if non-null call setPref(STATUS_BAR_MODE, migrated.ordinal) and removePref(oldKey) once; then simplify the statusBarMode getter to only call getPref(STATUS_BAR_MODE, StatusBarMode.EDGE_TO_EDGE.ordinal) and map via StatusBarMode.entries.getOrNull(value) ?: StatusBarMode.EDGE_TO_EDGE so subsequent accesses avoid extra I/O and redundant migration attempts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/res/values/strings.xml`:
- Line 668: The localized string resource
settings_interface_statusbar_mode_subtitle is unused and should be removed; open
the strings.xml entry for "settings_interface_statusbar_mode_subtitle" and
delete that <string> element, then run a project-wide search or Gradle
lint/assemble to confirm no references remain and commit the cleaned file.
---
Outside diff comments:
In
`@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt`:
- Around line 670-693: The status bar restart dialog is using the wrong string
resource (R.string.settings_language_restart_message); add a dedicated
confirmation string (e.g., settings_interface_statusbar_mode_restart_message
with text "Changing the status bar mode requires the app to restart. Do you want
to continue?") to strings.xml and update the MessageDialog call in
SettingsGroupInterface (the visible = showStatusBarRestartDialog block) to use
R.string.settings_interface_statusbar_mode_restart_message (or your new
settings_interface_statusbar_mode_restart_message) for the message instead of
the language string; leave the rest of the dialog handlers
(onConfirmClick/onDismissRequest/onDismissClick, pendingStatusBarMode,
showStatusBarLoadingDialog) unchanged.
In `@app/src/main/java/app/gamenative/utils/PaddingUtils.kt`:
- Around line 10-13: The KDoc in PaddingUtils still describes the old boolean
behavior ("When hideStatusBar is true") but the implementation now uses an enum;
update the comment to reference the enum-based parameter (e.g.,
StatusBarVisibility) and explain the actual semantics: when the enum value is
StatusBarVisibility.HIDDEN the top padding is 0.dp, otherwise top uses
defaultPadding and all other sides use defaultPadding; ensure the KDoc mentions
the parameter name (hideStatusBar or the current parameter name) and the enum
constants used so it matches the code.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/enums/StatusBarMode.kt`:
- Around line 3-7: PrefManager currently persists StatusBarMode using ordinal
which breaks if enum entries are reordered; change PrefManager to store the enum
by name using a stringPreferencesKey and read/write via
StatusBarMode.valueOf(storedString) (and write using mode.name), and update any
migration logic to write migrated.name instead of migrated.ordinal so existing
stored ordinals are converted to the correct enum name before switching the
preference key.
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 720-733: The getter for statusBarMode currently performs a
runBlocking DataStore read to migrate the old boolean key on every access; move
that one-time migration into init() by checking
booleanPreferencesKey("hide_status_bar_when_not_in_game") there (use runBlocking
{ dataStore.data.first()[oldKey] }) and if non-null call
setPref(STATUS_BAR_MODE, migrated.ordinal) and removePref(oldKey) once; then
simplify the statusBarMode getter to only call getPref(STATUS_BAR_MODE,
StatusBarMode.EDGE_TO_EDGE.ordinal) and map via
StatusBarMode.entries.getOrNull(value) ?: StatusBarMode.EDGE_TO_EDGE so
subsequent accesses avoid extra I/O and redundant migration attempts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/enums/StatusBarMode.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.ktapp/src/main/java/app/gamenative/utils/PaddingUtils.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt
EDGE_TO_EDGE (Immersive), HIDDEN, INSET (Standard) with SingleChoiceDialog in settings. Migrates old boolean pref on first access. navigationBarsPadding only in INSET mode.
jeremybernstein
force-pushed
the
jb/navbar-insets
branch
from
24cfbd8 to
a0312a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/enums/StatusBarMode.kt (1)
1-7: Consider localizing the mode labels if they’re user-facing.If
StatusBarMode.textis shown in UI, hard-coded English strings bypass localization. Consider switching to@StringResids.♻️ Suggested refactor
+import androidx.annotation.StringRes +import app.gamenative.R + -enum class StatusBarMode(val text: String) { - EDGE_TO_EDGE("Immersive"), - HIDDEN("Hidden"), - INSET("Standard"), -} +enum class StatusBarMode(`@StringRes` val labelRes: Int) { + EDGE_TO_EDGE(R.string.status_bar_mode_immersive), + HIDDEN(R.string.status_bar_mode_hidden), + INSET(R.string.status_bar_mode_standard), +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/enums/StatusBarMode.kt` around lines 1 - 7, The enum StatusBarMode currently stores hard-coded English strings in the text property; if these are shown in UI, change the enum to hold Android string resource IDs instead (e.g., rename text to textRes: `@StringRes` Int) and update all usages to call context.getString(statusBarMode.textRes) or use binding to resolve the string resource; modify the enum entries (EDGE_TO_EDGE, HIDDEN, INSET) to pass the appropriate R.string.* ids and update any call sites that previously relied on StatusBarMode.text to use the resource-resolving approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 719-733: The migration and default for status bar mode currently
map a false old boolean to StatusBarMode.EDGE_TO_EDGE and default new installs
to EDGE_TO_EDGE, which prevents INSET (which enables navigationBarsPadding())
from being applied; update the migration in the statusBarMode getter to map
oldValue == false to StatusBarMode.INSET (instead of EDGE_TO_EDGE) and change
the getPref default for STATUS_BAR_MODE to StatusBarMode.INSET.ordinal so both
migrated users and new installs get INSET by default; modify only the mapping
and default while keeping the existing setPref(STATUS_BAR_MODE, ...),
removePref(oldKey), and fallback to StatusBarMode.EDGE_TO_EDGE only as a final
null-safe fallback if needed.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/enums/StatusBarMode.kt`:
- Around line 1-7: The enum StatusBarMode currently stores hard-coded English
strings in the text property; if these are shown in UI, change the enum to hold
Android string resource IDs instead (e.g., rename text to textRes: `@StringRes`
Int) and update all usages to call context.getString(statusBarMode.textRes) or
use binding to resolve the string resource; modify the enum entries
(EDGE_TO_EDGE, HIDDEN, INSET) to pass the appropriate R.string.* ids and update
any call sites that previously relied on StatusBarMode.text to use the
resource-resolving approach.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/enums/StatusBarMode.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.ktapp/src/main/java/app/gamenative/utils/PaddingUtils.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
- app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt
- app/src/main/java/app/gamenative/utils/PaddingUtils.kt
|
@utkarshdalal now stable, ready for review |
|
@jeremybernstein can you show me an example of what the soft navigation bar looks like? Is it the android bar at the bottom with the back, home and recent apps button? If so, I think perhaps best to not add more options there. We have a UI overhaul coming soon, which I believe puts the app in fullscreen mode. I'm not sure how the bottom bar will be affected by that but might be good to wait for that first. takeaway is let's not add a third option to the display in addition to show and hide status bars, we should update the existing toggle to hide all status bars (edge to edge mode) or keep it normal, and if the bottom navigation bar is showing, add padding there. |
|
Immersive (current default, toggle off): Status hidden (current toggle on): New mode: |
|
Fine with waiting for UI overhaul. |
|
irrelevant with new UI, closing |
2 participants
Summary
navigationBarsPadding()to the library screen container so content isn't hidden behind the soft navbarCloses #621
Summary by cubic
Fixes Library content hidden behind the soft navigation bar by applying navigationBarsPadding only in Standard mode. Replaces the old status bar toggle with a 3‑mode setting (Immersive, Hidden, Standard) and migrates existing prefs automatically.
Written for commit a0312a3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes