Code Restructuring and Consistent Bottom Bar Added #289
Conversation
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
…te screen structure
…te screen structure
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR restructures the app navigation and adds a consistent bottom navigation bar with dedicated tabs for Home, Tags, and Settings. The main changes move from a single-screen stack-based navigation to a multi-tab architecture with separate navigation stacks per top-level route.
- Introduces a new navigation framework with
TopLevelBackStackto manage multi-stack navigation - Adds a dedicated Tag Selection screen as a separate tab
- Moves QR code scanning functionality from the bottom bar to Settings
- Replaces the floating toolbar with a Material 3 bottom navigation bar
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| strings.xml | Adds "home" string resource for the new home tab label |
| TagSelectionScreen.kt | New full-screen tag management interface replacing the bottom sheet approach |
| TagSelectionBottomSheet.kt | Removes old bottom sheet component (replaced by dedicated screen) |
| Home.kt | Refactors to work with new navigation system; adds Dashboard2 route class and removes QR scanner from bottom bar |
| Navigation.kt | New navigation abstraction with TopLevelBackStack for managing multiple navigation stacks |
| MainActivity.kt | Implements bottom navigation bar with three tabs (Home, Tags, Settings) |
| Settings.kt, BackupScreen.kt, RestoreScreen.kt, etc. | Updates screen definitions to implement new TopLevelRoute or Screen interfaces |
| ScanQRVirtualScreen.kt | New file moving QR scanning to Settings as a menu item |
| Splash.kt | New splash screen for initial app navigation |
| DeeprApplication.kt | Simplifies ViewModel injection using viewModelOf |
| .editorconfig | Adds linting exceptions for new composition locals |
| package com.yogeshpaliyal.deepr.ui.screens.home | ||
|
|
||
| import android.database.sqlite.SQLiteConstraintException | ||
| import android.view.Surface |
There was a problem hiding this comment.
Unused import android.view.Surface. This import is not used anywhere in the file and should be removed.
Suggested change
| import android.view.Surface |
Comment on lines
+468
to
+475
| runBlocking { | ||
| try { | ||
| viewModel.updateTag(Tags(tag.id, trimmedName)) | ||
| Result.success(true) | ||
| } catch (e: Exception) { | ||
| return@runBlocking Result.failure(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid using runBlocking in UI code as it blocks the main thread and can cause UI freezes. Consider using rememberCoroutineScope() with scope.launch to handle this asynchronously, or move the tag update logic to the ViewModel.
| when (viewType) { | ||
| ViewType.LIST -> { | ||
| LazyColumn( | ||
| state = listState as? LazyListState ?: rememberLazyListState(), |
There was a problem hiding this comment.
The type cast listState as? LazyListState ?: rememberLazyListState() creates a new LazyListState on every recomposition when the cast fails. This defeats the purpose of the listState parameter. Consider validating the type earlier or using separate composables for different view types.
Comment on lines
+155
to
+463
| placeholder = { Text("Enter tag name") }, | ||
| singleLine = true, | ||
| shape = RoundedCornerShape(12.dp), | ||
| trailingIcon = | ||
| if (newTagName.isNotBlank()) { | ||
| { | ||
| ClearInputIconButton( | ||
| onClick = { | ||
| newTagName = "" | ||
| }, | ||
| ) | ||
| } | ||
| } else { | ||
| null | ||
| }, | ||
| ) | ||
|
|
||
| FilledIconButton( | ||
| onClick = { | ||
| val trimmedTagName = newTagName.trim() | ||
| if (trimmedTagName.isNotBlank()) { | ||
| val existingTag = | ||
| tagsWithCount.find { | ||
| it.name.equals( | ||
| trimmedTagName, | ||
| ignoreCase = true, | ||
| ) | ||
| } | ||
|
|
||
| if (existingTag != null) { | ||
| Toast | ||
| .makeText( | ||
| context, | ||
| context.getString(R.string.tag_name_exists), | ||
| Toast.LENGTH_SHORT, | ||
| ).show() | ||
| } else { | ||
| deeprQueries.insertTag(trimmedTagName) | ||
| newTagName = "" | ||
| Toast | ||
| .makeText( | ||
| context, | ||
| "Tag created successfully", | ||
| Toast.LENGTH_SHORT, | ||
| ).show() | ||
| } | ||
| } | ||
| }, | ||
| enabled = newTagName.isNotBlank(), | ||
| modifier = Modifier.size(56.dp), | ||
| ) { | ||
| Icon( | ||
| imageVector = TablerIcons.Plus, | ||
| contentDescription = stringResource(R.string.create_tag), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Show selected tags info | ||
| AnimatedVisibility(selectedTag.isNotEmpty()) { | ||
| Column { | ||
| Spacer(modifier = Modifier.height(12.dp)) | ||
| Card( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| colors = | ||
| CardDefaults.cardColors( | ||
| containerColor = MaterialTheme.colorScheme.primaryContainer, | ||
| ), | ||
| shape = RoundedCornerShape(8.dp), | ||
| ) { | ||
| Row( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .padding(12.dp), | ||
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| Text( | ||
| text = | ||
| stringResource( | ||
| R.string.selected_tags_count, | ||
| selectedTag.size, | ||
| ), | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| color = MaterialTheme.colorScheme.onPrimaryContainer, | ||
| ) | ||
| TextButton( | ||
| onClick = { viewModel.setTagFilter(null) }, | ||
| ) { | ||
| Text(stringResource(R.string.clear_all_filters)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Surface { | ||
| // Tags List | ||
| if (tagsWithCount.isEmpty()) { | ||
| // Empty State | ||
| Box( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxSize() | ||
| .padding(32.dp), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| Column( | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| verticalArrangement = Arrangement.Center, | ||
| ) { | ||
| Icon( | ||
| imageVector = TablerIcons.Tag, | ||
| contentDescription = null, | ||
| modifier = Modifier.size(64.dp), | ||
| tint = MaterialTheme.colorScheme.outline, | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Text( | ||
| text = "No tags yet", | ||
| style = MaterialTheme.typography.titleLarge, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| ) | ||
| Spacer(modifier = Modifier.height(8.dp)) | ||
| Text( | ||
| text = "Create your first tag to organize your links", | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| color = MaterialTheme.colorScheme.outline, | ||
| textAlign = TextAlign.Center, | ||
| ) | ||
| } | ||
| } | ||
| } else { | ||
| LazyColumn( | ||
| modifier = Modifier.fillMaxSize(), | ||
| contentPadding = | ||
| androidx.compose.foundation.layout | ||
| .PaddingValues(16.dp), | ||
| verticalArrangement = Arrangement.spacedBy(8.dp), | ||
| ) { | ||
| items(tagsWithCount.sortedBy { it.name }) { tag -> | ||
| val isSelected = selectedTag.any { it.id == tag.id } | ||
| Card( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .clickable { | ||
| viewModel.setTagFilter(Tags(tag.id, tag.name)) | ||
| }, | ||
| shape = RoundedCornerShape(12.dp), | ||
| colors = | ||
| CardDefaults.cardColors( | ||
| containerColor = | ||
| if (isSelected) { | ||
| MaterialTheme.colorScheme.surfaceContainerHighest | ||
| } else { | ||
| MaterialTheme.colorScheme.surfaceVariant | ||
| }, | ||
| ), | ||
| border = | ||
| if (isSelected) { | ||
| BorderStroke(1.dp, MaterialTheme.colorScheme.primary) | ||
| } else { | ||
| null | ||
| }, | ||
| ) { | ||
| Row( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .padding(12.dp), | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| androidx.compose.material3.Checkbox( | ||
| checked = isSelected, | ||
| onCheckedChange = { | ||
| viewModel.setTagFilter(Tags(tag.id, tag.name)) | ||
| }, | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.width(12.dp)) | ||
|
|
||
| Column(modifier = Modifier.weight(1f)) { | ||
| Text( | ||
| text = tag.name, | ||
| style = MaterialTheme.typography.titleMedium, | ||
| fontWeight = if (isSelected) FontWeight.SemiBold else FontWeight.Normal, | ||
| color = | ||
| if (isSelected) { | ||
| MaterialTheme.colorScheme.onPrimaryContainer | ||
| } else { | ||
| MaterialTheme.colorScheme.onSurface | ||
| }, | ||
| ) | ||
| Spacer(modifier = Modifier.height(4.dp)) | ||
| Text( | ||
| text = "${tag.linkCount} ${if (tag.linkCount == 1L) "link" else "links"}", | ||
| style = MaterialTheme.typography.bodySmall, | ||
| color = | ||
| if (isSelected) { | ||
| MaterialTheme.colorScheme.onPrimaryContainer.copy( | ||
| alpha = 0.7f, | ||
| ) | ||
| } else { | ||
| MaterialTheme.colorScheme.onSurfaceVariant | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| Row( | ||
| horizontalArrangement = Arrangement.spacedBy(4.dp), | ||
| ) { | ||
| IconButton( | ||
| onClick = { isTagEditEnable = tag }, | ||
| colors = | ||
| IconButtonDefaults.iconButtonColors( | ||
| contentColor = | ||
| if (isSelected) { | ||
| MaterialTheme.colorScheme.onPrimaryContainer | ||
| } else { | ||
| MaterialTheme.colorScheme.onSurfaceVariant | ||
| }, | ||
| ), | ||
| ) { | ||
| Icon( | ||
| imageVector = TablerIcons.Edit, | ||
| contentDescription = stringResource(R.string.edit_tag_description), | ||
| modifier = Modifier.size(20.dp), | ||
| ) | ||
| } | ||
|
|
||
| IconButton( | ||
| onClick = { isTagDeleteEnable = tag }, | ||
| colors = | ||
| IconButtonDefaults.iconButtonColors( | ||
| contentColor = MaterialTheme.colorScheme.error, | ||
| ), | ||
| ) { | ||
| Icon( | ||
| imageVector = TablerIcons.Trash, | ||
| contentDescription = stringResource(R.string.delete_tag_description), | ||
| modifier = Modifier.size(20.dp), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| isTagEditEnable?.let { tag -> | ||
| AlertDialog( | ||
| onDismissRequest = { | ||
| isTagEditEnable = null | ||
| tagEditError = null | ||
| }, | ||
| title = { | ||
| Text( | ||
| text = stringResource(R.string.edit_tag), | ||
| style = MaterialTheme.typography.headlineSmall, | ||
| ) | ||
| }, | ||
| text = { | ||
| Column { | ||
| OutlinedTextField( | ||
| value = tag.name, | ||
| onValueChange = { | ||
| isTagEditEnable = tag.copy(name = it) | ||
| tagEditError = null | ||
| }, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| label = { Text("Tag name") }, | ||
| singleLine = true, | ||
| isError = tagEditError != null, | ||
| supportingText = { | ||
| tagEditError?.let { | ||
| Text( | ||
| text = it, | ||
| color = MaterialTheme.colorScheme.error, | ||
| ) | ||
| } | ||
| }, | ||
| shape = RoundedCornerShape(12.dp), | ||
| trailingIcon = | ||
| if (isTagEditEnable?.name?.isNotBlank() == true) { | ||
| { | ||
| ClearInputIconButton( | ||
| onClick = { | ||
| isTagEditEnable = tag.copy(name = "") | ||
| }, | ||
| ) | ||
| } | ||
| } else { | ||
| null | ||
| }, | ||
| ) | ||
| } | ||
| }, | ||
| confirmButton = { | ||
| Button( | ||
| onClick = { | ||
| val trimmedName = isTagEditEnable?.name?.trim() ?: "" | ||
| if (trimmedName.isBlank()) { | ||
| tagEditError = "Tag name cannot be empty" |
There was a problem hiding this comment.
Hardcoded user-facing strings should be extracted to strings.xml for proper localization. The following strings need to be moved:
- "No tags yet" (line 277)
- "Create your first tag to organize your links" (line 283)
- "Enter tag name" (line 155)
- "Tag name" (line 431)
- "Tag name cannot be empty" (line 463)
Comment on lines
+538
to
+558
| append("Are you sure you want to delete ") | ||
| withStyle(style = SpanStyle(fontWeight = FontWeight.Bold)) { | ||
| append("'${tag.name}'") | ||
| } | ||
| append(" tag?") | ||
| } | ||
| Text(text = message) | ||
|
|
||
| if (tag.linkCount > 0) { | ||
| Spacer(modifier = Modifier.height(8.dp)) | ||
| Card( | ||
| colors = | ||
| CardDefaults.cardColors( | ||
| containerColor = | ||
| MaterialTheme.colorScheme.errorContainer.copy( | ||
| alpha = 0.3f, | ||
| ), | ||
| ), | ||
| ) { | ||
| Text( | ||
| text = "This tag is used by ${tag.linkCount} ${if (tag.linkCount == 1L) "link" else "links"}", |
There was a problem hiding this comment.
Hardcoded user-facing strings in the delete confirmation dialog need to be extracted to strings.xml:
- "Are you sure you want to delete " (line 538)
- " tag?" (line 542)
- "This tag is used by ${tag.linkCount} ${if (tag.linkCount == 1L) "link" else "links"}" (line 558)
| import androidx.compose.ui.graphics.vector.ImageVector | ||
| import androidx.navigation3.runtime.NavKey | ||
| import com.yogeshpaliyal.deepr.ui.screens.home.Dashboard2 | ||
| import kotlin.collections.remove |
There was a problem hiding this comment.
The unused import statement for kotlin.collections.remove should be removed. The remove function is already available on LinkedHashMap without this import.
Suggested change
| import kotlin.collections.remove |
| val removedKey = topLevelStacks[topLevelKey]?.removeLastOrNull() | ||
| // If the removed key was a top level key, remove the associated top level stack | ||
| topLevelStacks.remove(removedKey) | ||
| topLevelKey = topLevelStacks.keys.last() |
There was a problem hiding this comment.
Potential null pointer exception: topLevelStacks.keys.last() on line 89 can throw NoSuchElementException if topLevelStacks becomes empty after removing the last key. Add a check to prevent navigation state from becoming invalid.
Suggested change
| topLevelKey = topLevelStacks.keys.last() | |
| if (topLevelStacks.isNotEmpty()) { | |
| topLevelKey = topLevelStacks.keys.last() | |
| } |
| get() = TablerIcons.Home | ||
| override val label: Int | ||
| get() = R.string.home | ||
|
|
There was a problem hiding this comment.
[nitpick] Missing documentation for the Content function parameter. The purpose and expected behavior of the windowInsets parameter should be documented, especially since it's part of the public interface.
Suggested change
| /** | |
| * Displays the main content for the Home screen. | |
| * | |
| * @param windowInsets The [WindowInsets] to be applied to the content, representing the areas of the window that might be covered by system UI (such as status bars, navigation bars, or cutouts). Use this to properly pad or offset UI elements to avoid overlap with system decorations. | |
| */ |
📱 APK Build Complete!Your debug APK has been built successfully and is ready for testing. 📥 Download APKNote: Click the link above, scroll down to the "Artifacts" section, and download the Retention: This artifact will be available for 3 days. |
🧪 Integrated Test ResultsThe integrated UI tests have completed. View full test results: Test Run #19308218208 Test reports are available in the artifacts section of the workflow run. |
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.
Uh oh!
There was an error while loading. Please reload this page.