Skip to content

feat: add held back-button routing #138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asturur wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add held back-button routing #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rust/zaparoo-core/src/input_actions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub mod actions {
pub const RIGHT: &str = "right";
pub const ACCEPT: &str = "accept";
pub const CANCEL: &str = "cancel";
pub const CANCEL_HOLD: &str = "cancel_hold";
pub const WRITE_CARD: &str = "write_card";
pub const DETAILS: &str = "details";
pub const PAGE_PREV: &str = "page_prev";
Expand Down
87 changes: 86 additions & 1 deletion src/ui/app/Main.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,45 @@ MainLayout {
Browse.AppState.active_screen = screen;
}

function _closeAllModals(): void {
root.hideCardWriteModal();
root.qrCodeModalVisible = false;
root.commercialNoticeModalVisible = false;
root.firstRunIndexModalVisible = false;
root.logUploadModalVisible = false;
root.quitConfirmModalVisible = false;
root.listPickerModalVisible = false;
root.settingNeedsRestartModalVisible = false;
root.contextMenuVisible = false;
root.contextMenuOwner = "";
root.contextMenuIndex = -1;
root.contextMenuMode = "main";
root.contextMenuEntries = [];
root._discoverParentEntries = [];
root._discoverMenuPending = false;
root.listPickerTitle = "";
root.listPickerEntries = [];
root.listPickerInitialId = "";
root.listPickerFieldId = "";
root._pendingLanguageSelection = "";
root._pendingResolutionSelection = "";
ScreenManager.modalStack = [];
}

function _forceHub(): void {
root._stopRepeat();
root._stopCancelHold();
root.gamesScreen.flushSelectedPersist();
root.pendingTransition = "";
if (root.activeScreen === root.screenHub) {
root._closeAllModals();
root.openQuitConfirmModal();
return;
}
root._closeAllModals();
root._goto(root.screenHub);
}

