RFC: Preliminary work for configurable navigation bar and top buttons#3279
Closed
Lurux wants to merge 10 commits intoMetrolistGroup:mainfrom
Closed
RFC: Preliminary work for configurable navigation bar and top buttons#3279Lurux wants to merge 10 commits intoMetrolistGroup:mainfrom
Lurux wants to merge 10 commits intoMetrolistGroup:mainfrom
Conversation
…THER.position setting and clean up unused code paths
📝 WalkthroughWalkthroughRefactors navigation from a sealed-class model ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as rgba(52,152,219,0.5) UI (TopBar/BottomBar)
participant Nav as rgba(46,204,113,0.5) NavController
participant Pref as rgba(155,89,182,0.5) DataStore/Preferences
participant Router as rgba(241,196,15,0.5) NavigationBuilder
UI->>Pref: read NavigationScreens.getNavbarItems()
Pref-->>UI: list of navbar items
UI->>UI: render nav bar & top bar using items
UI->>Nav: onItemClick(route)
Nav->>Router: navigate to route
Router-->>Nav: route composable invoked
Nav->>Pref: (optional) update NavigationScreens position via setter
Pref-->>Nav: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
3 tasks
…ens.getNavbarItems
Author
|
Update: changed everything to use NavigationScreens and removed legacy Screens enum. "Default open tab" now allows setting any of the NavigationScreens by default, I made it so selecting an item will pin it to the navigation bar by default, then revert to default config once unpinned. PR is ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/ui/component/AppNavigation.kt (1)
42-46:⚠️ Potential issue | 🟡 MinorKeep Search selected on result screens.
NavigationScreens.SEARCH.routeissearch_input, so a route likesearch/{query}never satisfiesstartsWith("$screenRoute/"). The rail/bar drops Search’s selected state as soon as the user opens results.MainActivityalready special-cases"search/", so this helper should mirror that rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/component/AppNavigation.kt` around lines 42 - 46, The selection logic in isRouteSelected fails to keep Search selected because NavigationScreens.SEARCH.route == "search_input" and routes like "search/{query}" won't match startsWith("$screenRoute/"); update isRouteSelected to special-case NavigationScreens.SEARCH.route (or when screenRoute equals that value) and also return true when currentRoute.startsWith("search/") (mirroring MainActivity's "search/" rule), while keeping the existing exact-equals and startsWith("$screenRoute/") checks for other screens.
🤖 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/kotlin/com/metrolist/music/constants/NavigationScreens.kt`:
- Around line 50-57: The SEARCH enum entry in NavigationScreens is using the
same persisted key as Home (stringPreferencesKey("nav_home_position")), which
couples their saved positions; change the SEARCH entry to use its own
Preferences key (e.g., stringPreferencesKey("nav_search_position")) so Search
position is persisted separately. Locate the SEARCH enum constant in
NavigationScreens and replace the stringPreferencesKey("nav_home_position")
argument with stringPreferencesKey("nav_search_position"); ensure any code that
reads/writes that key (if present) is updated to the new key name to maintain
consistency.
In `@app/src/main/kotlin/com/metrolist/music/MainActivity.kt`:
- Around line 575-592: The logic building topLevelScreens and resolving
currentTitleRes only uses NavigationScreens.getNavbarItems(), causing TopAppBar
visibility and title lookup to miss routes from
NavigationScreens.getTopbarItems(); update the code that constructs
topLevelScreens (and any title-resolution logic that references it, e.g.,
currentTitleRes) to include routes from getTopbarItems() as well (merge
getNavbarItems() and getTopbarItems() results before mapping to .route) so
History/Stats/AUTOMATIC overflow routes are treated as top-level and the title
lookup and TopAppBar visibility remain correct.
- Around line 583-588: The intent-action-to-screen mapping in the remember block
is inverted: when intent?.action == ACTION_SEARCH it should navigate to
NavigationScreens.SEARCH and when intent?.action == ACTION_LIBRARY it should
navigate to NavigationScreens.LIBRARY; update the when expression inside the
remember block (the mapping that currently maps ACTION_SEARCH ->
NavigationScreens.LIBRARY and ACTION_LIBRARY -> NavigationScreens.SEARCH) to
swap those targets so each ACTION_* constant points to the matching
NavigationScreens.* value.
- Around line 857-864: The refactor unconditionally renders the History icon by
iterating NavigationScreens.getTopbarItems() despite the existing
showHistoryButton gate; update the rendering to respect showHistoryButton by
filtering or conditionalizing so NavigationScreens.HISTORY is only included when
showHistoryButton is true—e.g., in the block that currently loops over
NavigationScreens.getTopbarItems(), skip or exclude the item whose route equals
NavigationScreens.HISTORY (or check item == NavigationScreens.HISTORY) unless
showHistoryButton is true before creating the IconButton/navController.navigate
call.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AppearanceSettings.kt`:
- Around line 1456-1459: The Switch's onCheckedChange is currently returning a
NavigationItemPosition instead of invoking the state callback and the mapping is
inverted; update the onCheckedChange lambda to call
onListenTogetherPositionChange(...) and pass NavigationItemPosition.TOP_BAR when
checked is true and NavigationItemPosition.NAV_BAR when checked is false (use
the existing listenTogetherInTopBar, onListenTogetherPositionChange, Switch, and
NavigationItemPosition identifiers to locate the code).
In `@app/src/main/kotlin/com/metrolist/music/ui/utils/NavControllerUtils.kt`:
- Around line 10-12: The backToMain() loop currently keeps popping until there
is no previousBackStackEntry, which also removes the selected top-level tab;
change the loop to stop when the next entry is a top-level destination by
checking the destination id against the nav graph start/top-level ids. Update
NavController.backToMain() to pop while previousBackStackEntry != null AND
previousBackStackEntry.destination.id != graph.startDestinationId (or not in
your set of top-level destination ids), using previousBackStackEntry,
popBackStack(), and graph.startDestinationId (or your topLevelIds set) to detect
and preserve the current top-level tab.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/component/AppNavigation.kt`:
- Around line 42-46: The selection logic in isRouteSelected fails to keep Search
selected because NavigationScreens.SEARCH.route == "search_input" and routes
like "search/{query}" won't match startsWith("$screenRoute/"); update
isRouteSelected to special-case NavigationScreens.SEARCH.route (or when
screenRoute equals that value) and also return true when
currentRoute.startsWith("search/") (mirroring MainActivity's "search/" rule),
while keeping the existing exact-equals and startsWith("$screenRoute/") checks
for other screens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcbe0d0f-94ba-4739-a765-f5e263c71b6d
📒 Files selected for processing (9)
app/src/main/kotlin/com/metrolist/music/MainActivity.ktapp/src/main/kotlin/com/metrolist/music/constants/NavigationScreens.ktapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/ui/component/AppNavigation.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/ListenTogetherScreen.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/NavigationBuilder.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/Screens.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/AppearanceSettings.ktapp/src/main/kotlin/com/metrolist/music/ui/utils/NavControllerUtils.kt
💤 Files with no reviewable changes (2)
- app/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.kt
- app/src/main/kotlin/com/metrolist/music/ui/screens/Screens.kt
Comment on lines
+857
to
864
| NavigationScreens.getTopbarItems().forEach { item -> | ||
| IconButton(onClick = { navController.navigate(item.route) }) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.group_outlined), | ||
| contentDescription = stringResource(R.string.together), | ||
| painter = painterResource(item.iconIdInactive), | ||
| contentDescription = stringResource(item.titleId), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve the History visibility gate.
This generic loop renders NavigationScreens.HISTORY unconditionally. The old showHistoryButton rule is still computed above, so the refactor has brought the History icon back even when listen history is paused and empty.
Suggested fix
- NavigationScreens.getTopbarItems().forEach { item ->
+ NavigationScreens.getTopbarItems()
+ .filterNot { it == NavigationScreens.HISTORY && !showHistoryButton }
+ .forEach { item ->
IconButton(onClick = { navController.navigate(item.route) }) {
Icon(
painter = painterResource(item.iconIdInactive),
contentDescription = stringResource(item.titleId),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| NavigationScreens.getTopbarItems().forEach { item -> | |
| IconButton(onClick = { navController.navigate(item.route) }) { | |
| Icon( | |
| painter = painterResource(R.drawable.group_outlined), | |
| contentDescription = stringResource(R.string.together), | |
| painter = painterResource(item.iconIdInactive), | |
| contentDescription = stringResource(item.titleId), | |
| ) | |
| } | |
| } | |
| NavigationScreens.getTopbarItems() | |
| .filterNot { it == NavigationScreens.HISTORY && !showHistoryButton } | |
| .forEach { item -> | |
| IconButton(onClick = { navController.navigate(item.route) }) { | |
| Icon( | |
| painter = painterResource(item.iconIdInactive), | |
| contentDescription = stringResource(item.titleId), | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/MainActivity.kt` around lines 857 -
864, The refactor unconditionally renders the History icon by iterating
NavigationScreens.getTopbarItems() despite the existing showHistoryButton gate;
update the rendering to respect showHistoryButton by filtering or
conditionalizing so NavigationScreens.HISTORY is only included when
showHistoryButton is true—e.g., in the block that currently loops over
NavigationScreens.getTopbarItems(), skip or exclude the item whose route equals
NavigationScreens.HISTORY (or check item == NavigationScreens.HISTORY) unless
showHistoryButton is true before creating the IconButton/navController.navigate
call.
Comment on lines
10
to
12
| fun NavController.backToMain() { | ||
| val mainRoutes = Screens.MainScreens.map { it.route } | ||
|
|
||
| while (previousBackStackEntry != null && | ||
| currentBackStackEntry?.destination?.route !in mainRoutes | ||
| ) { | ||
| while (previousBackStackEntry != null) { | ||
| popBackStack() |
There was a problem hiding this comment.
Stop backToMain() at the current top-level destination.
This now pops the selected tab off the stack as well. A flow like Home -> Library -> album will long-press back to Home instead of Library, which is a regression from the previous “stop at main screen” behavior.
Suggested fix
import androidx.navigation.NavController
+import com.metrolist.music.constants.NavigationScreens
fun NavController.backToMain() {
- while (previousBackStackEntry != null) {
+ val topLevelRoutes = NavigationScreens.entries.map { it.route }.toSet() + "listen_together_from_topbar"
+ while (
+ previousBackStackEntry != null &&
+ currentBackStackEntry?.destination?.route !in topLevelRoutes
+ ) {
popBackStack()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/kotlin/com/metrolist/music/ui/utils/NavControllerUtils.kt`
around lines 10 - 12, The backToMain() loop currently keeps popping until there
is no previousBackStackEntry, which also removes the selected top-level tab;
change the loop to stop when the next entry is a top-level destination by
checking the destination id against the nav graph start/top-level ids. Update
NavController.backToMain() to pop while previousBackStackEntry != null AND
previousBackStackEntry.destination.id != graph.startDestinationId (or not in
your set of top-level destination ids), using previousBackStackEntry,
popBackStack(), and graph.startDestinationId (or your topLevelIds set) to detect
and preserve the current top-level tab.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/kotlin/com/metrolist/music/MainActivity.kt (2)
591-591:⚠️ Potential issue | 🟠 MajorTop-level screens and title lookup miss top-bar routes.
topLevelScreensis built only fromgetNavbarItems(), so routes likehistoryandstatsfromgetTopbarItems()are excluded. This affects:
shouldShowTopBarlogic (line 776-778) may incorrectly hide the top bar on these screenscurrentTitleReslookup (line 809-812) returnsnullfor top-bar routes, leaving the title empty🔧 Suggested fix
- val topLevelScreens = navigationItems.map{ it.route } + val topBarItems = NavigationScreens.getTopbarItems() + val topLevelScreens = remember(navigationItems, topBarItems) { + (navigationItems + topBarItems).map { it.route }.toSet() + } // ... later ... val currentTitleRes = remember(navBackStackEntry) { - navigationItems.firstOrNull{ it.route == navBackStackEntry?.destination?.route }?.titleId + (navigationItems + topBarItems) + .distinctBy { it.route } + .firstOrNull { it.route == navBackStackEntry?.destination?.route } + ?.titleId }Also applies to: 809-812
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/MainActivity.kt` at line 591, topLevelScreens is built from navigationItems (getNavbarItems()) only, so routes returned by getTopbarItems() (e.g., history, stats) are missing which breaks shouldShowTopBar and currentTitleRes lookups; update the construction of topLevelScreens to include both navigationItems and topbar items by merging routes from getNavbarItems() and getTopbarItems(), then use that combined set in shouldShowTopBar and when resolving currentTitleRes so top-bar routes are recognized and their titles shown.
857-864:⚠️ Potential issue | 🟡 MinorHistory button visibility gate is missing.
The refactored loop renders all items from
getTopbarItems()unconditionally, includingHISTORY. The existingshowHistoryButtonvariable (computed at lines 818-821) should filter out the History icon when listen history is paused and empty.🔧 Suggested fix
- NavigationScreens.getTopbarItems().forEach { item -> + NavigationScreens.getTopbarItems() + .filter { it != NavigationScreens.HISTORY || showHistoryButton } + .forEach { item -> IconButton(onClick = { navController.navigate(item.route) }) { Icon( painter = painterResource(item.iconIdInactive), contentDescription = stringResource(item.titleId), ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/MainActivity.kt` around lines 857 - 864, The loop rendering topbar icons indiscriminately uses NavigationScreens.getTopbarItems(); update it to respect the existing showHistoryButton flag by filtering out the HISTORY item when showHistoryButton is false. Locate the forEach that calls IconButton(navController.navigate(item.route)) and change the source collection to NavigationScreens.getTopbarItems().filter { it != NavigationScreens.HISTORY || showHistoryButton } (or equivalent check against item id/enum), so the History Icon is only rendered when showHistoryButton is true.
🧹 Nitpick comments (3)
app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AppearanceSettings.kt (1)
561-568: Consider null-safety for the position setter lookup.The
onNavigationItemPositionChange[defaultOpenTab]?.invoke(...)pattern is good, but if a newNavigationScreensentry is added without updating the map, this would silently fail.💡 Optional: Add logging or assertion for missing keys
onSelect = { // Reset the previously selected item to its default position - onNavigationItemPositionChange[defaultOpenTab]?.invoke(defaultOpenTab.default_position) + onNavigationItemPositionChange[defaultOpenTab]?.invoke(defaultOpenTab.default_position) + ?: run { /* Consider logging if key is missing */ } // Update defaultOpenTab onDefaultOpenTabChange(it) // Pin newly selected item to navigation bar - onNavigationItemPositionChange[it]?.invoke(NavigationItemPosition.NAV_BAR) + onNavigationItemPositionChange[it]?.invoke(NavigationItemPosition.NAV_BAR) + ?: run { /* Consider logging if key is missing */ } // Close menu showDefaultOpenTabDialog = false },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AppearanceSettings.kt` around lines 561 - 568, The current use of onNavigationItemPositionChange[defaultOpenTab]?.invoke(defaultOpenTab.default_position) can silently no-op if defaultOpenTab is not present in the map; update the logic in the block around onDefaultOpenTabChange to explicitly check for a mapping before invoking (e.g., if (onNavigationItemPositionChange.containsKey(defaultOpenTab)) { invoke(...) } else { log or throw/assert }), do the same for onNavigationItemPositionChange[it] when pinning the new item, and include a clear log message referencing defaultOpenTab/it and NavigationItemPosition so missing map entries are detected during development.app/src/main/kotlin/com/metrolist/music/constants/NavigationScreens.kt (2)
37-39: Use camelCase for thedefault_positionproperty.Kotlin convention uses camelCase for property names.
default_positionshould bedefaultPosition.♻️ Suggested refactor
enum class NavigationScreens( `@StringRes` val titleId: Int, `@DrawableRes` val iconIdInactive: Int, `@DrawableRes` val iconIdActive: Int, val route: String, val key: Preferences.Key<String>, val type: NavigationItemType, - val default_position: NavigationItemPosition + val defaultPosition: NavigationItemPosition ) {Then update all usages (e.g.,
it.default_position→it.defaultPosition).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/constants/NavigationScreens.kt` around lines 37 - 39, Rename the property default_position to camelCase defaultPosition in the NavigationScreens data holder declaration (the property with type NavigationItemPosition alongside key: Preferences.Key<String> and type: NavigationItemType), and update every usage site to reference defaultPosition (e.g., replace it.default_position with it.defaultPosition) so all callers and references compile with Kotlin naming conventions.
96-104: Composable state reads inside enum methods may cause recomposition overhead.
getPosition()andgetPositionSetter()subscribe to DataStore preferences. When called in loops (e.g.,getNavbarItems()), each call creates a separate state subscription. With only 6 items this is acceptable, but be mindful if the enum grows significantly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/constants/NavigationScreens.kt` around lines 96 - 104, getPosition() and getPositionSetter() each call rememberEnumPreference separately which creates multiple DataStore subscriptions when used in loops (e.g., getNavbarItems()); instead, hoist the preference read by replacing these two methods with a single `@Composable` helper (eg. rememberNavigationPositionPair/rememberPositionPair) that calls rememberEnumPreference(this.key, this.default_position) once and returns both the value and setter (value and component2) as a pair; update callers (like getNavbarItems) to call that helper once and pass the returned value and setter into item construction so you avoid repeated state subscriptions and extra recompositions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/kotlin/com/metrolist/music/MainActivity.kt`:
- Line 591: topLevelScreens is built from navigationItems (getNavbarItems())
only, so routes returned by getTopbarItems() (e.g., history, stats) are missing
which breaks shouldShowTopBar and currentTitleRes lookups; update the
construction of topLevelScreens to include both navigationItems and topbar items
by merging routes from getNavbarItems() and getTopbarItems(), then use that
combined set in shouldShowTopBar and when resolving currentTitleRes so top-bar
routes are recognized and their titles shown.
- Around line 857-864: The loop rendering topbar icons indiscriminately uses
NavigationScreens.getTopbarItems(); update it to respect the existing
showHistoryButton flag by filtering out the HISTORY item when showHistoryButton
is false. Locate the forEach that calls
IconButton(navController.navigate(item.route)) and change the source collection
to NavigationScreens.getTopbarItems().filter { it != NavigationScreens.HISTORY
|| showHistoryButton } (or equivalent check against item id/enum), so the
History Icon is only rendered when showHistoryButton is true.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/constants/NavigationScreens.kt`:
- Around line 37-39: Rename the property default_position to camelCase
defaultPosition in the NavigationScreens data holder declaration (the property
with type NavigationItemPosition alongside key: Preferences.Key<String> and
type: NavigationItemType), and update every usage site to reference
defaultPosition (e.g., replace it.default_position with it.defaultPosition) so
all callers and references compile with Kotlin naming conventions.
- Around line 96-104: getPosition() and getPositionSetter() each call
rememberEnumPreference separately which creates multiple DataStore subscriptions
when used in loops (e.g., getNavbarItems()); instead, hoist the preference read
by replacing these two methods with a single `@Composable` helper (eg.
rememberNavigationPositionPair/rememberPositionPair) that calls
rememberEnumPreference(this.key, this.default_position) once and returns both
the value and setter (value and component2) as a pair; update callers (like
getNavbarItems) to call that helper once and pass the returned value and setter
into item construction so you avoid repeated state subscriptions and extra
recompositions.
In
`@app/src/main/kotlin/com/metrolist/music/ui/screens/settings/AppearanceSettings.kt`:
- Around line 561-568: The current use of
onNavigationItemPositionChange[defaultOpenTab]?.invoke(defaultOpenTab.default_position)
can silently no-op if defaultOpenTab is not present in the map; update the logic
in the block around onDefaultOpenTabChange to explicitly check for a mapping
before invoking (e.g., if
(onNavigationItemPositionChange.containsKey(defaultOpenTab)) { invoke(...) }
else { log or throw/assert }), do the same for
onNavigationItemPositionChange[it] when pinning the new item, and include a
clear log message referencing defaultOpenTab/it and NavigationItemPosition so
missing map entries are detected during development.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba5faab1-aa73-4d48-acd1-a62be8dd520e
📒 Files selected for processing (3)
app/src/main/kotlin/com/metrolist/music/MainActivity.ktapp/src/main/kotlin/com/metrolist/music/constants/NavigationScreens.ktapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/AppearanceSettings.kt
Author
|
@nyxiereal any thoughts on this so we can then move on with #3284 ? |
Member
|
this goes beyond what we would like to have in the project, the current implementation is fine. there is also a feature freeze rn, so no. |
Author
|
@nyxiereal was the feature freeze for the new release or is it something else ? May I ask you to take a glance at #3284 as it is based on this, should I rewrite it without this or drop the work there ? This would be necessary for the app to feel usable to me... |
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
2 participants
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.
This PR implements a new NavigationScreens enum, used to store and configure the position of buttons for the main navigation screens in the interface (top bar, navigation bar, automatic, hidden), replacing the old Screens enum.
This PR does not implement any user-facing changes on its own, instead it will be used as the basis for upcoming PRs.
Implementation
The positioning logic is implemented as follows:
A NavigationItemPosition enum is used to store the position a given button should take in the interface:
A NavigationItemType enum is used to differentiate between different item types:
The home and library pages were implemented as NavigationScreens to simplify the codebase, hence they've been marked as CORE to avoid misconfigurations. CORE items have been hard-coded to always be included in NavigationScreens's getNavbarItems function
The NavigationScreens enum serves as the main class holding everything together, including the following information about each entry:
The NavigationScreens enum additionally has the following helper functions:
Current use
The app has been adapted to use this new mechanism to build the navigation bar and populate the top bar icons, with no changes to the settings or default behavior compared to the current state of the app. The search tab has been made visible by default and the positioning of the "Listen together" button bound to the "Listen together in the top bar" setting item.
Specifics
The following buttons are now implemented as NavigationScreens:
Future use
This PR is to be used in upcoming works in order to allow adding/removing items from the top bar, and/or pinning library pages or other items to the navigation bar, through a flexible and coherent underlying system.
Any reviews and comments welcome !
Cheers,
Lurux
Summary by CodeRabbit
Release Notes
Refactor
New Features