ADFA-3569 | Refactor editor fullscreen state handling#1149
Merged
Conversation
jatezzz
requested review from
a team,
Daniel-ADFA,
dara-abijo-adfa and
itsaky-adfa
Contributor
📝 Walkthrough
WalkthroughReplaces the landscape-specific immersive controller with a new FullscreenManager and StateFlow-driven fullscreen UI state in EditorViewModel; updates BaseEditorActivity wiring, gestures, insets, bottom-sheet anchoring, layouts, icons, and strings to support unified fullscreen behavior across orientations. Changes
Sequence DiagramsequenceDiagram
participant User
participant BaseEditorActivity
participant FullscreenManager
participant EditorViewModel
participant AppBarLayout
participant BottomSheetBehavior
User->>BaseEditorActivity: onCreate()
BaseEditorActivity->>FullscreenManager: construct + bind()
FullscreenManager->>AppBarLayout: register OnOffsetChangedListener()
FullscreenManager->>BottomSheetBehavior: add BottomSheetCallback()
FullscreenManager->>BaseEditorActivity: closeDrawerAction / onFullscreenToggleRequested callbacks
BaseEditorActivity->>EditorViewModel: setupFullscreenObserver() (STARTED)
EditorViewModel-->>BaseEditorActivity: uiState.isFullscreen updates
User->>FullscreenManager: tap fullscreen toggle
FullscreenManager->>EditorViewModel: onFullscreenToggleRequested()
EditorViewModel->>EditorViewModel: toggleFullscreen()
EditorViewModel-->>BaseEditorActivity: emit new uiState
BaseEditorActivity->>FullscreenManager: render(isFullscreen, animate)
FullscreenManager->>AppBarLayout: expand/collapse & animate alpha
FullscreenManager->>BottomSheetBehavior: set hideable/state & adjust margin
FullscreenManager->>BaseEditorActivity: close drawer (if needed)
User->>BaseEditorActivity: fling from edge (dismiss)
BaseEditorActivity->>EditorViewModel: exitFullscreen()
EditorViewModel-->>FullscreenManager: render(isFullscreen=false, animate=true)
BaseEditorActivity->>FullscreenManager: preDestroy()/destroy()
FullscreenManager->>AppBarLayout: unregister listener
FullscreenManager->>BottomSheetBehavior: remove callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt (1)
62-67: Consider consolidating to a single state source.The fullscreen state is maintained in both
_isFullscreen(LiveData) and_uiState(StateFlow). WhileupdateFullscreenkeeps them synchronized, this dual-source pattern increases maintenance overhead and risk of divergence.If the LiveData is still needed for legacy observers, consider deriving it from the StateFlow or document why both are required. Otherwise, consolidating to StateFlow alone would simplify the state management.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt` around lines 62 - 67, Consolidate fullscreen state into the single source _uiState: remove the backing MutableLiveData _isFullscreen and have updateFullscreen update only _uiState (UiState.fullscreen flag); if legacy LiveData observers are required, expose a derived LiveData by converting _uiState (e.g., via asLiveData or a mapping) so observers read from that derived LiveData instead of a separate _isFullscreen, ensuring updateFullscreen no longer needs to sync two sources and eliminating divergence risk.app/src/main/java/com/itsaky/androidide/utils/WindowInsetsExtensions.kt (1)
109-113: Unused parameterorientationshould be removed.The
orientationparameter is no longer used after simplifying to always callresetOffsetAnchor(). This creates a misleading API signature.♻️ Proposed fix to remove unused parameter
-fun ContentEditorBinding.applyBottomSheetAnchorForOrientation(orientation: Int) { +fun ContentEditorBinding.applyBottomSheetAnchor() { bottomSheet.resetOffsetAnchor() }Note: This would also require updating the call sites in
BaseEditorActivity.kt(lines 679 and 1402).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/utils/WindowInsetsExtensions.kt` around lines 109 - 113, Remove the unused orientation parameter from the function signature of ContentEditorBinding.applyBottomSheetAnchorForOrientation and update all call sites to the new zero-arg function; specifically, change the method to take no parameters and keep the body as bottomSheet.resetOffsetAnchor(), then update callers (e.g., in BaseEditorActivity where applyBottomSheetAnchorForOrientation(...) is invoked) to call applyBottomSheetAnchorForOrientation() with no argument. Ensure method references/imports/overloads are adjusted accordingly so there are no compilation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/utils/WindowInsetsExtensions.kt`:
- Around line 109-113: Remove the unused orientation parameter from the function
signature of ContentEditorBinding.applyBottomSheetAnchorForOrientation and
update all call sites to the new zero-arg function; specifically, change the
method to take no parameters and keep the body as
bottomSheet.resetOffsetAnchor(), then update callers (e.g., in
BaseEditorActivity where applyBottomSheetAnchorForOrientation(...) is invoked)
to call applyBottomSheetAnchorForOrientation() with no argument. Ensure method
references/imports/overloads are adjusted accordingly so there are no
compilation errors.
In `@app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt`:
- Around line 62-67: Consolidate fullscreen state into the single source
_uiState: remove the backing MutableLiveData _isFullscreen and have
updateFullscreen update only _uiState (UiState.fullscreen flag); if legacy
LiveData observers are required, expose a derived LiveData by converting
_uiState (e.g., via asLiveData or a mapping) so observers read from that derived
LiveData instead of a separate _isFullscreen, ensuring updateFullscreen no
longer needs to sync two sources and eliminating divergence risk.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecb6ff41-e949-4b60-9a4a-162f92e75e9c
📒 Files selected for processing (13)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/FullscreenManager.ktapp/src/main/java/com/itsaky/androidide/activities/editor/LandscapeImmersiveController.ktapp/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.ktapp/src/main/java/com/itsaky/androidide/utils/WindowInsetsExtensions.ktapp/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.ktapp/src/main/res/drawable/bg_hollow_green_circle.xmlapp/src/main/res/layout-land/content_editor.xmlapp/src/main/res/layout/content_editor.xmlresources/src/main/res/drawable/ic_fullscreen.xmlresources/src/main/res/drawable/ic_fullscreen_exit.xmlresources/src/main/res/values/dimens.xmlresources/src/main/res/values/strings.xml
💤 Files with no reviewable changes (2)
- app/src/main/res/drawable/bg_hollow_green_circle.xml
- app/src/main/java/com/itsaky/androidide/activities/editor/LandscapeImmersiveController.kt
itsaky-adfa
requested changes
Apr 6, 2026
Contributor
itsaky-adfa
left a comment
itsaky-adfa
left a comment
There was a problem hiding this comment.
When the bottom sheet is expanded, we used to have the toolbar visible and the bottom sheet anchored below it. That behavior has been removed and the bottom sheet now spans the full screen instead. Is that intentional?
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/activities/editor/FullscreenManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/activities/editor/FullscreenManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.kt
Outdated
Show resolved
Hide resolved
jatezzz
force-pushed
the
feat/ADFA-3569-fullscreen-portrait-and-landscape
branch
from
23ff5b6 to
5a8bcdb
Compare
Collaborator
Author
Great catch! That was an oversight during the refactor, but it led to a nice UX improvement. I've updated the logic to be orientation-aware: in portrait mode, the bottom sheet is anchored again so the toolbar remains visible, just like before. In landscape, it now removes the anchor to maximize the limited vertical screen real estate. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/activities/editor/FullscreenManager.kt (1)
228-241: Consider settingisHideable = falseafter bottom sheet reaches collapsed state.When exiting fullscreen, if the bottom sheet is already collapsed,
isHideableis set tofalseimmediately. However, if the state transition is animated, the sheet might not be collapsed yet when this code runs. The bottom sheet callback at Line 189-191 handles this case, but the logic split between two locations could be confusing.💡 Optional: Consolidate hideable management in the callback
Consider moving all
isHideable = falselogic tohandleBottomSheetStateChangefor consistency:private fun applyNonFullscreen(animate: Boolean) { topBar.setExpanded(true, animate) appBarContent.alpha = 1f if (bottomSheetBehavior.state != BottomSheetBehavior.STATE_COLLAPSED) { bottomSheetBehavior.state = BottomSheetBehavior.STATE_COLLAPSED - } else { - bottomSheetBehavior.isHideable = false } + // Note: isHideable is set to false in handleBottomSheetStateChange when collapsed editorContainer.updateLayoutParams<ViewGroup.MarginLayoutParams> { bottomMargin = defaultEditorBottomMargin } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/activities/editor/FullscreenManager.kt` around lines 228 - 241, The current applyNonFullscreen(animate: Boolean) sets bottomSheetBehavior.isHideable = false immediately when the sheet is already collapsed, but when the collapse is animated the sheet may not yet be collapsed; instead consolidate hideable management into the BottomSheet callback (handleBottomSheetStateChange) so isHideable is set only once the state reaches BottomSheetBehavior.STATE_COLLAPSED, and in applyNonFullscreen only set bottomSheetBehavior.state = BottomSheetBehavior.STATE_COLLAPSED (never change isHideable); if you want to preserve the existing fast-path for already-collapsed state, check bottomSheetBehavior.state == BottomSheetBehavior.STATE_COLLAPSED and then set isHideable = false, otherwise rely on handleBottomSheetStateChange to set isHideable when the collapsed state is reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/FullscreenManager.kt`:
- Around line 228-241: The current applyNonFullscreen(animate: Boolean) sets
bottomSheetBehavior.isHideable = false immediately when the sheet is already
collapsed, but when the collapse is animated the sheet may not yet be collapsed;
instead consolidate hideable management into the BottomSheet callback
(handleBottomSheetStateChange) so isHideable is set only once the state reaches
BottomSheetBehavior.STATE_COLLAPSED, and in applyNonFullscreen only set
bottomSheetBehavior.state = BottomSheetBehavior.STATE_COLLAPSED (never change
isHideable); if you want to preserve the existing fast-path for
already-collapsed state, check bottomSheetBehavior.state ==
BottomSheetBehavior.STATE_COLLAPSED and then set isHideable = false, otherwise
rely on handleBottomSheetStateChange to set isHideable when the collapsed state
is reached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f352448-7ea0-4be8-b403-1896a10c31d1
📒 Files selected for processing (13)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/FullscreenManager.ktapp/src/main/java/com/itsaky/androidide/activities/editor/LandscapeImmersiveController.ktapp/src/main/java/com/itsaky/androidide/ui/EditorBottomSheet.ktapp/src/main/java/com/itsaky/androidide/utils/WindowInsetsExtensions.ktapp/src/main/java/com/itsaky/androidide/viewmodel/EditorViewModel.ktapp/src/main/res/drawable/bg_hollow_green_circle.xmlapp/src/main/res/layout-land/content_editor.xmlapp/src/main/res/layout/content_editor.xmlresources/src/main/res/drawable/ic_fullscreen.xmlresources/src/main/res/drawable/ic_fullscreen_exit.xmlresources/src/main/res/values/dimens.xmlresources/src/main/res/values/strings.xml
💤 Files with no reviewable changes (2)
- app/src/main/res/drawable/bg_hollow_green_circle.xml
- app/src/main/java/com/itsaky/androidide/activities/editor/LandscapeImmersiveController.kt
✅ Files skipped from review due to trivial changes (3)
- resources/src/main/res/values/dimens.xml
- resources/src/main/res/drawable/ic_fullscreen.xml
- resources/src/main/res/drawable/ic_fullscreen_exit.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/utils/WindowInsetsExtensions.kt
itsaky-adfa
approved these changes
Apr 7, 2026
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.
Description
Refactored the editor's immersive mode by replacing the orientation-specific
LandscapeImmersiveControllerwith a universalFullscreenManager. Fullscreen state is now centrally managed and observed viaEditorViewModelusing aStateFlow. Additionally, the disparate top and bottom bar toggle buttons were replaced with a single, unified fullscreen toggle button.Details
Dex/Tablet
document_5075574213318805643.mp4
Phone
Screen_Recording_20260402_161348_Code.on.the.Go.mp4
Ticket
ADFA-3569
Observation
The previous implementation restricted immersive capabilities to landscape mode and relied on direct view manipulation. Moving the state to the ViewModel and utilizing a dedicated
FullscreenManagercreates a more predictable, reactive, and orientation-independent user experience.