fix(dashboard): make system back mirror the AppBar back arrow#244
fix(dashboard): make system back mirror the AppBar back arrow#244deaflynx wants to merge 3 commits into
Conversation
The Android system back button (and iOS edge-swipe) now walks through dashboard state the same way the in-app AppBar back arrow does, instead of immediately popping the route or showing the exit dialog. Introduces a shared DashboardBackHandler that wraps each dashboard page in a PopScope and centralizes the back logic (close right layout -> WebView goBack -> page-specific fallback), reused by MainDashboardPage, HomeDashboardPage, SingleDashboardView and FullscreenDashboardPage. TwoPageViewController now exposes a reactive currentIndex so MainDashboardPage only intercepts back while the dashboard (not the list) is visible, letting the bottom-nav shell handle tab/exit back. Also replaces the "exit?" confirmation dialogs with a direct app exit to match standard Android apps (Gmail/YouTube/Slack), and removes the now-unused areYouSureYouWantToExit / confirmToCloseTheApp l10n keys.
vvlladd28
left a comment
vvlladd28
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 36 changed files in fix(dashboard): make system back mirror the AppBar back arrow. The consolidation into DashboardBackHandler is a nice cleanup and the pageIndex == 1 gating cleanly delegates the dashboards-list back press to the bottom-nav shell. A few small concerns left inline, mostly around the fullscreen-dashboard back press, mounted checks after async gaps, and a couple of API/style polish points.
This is an auto-generated review. Findings may contain errors — please verify before applying changes.
| } | ||
|
|
||
| Future<void> _handleBack() async { | ||
| await DashboardBackHandler.tryNavigateBack(_dashboardController); |
There was a problem hiding this comment.
Discarding the bool from tryNavigateBack means that when the WebView can't go back and the right panel is closed, _handleBack is a no-op — but the wrapping DashboardBackHandler still has canPop: false, so the system back gesture is silently swallowed. Previously the gesture at least bubbled up to the now-removed exit dialog, so users had some feedback. Consider falling back to if (mounted && context.canPop()) Navigator.of(context).pop(); (mirroring single_dashboard_view.dart) when tryNavigateBack returns false — otherwise a fullscreen dashboard with no WebView history has no system-back exit at all.
There was a problem hiding this comment.
Good catch — confirmed regression. Pre-PR this page had no PopScope, so the system back gesture popped the pushed GoRoute by default. The new DashboardBackHandler wraps the body with canPop: false, so when tryNavigateBack returns false (no right layout, no WebView history) the gesture was silently swallowed.
Fixed in 44b713c — added a Navigator.pop fallback after tryNavigateBack, mirroring SingleDashboardView._handleBack exactly.
| return; | ||
| } | ||
| await widget.controller.closeDashboard(); | ||
| _dashboardLoadingCtrl?.value = true; |
There was a problem hiding this comment.
After the two awaits above, the widget may have been disposed. single_dashboard_view.dart guards its post-await work with a mounted check; this one writes _dashboardLoadingCtrl?.value = true (and implicitly relies on widget.controller still being valid) without one. If the user backs out fast enough — or if the route is replaced concurrently — this can fire on a stale state. A if (!mounted) return; between the await widget.controller.closeDashboard(); and the assignment would make this consistent with the other handlers.
There was a problem hiding this comment.
Agreed. Added if (!mounted) return; between the await widget.controller.closeDashboard() and the _dashboardLoadingCtrl?.value = true write. Matches the SingleDashboardView pattern. Fixed in fc75a3c.
|
|
||
| static Future<bool> tryNavigateBack(DashboardController? controller) async { | ||
| if (controller == null) return false; | ||
| if (controller.rightLayoutOpened.value == true) { |
There was a problem hiding this comment.
Minor: controller.rightLayoutOpened.value is already a bool, so the == true here (and on line 23's await web.canGoBack()-style check via the surrounding pattern) is redundant. The old call sites used ?.value == true because of the nullable receiver, but controller is already null-checked one line above. Just if (controller.rightLayoutOpened.value) reads cleaner.
There was a problem hiding this comment.
Right — rightLayoutOpened is declared as a non-null ValueNotifier<bool> in DashboardController, so the == true is redundant. Dropped. Fixed in fc75a3c.
|
|
||
| class TwoPageViewController { | ||
| _TwoPageViewState? _state; | ||
| final ValueNotifier<int> currentIndex = ValueNotifier<int>(0); |
There was a problem hiding this comment.
currentIndex is exposed as a mutable ValueNotifier<int>, so any caller can call controller.currentIndex.value = 5 and silently desync it from _selectedIndex. Since the only intended public use is listening (see MainDashboardPage's ValueListenableBuilder<int>), consider exposing it as ValueListenable<int> publicly while keeping a private ValueNotifier<int> for internal updates — that locks the source of truth to _open/_close/onPageChanged.
There was a problem hiding this comment.
Agreed — exposing the ValueNotifier publicly lets outside callers mutate the source of truth, which we don't want here. Made the backing field private (_currentIndex) and exposed a read-only ValueListenable<int> get currentIndex. The only external consumer (MainDashboardPage's ValueListenableBuilder<int>) is unaffected since it only reads. The three internal writers in _TwoPageViewState (_open, _close, onPageChanged) now go through the private field. Fixed in fc75a3c.
- Guard MainDashboardPage._handleBack with a mounted check after awaiting closeDashboard, mirroring SingleDashboardView. - Drop redundant `== true` on DashboardController.rightLayoutOpened.value. - Expose TwoPageViewController.currentIndex as a read-only ValueListenable so external listeners cannot mutate the source.
DashboardBackHandler's PopScope (canPop: false) was swallowing the system back gesture on FullscreenDashboardPage when tryNavigateBack returned false (no right layout, no WebView history), leaving the user stuck. Mirror SingleDashboardView._handleBack and fall back to Navigator.pop so deep-linked / public-user entries can exit the page.
2 participants
Summary
The Android system back button (and iOS edge-swipe) now navigates through dashboard state exactly like the in-app AppBar back arrow, instead of immediately popping the route or showing the exit dialog.
Addresses the remaining system-back behavior reported in thingsboard/flutter_thingsboard_pe_app#285 — the AppBar arrow was already fixed in v1.8.0, but the hardware/gesture back still diverged.
Changes
DashboardBackHandler— wraps each dashboard page in aPopScopeand centralizes the back logic (close right layout → WebViewgoBack()→ page-specific fallback). Reused byMainDashboardPage,HomeDashboardPage,SingleDashboardView, andFullscreenDashboardPage, so the AppBar arrow and system back share one code path.TwoPageViewController.currentIndex—MainDashboardPagenow only intercepts back while the dashboard (not the dashboards list) is visible, letting the bottom-nav shell handle tab-switch/exit back as before.areYouSureYouWantToExit/confirmToCloseTheAppl10n keys across all locales.Test plan