diff --git a/Panel.qml b/Panel.qml index 7306b77..d153c62 100644 --- a/Panel.qml +++ b/Panel.qml @@ -27,6 +27,10 @@ Panel { property bool issuesExpanded: false property bool actionsExpanded: false property bool failuresExpanded: false + // Carry sub-notch wheel deltas between events. Touchpads emit many small + // angleDeltas; mice often emit a fake 1–2px pixelDelta that would otherwise + // crawl the dashboard a couple of pixels per click. + property real wheelAccumulator: 0 readonly property int activityPreviewCount: 5 readonly property int activityExpandedCount: 25 readonly property var metricFilters: [ @@ -78,12 +82,42 @@ Panel { } function activateCursor() { if (!selectedTarget) return - openUrl(selectedTarget.row.url) + openRow(selectedTarget.kind, selectedTarget.row.id, selectedTarget.row.url) + } + // Snapshot id/url before marking. hideNotification destroys the row, and + // reading linkRow.url after that leaves openUrl with an empty target. + function openRow(kind, id, url) { + var target = String(url || "") + var notificationId = String(id || "") + openUrl(target) + if (kind === "notification") github.markNotificationRead(notificationId) } function markSelectedRead() { - if (github.loading || github.marking) return if (selectedTarget && selectedTarget.kind === "notification") github.markNotificationRead(String(selectedTarget.row.id || "")) } + function applyPanelWheel(event) { + if (!panelFlick || (sortPicker && sortPicker.popup.visible)) return false + var maxY = Math.max(0, panelFlick.contentHeight - panelFlick.height) + if (maxY <= 0) return false + var pixel = event.pixelDelta.y + var angle = event.angleDelta.y + var wheel = Util.wheelSteps(root.wheelAccumulator, angle) + root.wheelAccumulator = wheel.remainder + // A mouse notch is 120°. Move about one dashboard row per notch. + if (wheel.steps !== 0) { + panelFlick.contentY = Math.max(0, Math.min(maxY, panelFlick.contentY - wheel.steps * Style.space(80))) + return true + } + // Touchpads report a real pixelDelta larger than Qt's angle conversion. + // Scale it so two-finger scroll matches the notch distance above. + if (pixel !== 0 && Math.abs(pixel) > Math.abs(angle) / 8) { + root.wheelAccumulator = 0 + panelFlick.contentY = Math.max(0, Math.min(maxY, panelFlick.contentY - pixel * 3)) + return true + } + // Swallow leftover high-res angle crumbs so Flickable cannot crawl 1–2px. + return angle !== 0 || pixel !== 0 + } function scrollItemIntoView(item) { if (!panelFlick || !item) return Qt.callLater(function() { @@ -113,7 +147,9 @@ Panel { function openUrl(url) { var value = String(url || "") if (value === "") return - Quickshell.execDetached(["omarchy-launch-browser", value]) + // Hand the URL to the existing browser so it becomes a tab. omarchy-launch-browser + // starts a new uwsm unit per click, which Brave/Chromium often turn into a window. + Quickshell.execDetached(["xdg-open", value]) close() } @@ -232,6 +268,16 @@ Panel { flickableDirection: Flickable.VerticalFlick interactive: contentHeight > height ScrollBar.vertical: ScrollBar { policy: ScrollBar.AsNeeded } + // Must be a direct child of Flickable or Qt keeps the default + // 1–2px wheel distance and this handler never runs. + WheelHandler { + acceptedDevices: PointerDevice.Mouse | PointerDevice.TouchPad + orientation: Qt.Vertical + grabPermissions: PointerHandler.CanTakeOverFromAnything + onWheel: function(event) { + if (root.applyPanelWheel(event)) event.accepted = true + } + } Column { id: content @@ -407,6 +453,7 @@ Panel { contentHeight: height clip: true flickableDirection: Flickable.HorizontalFlick + interactive: contentWidth > width Row { id: filterRow spacing: Style.space(6) @@ -752,7 +799,7 @@ Panel { hoverEnabled: true cursorShape: Qt.PointingHandCursor onEntered: root.selectKey(linkRow.cursorKey) - onClicked: root.openUrl(linkRow.url) + onClicked: root.openRow(linkRow.rowKind, linkRow.notificationId || linkRow.rowId, linkRow.url) } RowLayout { id: row @@ -799,7 +846,7 @@ Panel { } PanelActionButton { visible: linkRow.showReadAction - enabled: !github.loading && !github.marking + enabled: github.markingNotificationId !== linkRow.notificationId iconText: github.markingNotificationId === linkRow.notificationId ? "󰑐" : "󰄬" tooltipText: "Mark this notification read (M)" foreground: root.foreground diff --git a/README.md b/README.md index 9036874..1412d88 100644 --- a/README.md +++ b/README.md @@ -100,8 +100,8 @@ omarchy plugin remove robzolkos.github | --- | --- | | Left click Octocat | Open or close the dashboard | | Right or middle click Octocat | Refresh | -| Click a row | Open it on GitHub | -| Check button on a notification | Mark the thread read after GitHub confirms it | +| Click a row | Open it on GitHub; notification rows are also marked read | +| Check button on a notification | Mark the thread read immediately, then confirm with GitHub | | **Mark all read** in the notifications footer | Arm the bulk mark-as-read | | **Confirm?** on the armed button | Mark every notification on screen read | | `j` / `k` or arrow keys | Move through visible rows | diff --git a/Service.qml b/Service.qml index 1bb8985..fc8d029 100644 --- a/Service.qml +++ b/Service.qml @@ -34,6 +34,11 @@ Item { property string notificationActionStatus: "" property string _markStdout: "" property string _markStderr: "" + // Thread IDs waiting for PATCH after GitHub confirmed them locally. An + // in-flight refresh must not restore these rows, or the bar stays lit until + // the next poll even though the user already opened or marked the thread. + property var hiddenNotifications: ({}) + property var markQueue: [] // Single-thread and bulk marking share one process, so the panel gates every // entry point on this rather than on whichever flag a given call happens to // set. A caller added later inherits the guard instead of having to know. @@ -111,8 +116,108 @@ Item { return [helperPath(), "--include-archived", boolSetting("includeArchived", false) ? "true" : "false", "--include-forks", boolSetting("includeForks", false) ? "true" : "false", "--repository-scope", repositoryMode(), "--include-archived-reviews", boolSetting("includeArchivedReviewRequests", false) ? "true" : "false", "--include-draft-reviews", boolSetting("includeDraftReviewRequests", false) ? "true" : "false", "--action-scan", actionMode(), "--action-repo-limit", String(intSetting("actionScanRepoLimit", 15, 5, 200)), "--concurrency", String(intSetting("actionScanConcurrency", 6, 1, 12)), "--failed-days", String(intSetting("failedActionDays", 7, 1, 30)), "--failed-limit", String(intSetting("failedActionLimit", 20, 1, 100))]; } + function copyMap(value) { + var copy = {}; + var source = value || {}; + for (var key in source) + copy[key] = source[key]; + return copy; + } + + function hideNotification(id) { + var value = String(id || ""); + if (value === "") + return ; + + var hidden = copyMap(hiddenNotifications); + var next = []; + var found = false; + for (var i = 0; i < notifications.length; i++) { + var item = notifications[i]; + if (String(item.id || "") === value) { + hidden[value] = item; + found = true; + } else { + next.push(item); + } + } + if (!found && hidden[value] === undefined) + hidden[value] = {id: value}; + + hiddenNotifications = hidden; + if (found) { + notifications = next; + notificationsRevision++; + } + } + + function restoreHiddenNotification(id) { + var value = String(id || ""); + var item = hiddenNotifications[value]; + var hidden = copyMap(hiddenNotifications); + delete hidden[value]; + hiddenNotifications = hidden; + if (!item) + return ; + + for (var i = 0; i < notifications.length; i++) { + if (String(notifications[i].id || "") === value) + return ; + } + notifications = [item].concat(notifications); + notificationsRevision++; + } + + function visibleNotifications(rows) { + var incoming = Array.isArray(rows) ? rows : []; + var hidden = hiddenNotifications || {}; + var nextHidden = {}; + var visible = []; + for (var i = 0; i < incoming.length; i++) { + var item = incoming[i]; + var id = String(item.id || ""); + if (hidden[id]) + nextHidden[id] = item; + else + visible.push(item); + } + hiddenNotifications = nextHidden; + return visible; + } + + function enqueueMark(id) { + var value = String(id || ""); + if (value === "" || markingNotificationId === value) + return ; + + for (var i = 0; i < markQueue.length; i++) { + if (markQueue[i] === value) + return ; + } + markQueue = markQueue.concat([value]); + } + + function startQueuedMark() { + if (fetchProcess.running || markProcess.running || markQueue.length === 0) + return false; + + var value = String(markQueue[0] || ""); + markQueue = markQueue.slice(1); + if (value === "") + return startQueuedMark(); + + actionStatusTimer.stop(); + markingNotificationId = value; + notificationActionStatus = "Marking notification read…"; + _markStdout = ""; + _markStderr = ""; + markProcess.command = [helperPath(), "--mark-notification-read", value]; + markProcess.running = true; + return true; + } + function refresh() { - if (fetchProcess.running || markProcess.running) { + if (fetchProcess.running || markProcess.running || markQueue.length > 0) { refreshQueued = true; return ; } @@ -132,7 +237,7 @@ Item { login = String(data.login || ""); fetchedRepositoryScope = String(data.repositoryScope || "owned"); fetchedAt = String(data.fetchedAt || ""); - notifications = Array.isArray(data.notifications) ? data.notifications : []; + notifications = visibleNotifications(data.notifications); notificationsRevision++; reviewRequests = Array.isArray(data.reviewRequests) ? data.reviewRequests : []; assignedIssues = Array.isArray(data.assignedIssues) ? data.assignedIssues : []; @@ -152,16 +257,15 @@ Item { function markNotificationRead(id) { var value = String(id || ""); - if (value === "" || loading || fetchProcess.running || markProcess.running) + if (value === "") return ; - actionStatusTimer.stop(); - markingNotificationId = value; - notificationActionStatus = "Marking notification read…"; - _markStdout = ""; - _markStderr = ""; - markProcess.command = [helperPath(), "--mark-notification-read", value]; - markProcess.running = true; + // Drop the row before GitHub round-trips. Opening a thread while a + // refresh is already running used to no-op, so the icon stayed alarming + // until the next poll even after the user had seen the notification. + hideNotification(value); + enqueueMark(value); + startQueuedMark(); } function canonicalNotificationTimestamp(value) { @@ -277,6 +381,9 @@ Item { root.state = "error"; root.message = stderr !== "" ? stderr : "GitHub data refresh failed."; } + if (root.startQueuedMark()) + return ; + if (root.refreshQueued) { root.refreshQueued = false; Qt.callLater(root.refresh); @@ -311,15 +418,21 @@ Item { } catch (error) { } var all = root.markingAllNotifications; + var markedId = root.markingNotificationId; if (exitCode === 0 && response && response.state === "ready") { root.notificationActionStatus = all ? "Notifications marked read. Refreshing…" : "Notification marked read. Refreshing…"; } else { var fallback = all ? "Could not mark all notifications read." : "Could not mark notification read."; root.notificationActionStatus = response && response.message ? String(response.message) : String(markErrors.text || root._markStderr || fallback).trim(); + if (!all && markedId !== "") + root.restoreHiddenNotification(markedId); } root.markingNotificationId = ""; root.markingAllNotifications = false; actionStatusTimer.restart(); + if (root.startQueuedMark()) + return ; + // GitHub is authoritative after every attempt. This reconciles // successful, failed, and partially completed bulk operations. root.refreshQueued = false; diff --git a/tests/panel-source-test.sh b/tests/panel-source-test.sh index 6ea765f..de57852 100755 --- a/tests/panel-source-test.sh +++ b/tests/panel-source-test.sh @@ -8,6 +8,9 @@ fail() { echo "FAIL: $*" >&2; exit 1; } assert_contains() { [[ $PANEL_SOURCE == *"$1"* ]] || fail "$2" } +assert_not_contains() { + [[ $PANEL_SOURCE != *"$1"* ]] || fail "$2" +} assert_contains 'glyph: broken ? "󰅖" : (running ? "󰑮" : (checks === "SUCCESS" ? "󰄬" : ""))' \ "authored pull requests without checks do not use the pull request glyph" @@ -25,10 +28,27 @@ assert_contains $'onActionBusyChanged: if (section.actionBusy) section.disarmAct "bulk confirmation is not invalidated when notification state changes" assert_contains $'var confirmed = section.preparedAction\n section.disarmAction()\n section.actionTriggered(confirmed)' \ "bulk action does not submit the originally prepared snapshot" -assert_contains $'function markSelectedRead() {\n if (github.loading || github.marking) return' \ - "keyboard notification marking is enabled during refresh" -assert_contains $'PanelActionButton {\n visible: linkRow.showReadAction\n enabled: !github.loading && !github.marking' \ - "notification row marking is enabled during refresh" +assert_contains $'function activateCursor() {\n if (!selectedTarget) return\n openRow(selectedTarget.kind, selectedTarget.row.id, selectedTarget.row.url)' \ + "opening a notification from the keyboard does not mark it read" +assert_contains $'function openRow(kind, id, url) {\n var target = String(url || "")\n var notificationId = String(id || "")\n openUrl(target)\n if (kind === "notification") github.markNotificationRead(notificationId)' \ + "opening a notification marks it before launching the URL" +assert_contains $'function markSelectedRead() {\n if (selectedTarget && selectedTarget.kind === "notification") github.markNotificationRead(String(selectedTarget.row.id || ""))' \ + "keyboard notification marking is blocked during refresh" +assert_contains $'onClicked: root.openRow(linkRow.rowKind, linkRow.notificationId || linkRow.rowId, linkRow.url)' \ + "clicking a notification does not open and mark it read" +assert_contains 'Quickshell.execDetached(["xdg-open", value])' \ + "links are not handed to the existing browser" +assert_not_contains '["omarchy-launch-browser"' \ + "links still start a new uwsm browser unit" +assert_contains $'PanelActionButton {\n visible: linkRow.showReadAction\n enabled: github.markingNotificationId !== linkRow.notificationId' \ + "notification row marking is disabled during refresh" + +assert_contains $'function applyPanelWheel(event) {\n if (!panelFlick || (sortPicker && sortPicker.popup.visible)) return false' \ + "the panel still uses Flickable's default wheel distance" +assert_contains $'panelFlick.contentY = Math.max(0, Math.min(maxY, panelFlick.contentY - wheel.steps * Style.space(80)))' \ + "a mouse-wheel notch does not move about one row" +assert_contains $'ScrollBar.vertical: ScrollBar { policy: ScrollBar.AsNeeded }\n // Must be a direct child of Flickable or Qt keeps the default\n // 1–2px wheel distance and this handler never runs.\n WheelHandler {\n acceptedDevices: PointerDevice.Mouse | PointerDevice.TouchPad' \ + "the wheel handler is not a direct child of the panel Flickable" assert_contains 'github.fetchedRepositoryScope === "owned" ? "OWNED REPOSITORIES " : "REPOSITORIES "' \ "the repository heading does not follow the fetched scope" diff --git a/tests/service-source-test.sh b/tests/service-source-test.sh index b76d14f..5c45d35 100755 --- a/tests/service-source-test.sh +++ b/tests/service-source-test.sh @@ -21,12 +21,16 @@ assert_not_contains() { [[ $SERVICE_SOURCE != *"$1"* ]] || fail "$2" } -assert_contains $'function refresh() {\n if (fetchProcess.running || markProcess.running) {\n refreshQueued = true;\n return ;\n }' \ +assert_contains $'function refresh() {\n if (fetchProcess.running || markProcess.running || markQueue.length > 0) {\n refreshQueued = true;\n return ;\n }' \ "refresh and notification marking are not serialized" -assert_contains $'notifications = Array.isArray(data.notifications) ? data.notifications : [];\n notificationsRevision++;' \ +assert_contains $'notifications = visibleNotifications(data.notifications);\n notificationsRevision++;' \ "notification refreshes do not invalidate prepared confirmations" -assert_contains $'function markNotificationRead(id) {\n var value = String(id || "");\n if (value === "" || loading || fetchProcess.running || markProcess.running)' \ - "single-notification marking is not blocked during refresh" +assert_contains $'hideNotification(value);\n enqueueMark(value);\n startQueuedMark();' \ + "single-notification marking is dropped during refresh" +assert_contains 'notifications = [item].concat(notifications);' \ + "failed notification marking does not restore the hidden row" +assert_not_contains $'if (value === "" || loading || fetchProcess.running || markProcess.running)' \ + "single-notification marking is still blocked during refresh" assert_contains $'function canonicalNotificationTimestamp(value) {\n var text = String(value || "");\n if (!/^\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}Z$/.test(text))\n return "";' \ "notification boundaries are not shape validated" assert_contains 'return milliseconds <= Date.now() ? text : "";' \