U5: row context menu on right-click #15
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "upgrade/u5-ctxmenu"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Right-clicking a history row opens a menu of actions for that interval. Three
things that needed a terminal before.
Where it can be bound is not a choice
pointerContexthas exactly one producer in the shell —ui_tree_reconciler.cpp:1385,inside
setOnRightClickWithPointer— andpanel.openContextMenureturnsfalsewithout it (
plugin_bindings.cpp:760-765).onRightClickis in turn absent from thekFlexandkBoxwhitelists, so no container can host a menu. That leaves the row'stag button, which is a
ui.buttononly becauseonClickneeded one; the dot beside itis a
ui.boxand cannot share this the way it shares the click.onActivatemust be a named global string, not a closure(
plugin_bindings.cpp:650, max 128 bytes). So this PR reintroduces exactly one namedglobal — the thing #13 deleted 27 of — deliberately, and routes every row through it
using the request's
context. There is no per-row name and no numbering, which is whatkeeps the list uncapped.
Ruling: stable identifier, not a row index
timew's ids move.
@Ncounts backwards from the newest interval, so every insertrenumbers every id. Verified live rather than reasoned about:
An interval's start is the identifier that does not move: timew forbids overlap, so
no two intervals share one, and nothing in this panel modifies a start.
Parse.intervalnow keeps the whole timestamp beside the two clipped halves the listdisplays, and the menu carries it in
context.Positional is safe at open time and wrong at activate time, which is why the index
is spent in one place and the identifier crosses into the other:
openRowMenu(i)indexesstate.shown, exactly asfilterFromRow(i)does.Safe by the same invariant: the shell resolves a closure out of the handler table the
newest render installed (
luau_host.cpp:3084-3090) andcommitpublishesstate.shownoff the very model it renders, so the closure and the list it indexesare always from one commit.
tick and on all four state watches, so by the time an item is chosen
state.shownmaybe shorter, reordered, or a different day's — while the menu still shows the row that
was right-clicked. An index would still be
2; it would name something else.Timew.cmd.byStartcloses the remaining window by making the resolve and the act oneprocess: the walk finds the interval whose start matches
$2andexecs timew onthat id, so the number never returns to Luau to go stale. A row already deleted from a
terminal resolves to nothing and says so — it cannot resolve to a different interval.
Two subprocesses, not one per id: the starts arrive from a single
timew getover everyreference, and a token's position in that output is the id. 70 ms over 66 intervals,
against ~950 ms at
ALL_WALK_MAXfor the per-id spelling andTimew.exec's 3 s timeout.The bound is
ALL_WALK_MAXand the suite reads it out of both scripts, so a resolvewalk shorter than the list walk fails rather than leaving the oldest rows on screen
unactionable.
Ruling: no confirmation, and why that is not an omission
Backed by
timew help delete,timew help continue,timew help undoon 1.9.1.A two-step menu is structurally impossible.
onActivateis enqueued withmakeScriptSnapshot()(plugin_panel.cpp:406-423), and that function leavespointerContextunset (:471-478) — the UI-callback path is the only one that fills itin, at
:173-182. So a secondopenContextMenufrom an activation returnsfalse.There is no dialog primitive either. An "are you sure" step has nowhere to be drawn.
So the mis-click is made cheap and legible instead of impossible, on three counts:
the wrong row is visible before anything is chosen.
an over-travelled pointer lands; undo sits there instead.
Undo is gated on
state.undoable, the oneenabledflag this menu writes down. Thejournal is global and older than the panel, so offering it unconditionally would invite a
user to revert something done in a terminal an hour ago with no way to see what it was.
Gated, the item means "undo what I just did here". It is cleared after undoing, because
the entry underneath belongs to whoever made it and
timew undocannot itself be undone.Ruling:
continue @id, notstart <tag>A row shows one string where an interval can carry several tags — timew's DOM prints
them as
alpha "deep work"— sotimew start <row.tag>would create a single tagliterally called
alpha "deep work".timew continue @idcopies the whole set:It also stops whatever is running first (exit 0, no error), which is what switching tasks
means in a time tracker — so it is offered while tracking rather than greyed out.
Refresh after acting
Every action goes through the existing
run, which already refetches and pokes the bar.No second path.
rungrew one parameter for the undo flag and now readsTimew.exec'sok, so a failed command arms nothing.Trust boundary
A tag comes out of timew's database, which is to say out of whatever the user has ever
typed at a shell, and a label over 512 bytes makes the binding raise rather than
return (
plugin_bindings.cpp:638-640) — taking the handler down with it. Tags areclipped to 120 bytes for the menu; a 3000-byte tag is in the suite.
byStartcarries three data elements, one of them a translated sentence — a locale is afile on disk, so the hostile-bytes assert is pointed at the message slot as well as the
start.
Files
timew/parse.luauis in the diff beyond the task's file list, and unavoidably: the rowhad nowhere to keep its identity.
Parse.intervalgains one nested capture and one field;date,at,durationandtagare byte-identical.Gates
plugin_apistays at 3 — the context menu needs no bump, it already works there. Noformatting settings, no gestures, no bar changes.
Mutation results — 13 applied, 13 killed
context = row.start→context = ithe context is row 2's own start, not its index: "2"openRowMenu(i)→openRowMenu(1)at the call sitethe context is row 2's own start...: "2026-08-31T17:48:53"separatoritemheader, continue, separator, delete, undo: got 4enabled = state.undoable→enabled = truewith the journal popped, undo is disabled rather than absentrun(Timew.cmd.undo(), false)→run(Timew.cmd.undo())undoing pops the journal, so the entry underneath is no longer this panel's to offerapply({ undoable = undoable ~= false })→ no-opa landed stop of this panel's is a change it may offer backrefresh()→fetchDays()inrunthe delete refetched through refresh: 1 commands, not 4refresh()→ nothing inrunno queued command matching /bin/sh -c for i in $(seq 0 13)...id == "continue" or id == "delete"guardonActivate acted on the id "start", which this menu never published#tag <= 120→#tag <= 6000items[1].label is 3023 bytes, not 1..512-gt ALL_WALK_MAX→-gt DAY_WALK_MAXinbyStartdelete by start: argv[3] is ... -gt 60 ...start = start→start = dateinParse.intervalthe row keeps its whole start: 2026-08-31What a reviewer should challenge hardest
undoableas panel state. It is the first field instatethat no fetch answersand no
viewModelfield carries — the menu reads it directly, the way handlers readstate.shown. If "the journal is global, so only offer this panel's own change" isthe wrong call, the alternative is offering undo unconditionally and the field goes.
separator + position +
timew undo" being enough is a judgement. The remaining holeis a delete followed by a write from a terminal before the user reaches for undo — the
journal's top is then not the delete, and the item silently reverts the wrong thing.
Nothing in the plugin can see that.
continuewhile tracking stops the running clock, silently, with exit 0. Offeredrather than disabled, on the argument that switching tasks is the common case. A user
who reads the label as "also start this" loses the current interval's boundary — and
timew undodoes bring it back.byStart's alignment assumption: that a token's position intimew get <200 refs>is the dom id. It holds because ids 1..n are all valid and timew prints one value per
reference in order, and the miss path is safe (no match → exit 1, translated toast),
but a timew that ever skipped a reference would act on the wrong id.
the alternative — one replacement glyph versus a
luaL_error— but it is a visibleartefact in a non-Latin locale.
Verdict: APPROVE
Reviewed by the integrator. The three rulings this PR rests on are each backed by live output,
and I re-derived all three independently rather than reading the argument.
Verified
Ids move, so start-based identity is necessary, not stylistic. Reproduced on this machine:
an interval sitting at
@1became@2after a single unrelatedtimew start. A menu carrying@Nacross a refresh would act on a different interval, exactly as the PR says.Undo restores a delete, so the no-confirmation ruling holds. Reproduced end to end:
2 intervals ->
timew delete-> 1 ->timew undo-> 2, tags and ids intact. That makes themis-click recoverable from the same menu, which is the substance behind the argument that a
confirm step is unavailable. The structural claim is also right:
onActivateis enqueued withmakeScriptSnapshot(), which leavespointerContextunset, so a secondopenContextMenufroman activation returns
false- a two-step menu genuinely cannot be built here.Resolve-and-act in one process is the right shape.
byStartnever returns an id to Lua, sothere is no window where the number goes stale between resolving and acting. A row deleted from
a terminal resolves to nothing and says so, rather than resolving to its neighbour. This is the
part I would have expected to get wrong, and it is correct.
Gates:
12 states, 1739 field comparisons / ok; lint0 errors, 0 warnings.Mutations
Six against the new safety properties, each restored byte-identically (patterns verified to have
applied - an unapplied mutation reports as a survivor):
Parse.intervalloses its ISO startcontextaccepted for a destructive actionM7 is the one worth naming: a tag over 512 bytes makes the binding raise rather than return,
taking the handler down with it, and tags come from whatever the user has ever typed at a shell.
Clipping to 120 bytes with a 3000-byte tag in the suite is the right defence, and it is tested.
Advisory
state.undoablemeans "this panel changed something", and is cleared after undoing becausethe journal entry underneath belongs to someone else. That reasoning is sound but lives only
in the PR body - it belongs in a comment beside the field, where the next reader will be.
timew continuestopping the currentinterval first. True on 1.9.1 and verified in the PR, but it is an upstream behaviour the
plugin now depends on silently; worth a line in the README when U8 syncs it.