// Single-shot callback slots fired by the loadingChanged
// listeners below. Only one transition is in flight at a time
// (input gate guarantees this), so two scalars are enough.
Expand Down Expand Up @@ -1276,6 +1315,10 @@ MainLayout {
if (root._maybeDismissScreensaver())
return;
root._resetIdle();
if (action === "cancel_hold") {
root._forceHub();
return;
}
// Input gate. While a forward transition is in flight, swallow
// every press so a user mashing buttons during the loading
// wait can't queue a second transition or kick a half-cancel
Expand Down Expand Up @@ -1347,13 +1390,18 @@ MainLayout {
// active, just like fresh presses.
readonly property int _repeatInitialMs: 350
readonly property int _repeatTickMs: 90
readonly property int _cancelHoldMs: 1000
property string _heldAction: ""
property int _heldKey: 0
property bool _dispatchingRepeat: false
property bool _cancelPending: false
property bool _cancelHoldReady: false
property int _cancelHeldKey: 0
// Aliased so tst_navigation.qml can observe the repeat state machine
// — child Timer ids are file-scoped and aren't reachable otherwise.
property alias _repeatPending: repeatInitial.running
property alias _repeatTicking: repeatTick.running
property alias _cancelHoldPending: cancelHoldTimer.running

function _stopRepeat(): void {
repeatInitial.stop();
Expand All @@ -1370,10 +1418,24 @@ MainLayout {
root.gamesScreen.flushSelectedPersist();
}

function _stopCancelHold(): void {
cancelHoldTimer.stop();
root._cancelPending = false;
root._cancelHoldReady = false;
root._cancelHeldKey = 0;
}

function _isRepeatableAction(action: string): bool {
return action === "up" || action === "down" || action === "left" || action === "right";
}

function _armCancelHold(key: int): void {
root._cancelPending = true;
root._cancelHoldReady = false;
root._cancelHeldKey = key;
cancelHoldTimer.restart();
}

// State-machine half of handleKey: records the held key/action and
// arms the initial-delay timer. Pulled out of handleKey so unit
// tests can drive the repeat state machine without also routing
Expand All @@ -1399,6 +1461,10 @@ MainLayout {
const action = Browse.Input.action_for_key(key);
if (action === "")
return;
if (action === "cancel") {
root._armCancelHold(key);
return;
}
root.handleAction(action);
root._armRepeat(action, key);
}
Expand Down Expand Up @@ -1514,6 +1580,12 @@ MainLayout {
// a release of any other key in flight (a chord, an unrelated press
// mid-hold) is ignored.
function handleKeyRelease(key: int): void {
if (root._cancelPending && key === root._cancelHeldKey) {
const action = root._cancelHoldReady ? "cancel_hold" : "cancel";
root._stopCancelHold();
root.handleAction(action);
return;
}
if (root._heldAction !== "" && key === root._heldKey)
root._stopRepeat();
}
Expand All @@ -1524,6 +1596,17 @@ MainLayout {
root._dispatchingRepeat = false;
}

Timer {
id: cancelHoldTimer
interval: root._cancelHoldMs
repeat: false
onTriggered: {
if (!root._cancelPending)
return;
root._cancelHoldReady = true;
}
}
Comment on lines +1599 to +1608

@coderabbitai coderabbitai Bot May 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Timer only sets flag—tests expect navigation on timer fire, not on release.

The timer's onTriggered sets _cancelHoldReady = true but doesn't dispatch the action. The tests (test_cancel_hold_from_games_routes_directly_to_hub, test_cancel_hold_on_hub_opens_quit_confirm) use tryCompare before calling handleKeyRelease, expecting navigation to occur when the timer fires. With the current implementation, navigation only happens on key release.

If the intended UX is "navigate when hold threshold passes (key still held)," the timer should dispatch the action:

Proposed fix for timer-triggered dispatch 
 Timer {
     id: cancelHoldTimer
     interval: root._cancelHoldMs
     repeat: false
     onTriggered: {
         if (!root._cancelPending)
             return;
         root._cancelHoldReady = true;
+        root._stopCancelHold();
+        root.handleAction("cancel_hold");
     }
 }

Alternatively, if the intended UX is "navigate on release after holding 1 second," the tests need restructuring to call handleKeyRelease and use a wait() or tryCompare on a different property.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/ui/app/Main.qml` around lines 1599 - 1608, The timer currently only sets
root._cancelHoldReady but tests expect navigation immediately when the hold
threshold passes; update the Timer (cancelHoldTimer) onTriggered handler to not
only set root._cancelHoldReady but also invoke the same action/path used on key
release to perform navigation (i.e., call the existing cancel/release handler
that the key-release path uses) when root._cancelPending is true so navigation
occurs while the key is still held.


Timer {
id: cardWriteFailureTimer
interval: 1500
Expand Down Expand Up @@ -1561,8 +1644,10 @@ MainLayout {
// quirk) would leave the timer ticking forever. `root.active` is
// ApplicationWindow's own active property.
onActiveChanged: {
if (!root.active)
if (!root.active) {
root._stopRepeat();
root._stopCancelHold();
}
}

Item {
Expand Down
30 changes: 30 additions & 0 deletions tests/ui/tst_navigation.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ TestCase {
// run in microseconds, so the pending fire would land on the
// next test if we didn't reset it here.
main._stopRepeat();
main._stopCancelHold();
}

function test_initial_state_is_hub(): void {
Expand Down Expand Up @@ -95,13 +96,15 @@ TestCase {
function test_escape_on_games_returns_to_systems(): void {
main.activeScreen = main.screenGames;
main.handleKey(Qt.Key_Escape);
main.handleKeyRelease(Qt.Key_Escape);
compare(main.activeScreen, main.screenSystems);
}

// Escape on systems goes back to hub.
function test_escape_on_systems_returns_to_hub(): void {
main.activeScreen = main.screenSystems;
main.handleKey(Qt.Key_Escape);
main.handleKeyRelease(Qt.Key_Escape);
compare(main.activeScreen, main.screenHub);
}

Expand All @@ -118,9 +121,36 @@ TestCase {
function test_backspace_behaves_like_escape_on_games(): void {
main.activeScreen = main.screenGames;
main.handleKey(Qt.Key_Backspace);
main.handleKeyRelease(Qt.Key_Backspace);
compare(main.activeScreen, main.screenSystems);
}

function test_cancel_press_arms_hold_without_immediate_navigation(): void {
main.activeScreen = main.screenGames;
main.handleKey(Qt.Key_Escape);
compare(main.activeScreen, main.screenGames, "Back press must wait for release-or-hold timeout");
compare(main._cancelHoldPending, true);
}

function test_cancel_hold_from_games_routes_directly_to_hub(): void {
main.activeScreen = main.screenGames;
main.handleKey(Qt.Key_Escape);
tryCompare(main, "activeScreen", main.screenHub);
compare(main._cancelHoldPending, false, "Hold timer must clear after firing");
main.handleKeyRelease(Qt.Key_Escape);
compare(main.activeScreen, main.screenHub, "Release after long-hold must not fire a second cancel");
}

function test_cancel_hold_on_hub_opens_quit_confirm(): void {
main.activeScreen = main.screenHub;
main.handleKey(Qt.Key_Escape);
tryCompare(main, "quitConfirmModalVisible", true);
compare(main.activeScreen, main.screenHub, "Long-hold Back on Hub must stay on Hub and ask for quit");
compare(main._cancelHoldPending, false, "Hold timer must clear after opening quit confirm");
main.handleKeyRelease(Qt.Key_Escape);
compare(main.quitConfirmModalVisible, true, "Release after long-hold must not close the quit confirm");
}
Comment on lines +135 to +152

@coderabbitai coderabbitai Bot May 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Tests expect timer-triggered navigation but implementation dispatches on release.

Both test_cancel_hold_from_games_routes_directly_to_hub and test_cancel_hold_on_hub_opens_quit_confirm call tryCompare immediately after handleKey(Qt.Key_Escape), before any handleKeyRelease. This expects the timer's onTriggered to navigate/open the modal. However, the current cancelHoldTimer only sets _cancelHoldReady = true—the actual action dispatch happens in handleKeyRelease.

These tests will timeout because the screen/modal state never changes until release.

If the fix is applied to cancelHoldTimer (dispatching on timer fire), these tests are correct. If the intended behavior is release-triggered, restructure:

Alternative test structure for release-triggered behavior 
 function test_cancel_hold_from_games_routes_directly_to_hub(): void {
     main.activeScreen = main.screenGames;
     main.handleKey(Qt.Key_Escape);
+    wait(root._cancelHoldMs + 50);  // Wait past hold threshold
+    compare(main.activeScreen, main.screenGames, "Must not navigate while key is still held");
+    main.handleKeyRelease(Qt.Key_Escape);
-    tryCompare(main, "activeScreen", main.screenHub);
+    compare(main.activeScreen, main.screenHub);
     compare(main._cancelHoldPending, false, "Hold timer must clear after firing");
-    main.handleKeyRelease(Qt.Key_Escape);
-    compare(main.activeScreen, main.screenHub, "Release after long-hold must not fire a second cancel");
 }

This comment is related to the cancelHoldTimer issue flagged in Main.qml.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ui/tst_navigation.qml` around lines 135 - 152, The tests expect the
cancel-hold behavior to execute when the cancelHoldTimer fires, so update the
cancelHoldTimer handler in Main.qml to perform the actual navigation/modal open
(same logic currently in handleKeyRelease) instead of only setting
_cancelHoldReady; specifically, call the code that routes to screenHub or sets
quitConfirmModalVisible, clear/disable _cancelHoldPending/_cancelHoldReady and
stop the timer, and ensure handleKeyRelease checks those flags and returns early
(no-op) if the timer already executed to avoid double actions.


// Cross-row mapping. The test harness has no live CategoriesModel
// so we can't drive the full handleAction("down") flow with real
// categories — instead we unit-test the pure arithmetic helper
Expand Down
Loading