Skip to content

feat: organize settings into navigable categories - #102

Open
AvianJay wants to merge 2 commits into
mainfrom
feat/categorized-settings
Open

AvianJay wants to merge 2 commits into
mainfrom
feat/categorized-settings

Conversation

@AvianJay

@AvianJay AvianJay commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Settings previously displayed every control in one long list. Replace it with a category list that opens each section, with back navigation and restored scroll positions on both mobile and the desktop settings dialog.

The back arrow returns to the category list. Clicking outside the desktop dialog or pressing Escape closes Settings immediately; full-page settings retain system-back navigation through the category list.

Preserve the existing account and database pages, platform-specific options, and ad-toggle state. Add English and Traditional Chinese category descriptions, and move About version chips below the header so they fit narrow screens with larger text.

Validation:

  • Full Flutter suite passed: 484 tests, including desktop scrim/Escape dismissal, category navigation, persistence, scroll restoration, onboarding restart, Wear OS visibility, ads, localization, and large text.
  • Existing locale-dropdown tests now enter Appearance before changing the language.
  • AAB-specific test passed, confirming that the App Update category stays hidden.
  • flutter analyze --no-pub passed.
  • GitHub Verify passed, including the Chrome route geometry tests.
  • Visually checked rendered mobile and desktop category lists and appearance settings.

@AvianJay
AvianJay requested a review from Av1anJay October 4, 2026 04:24

@Av1anJay Av1anJay left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Changes Requested — 1 navigation regression and 1 failing-test regression.

  • On desktop, opening a category changes the enclosing DialogRoute pop disposition. Scrim clicks and Escape therefore return to the category catalog instead of dismissing Settings; users must dismiss twice. See the inline comment.
  • The full CI suite is red: test/bus_app_locale_scale_test.dart:100-101 and :174-180 still search for the locale dropdown immediately after opening Settings. It now exists only after entering Appearance. Update both tests to open Appearance first. The Verify job reports 477 passed, 2 failed, and every build job is skipped: https://github.com/YetAnotherBusDeveloper/yetanotherbusapp/actions/runs/37173920461/job/111352445847

The new mobile/desktop category navigation, state persistence, scroll restoration, platform visibility, and large-text tests otherwise pass.

Comment thread lib/screens/settings_screen.dart Outdated
);

return PopScope<void>(
canPop: category == null,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Navigation regression on the desktop dialog: when a category is selected this makes the enclosing DialogRoute report doNotPop. showDialog dismisses its scrim and Escape shortcut through Navigator.maybePop, so both actions are vetoed and onPopInvokedWithResult below merely returns to the category list. Repro at width ≥1100: open Settings → Appearance → click outside the dialog or press Escape; the dialog remains open, requiring a second dismissal. Please handle category-back without changing the dialog route's pop disposition, or explicitly distinguish desktop dismissal attempts from in-dialog back navigation.

@Av1anJay Av1anJay left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved — both blockers from the previous review are resolved on 60bd488d.

  • Desktop dismissal: canPop: isDialog || category == null no longer vetoes Navigator.maybePop, so scrim clicks and Escape close the settings dialog while the AppBar BackButton still returns to the category catalog. The new cases cover Escape and scrim both on the catalog and inside a category, plus full-page category back at desktop width.
  • Locale test regression: both cases in test/bus_app_locale_scale_test.dart now enter Appearance before looking for the locale dropdown, and the assertions still check the same item lists.

Checks: this host has no Flutter/Dart toolchain, so I could not run flutter analyze/flutter test locally — this is a static review of the full diff plus surrounding code, backed by CI. On 60bd488d the Verify job is green (Analyze ✅, Test ✅ full suite, Web route-geometry ✅); the platform build jobs were still queued when this review was written.

One trade-off, non-blocking: at width ≥1100 settings renders in a dialog, so on a wide Android tablet/Chromebook the system back gesture now dismisses the whole dialog instead of returning to the category list. That is inherent to the fix (system back and scrim/Escape all route through maybePop), and in-dialog category navigation is still covered by the AppBar back button.

return PopScope<void>(
// Scrim clicks and Escape dismiss the dialog. Category back is handled
// by the app bar there, and by system back on full-page settings.
canPop: isDialog || category == null,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking: this is true for any dialog route, so on a wide Android device (settings opens in a dialog at width ≥1100) the system back gesture now dismisses the entire dialog instead of returning to the category catalog. System back cannot be distinguished from scrim/Escape here because all three go through Navigator.maybePop, so the trade-off is reasonable — flagging it only so the behaviour change on that device class is a conscious choice. In-dialog category navigation is still available through the AppBar back button.

This branch has not been deployed

No deployments
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.

2 participants