Skip to content

fix: back button moves up a folder instead of closing app - #226

Open
wernrab wants to merge 1 commit into
opencloud-eu:mainfrom
wernrab:fix_folder_up
Open

wernrab wants to merge 1 commit into
opencloud-eu:mainfrom
wernrab:fix_folder_up

Conversation

@wernrab

@wernrab wernrab commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

This is similar to oC #4943 but closes the FAB menu on root level as well.

@wernrab wernrab self-assigned this Sep 10, 2026
@wernrab
wernrab marked this pull request as ready for review September 10, 2026 14:23
Copilot AI lite review requested due to automatic review settings September 10, 2026 14:23
@guruz
guruz self-requested a review September 10, 2026 14:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The current “subfolder” detection uses parentId != ROOT_PARENT_ID, which can disable Back handling in legitimate subfolders (and enable it in unexpected cases), breaking the intended navigation behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates back-button handling in MainFileListFragment so that pressing Back at the root level closes the app (and also collapses the FAB menu when expanded), aligning behavior with the intent described in the PR description and referenced PR.

Changes:

  • Adds an OnBackPressedCallback to intercept Back presses and collapse the FAB menu when expanded.
  • Enables/disables the back-press callback based on whether the FAB is expanded or the user is inside a subfolder.
  • Refreshes the callback enabled-state when the displayed folder changes and when the FAB expand/collapse state changes.
File summaries
File Description
opencloudApp/src/main/java/eu/opencloud/android/presentation/files/filelist/MainFileListFragment.kt Adds Back-press callback to collapse the FAB menu first and otherwise browse up, with conditional enablement at root level.
Review details

Suppressed comments (1)

opencloudApp/src/main/java/eu/opencloud/android/presentation/files/filelist/MainFileListFragment.kt:517

  • isInSubfolder is derived from parentId != ROOT_PARENT_ID, but ROOT_PARENT_ID is also used as the root sentinel in browse-up logic (see manageBrowseUp()), so folders directly under root can have parentId == ROOT_PARENT_ID. In that case this callback will be disabled and Back will fall through to the Activity (exiting) instead of browsing up to root. Also, because parentId is nullable, null != ROOT_PARENT_ID evaluates to true and would incorrectly enable browse-up.

Derive the state from the actual folder being shown (e.g., remotePath != ROOT_PATH) so root is detected reliably.

    private fun updateBackPressedCallbackState() {
        val isInSubfolder = mainFileListViewModel.currentFolderDisplayed.value.parentId != ROOT_PARENT_ID
        onBackPressedCallback.isEnabled = isFabExpanded() || isInSubfolder
    }
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new back-press enablement logic treats a nullable parentId as “in subfolder,” which can enable the callback in invalid states and lead to a crash path in browse-up handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The current diff introduces a compilation issue in onViewCreated and the back-press enablement logic can disable navigation in folders whose parent is root.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new fragment-level back callback is likely ineffective because host activities override onBackPressed() and don’t delegate to the OnBackPressedDispatcher in the relevant paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

@bilogic

bilogic commented Oct 3, 2026

Copy link
Copy Markdown

can we merge this please?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

One or more issues must be addressed before approval.

3 open findings
1 resolved since last review

🧠 Review effort: Lite


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

One or more issues must be addressed before approval.

2 open findings
1 resolved since last review

🧠 Review effort: Lite


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The back callback can bypass preview cleanup and drawer priority, become stale when the FAB is hidden, and leave an incorrect accessibility description.

2 open findings

🧠 Review effort: Lite

@wernrab
wernrab requested a lite review from Copilot October 9, 2026 11:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Replacing the file-list fragment removes the Back callback without restoring it for the replacement.

1 open finding

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new menu-collapse paths leave screen readers announcing an incorrect expanded state.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused callback and menu-state changes preserve existing navigation behavior, with no unresolved blocking findings.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused changes preserve existing navigation behavior, with no confirmed blocking issues identified.

No open issues 🎉

Review effort: Balanced

fileIcon.setImageResource(MimetypeIconUtil.getFileTypeIconId(file.mimeType, file.fileName))

if (file.isImage) {
if (file.hasPreview || file.isImage) {
@wernrab

wernrab commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Beginning with API 33, the activities' onBackPressed() functions might no longer be called directly. A migration to OnBackPressedDispatcher was requested instead. FileDisplayActivity and FolderPickerActivity are fixed with this PR. Other (less important) activities are left unchanged so far.

A more detailed explanation by Copilot:

Activity.onBackPressed() is deprecated since API 33. How reliably it runs depends on the Android version and your targetSdk:

  • Android 12 and below (API ≤ 32): it is called for the back button, back gesture and nav-bar back, and works reliably.
  • Android 13+ (API 33+): it is only called if the app hasn't opted into predictive back. That means android:enableOnBackInvokedCallback is false or unset in the manifest. If it is set, the framework skips onBackPressed() and dispatches to OnBackInvokedCallback or OnBackPressedDispatcher instead.
  • Android 16 (API 36) with targetSdk 36: predictive back is on by default and onBackPressed() is never called. Apps can opt out only temporarily.
  • AndroidX  ComponentActivity: its onBackPressed() forwards to the  OnBackPressedDispatcher. If you override it without calling  super, your override runs only when the platform still routes to it, which is the "not directly called on some devices" case in the commit comment.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes reuse the existing navigation hierarchy, with no unresolved blocking issues identified.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused navigation and FAB changes have no identified blocking defects.

1 open issue

Review effort: Balanced

@bilogic

bilogic commented Oct 20, 2026

Copy link
Copy Markdown

sorry, i'm subscribed to this waiting for its completion and i keep seeing force-pushes, i mean.. is it going to end soon?

@wernrab
wernrab marked this pull request as ready for review October 20, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants