feat(timew): open the panel on left click, drop the row tooltip and today's total #18
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/click-bindings"
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?
Four changes from one report: two behaviour, two UI faults.
Left click opens the panel; right click is unbound
Left click was the start/stop toggle and right click opened the panel. They
swap, and only half of that is a rebinding —
onRightClickis deleted, notpointed at nothing.
That is a product choice, not a workaround: the bar ships no default on right
click, so the button a user is most likely to rebind is theirs to claim, and
claiming it is how you start and stop from the bar:
Starts the
tagsetting, stops what runs. Bindings resolve per gesture keyrather than per table (
widget_action.cpp:66-123), so this leaves themanifest's
middle = "settings-open-widget"in place — verified live with bothset at once.
Correction to the original description of this PR. It claimed the reason
was that "which of a gesture and a widget-level global wins when both claim a
button is unestablished". It is established, and the comments in
bar.luau,plugin.tomland the working-tree README that said otherwise have beenrewritten.
widget.cpp:265-270runsm_innerArea->setAcceptedButtons(m_innerBaseButtons & ~mask)on the plugin'sown input area — the source comment there says this is "what lets a config
binding override a gesture the widget still handles itself" — and
input_dispatcher.cpp:55-61selects exactly one innermost accepting area. Aconfigured binding therefore always wins and the Lua global never fires for
that button; the two are mutually exclusive per button, and the rule is
button-agnostic. A plugin can ship an overridable Lua default for a
rebindable button. This one chooses not to, on right click only.
toggleis now the only route to start/stop from the bar, so the suite drivesthe start argv, the failure toast and the cross-entry stop comparison through
onIpcinstead ofonClick. The absence ofonRightClickis asserted, notassumed.
The row tooltip covered its own context menu
Rows carried
Filter by <tag>. A hover tooltip outlives the press that opensthe context menu, so right-clicking a row drew the tooltip on top of the menu.
The shell exposes no dismissal callback to Lua (
setOnDismissedis internal toplugin_panel.cpp), so a repaint cannot know when to put the prop back —sequencing around it is not constructible. Deleted instead. The row's own text
is the tag, so the tooltip restated it.
tooltipTagleaves the model with it. The untagged row's refusal to filter isstill covered, by the click assert that was always the real check.
"Today: …" printed a timestamp, and is deleted
Not a broken date formatter — never a date at all. On a day with nothing
tracked,
timew summary :dayanswers:Timew.parseTotaltook the last whitespace field of that, so the card readToday: 2026-09-03T00:00:00.. The suite asserted it verbatim as intendedbehaviour ("prose comes back verbatim, exactly as the awk left it").
Fixing the parser was one line, but the label is also redundant: the day header
already shows the same number for the selected scope, and today is the default
scope. So the label,
parseTotal,Timew.todayTotal,Timew.cmd.todayTotal,state.totalandm.todayTotalall go — one fetch fewer per open and permutation.
parseRangeTotalis untouched. It guards with^%d+:%d%d:%d%d$, which is whythe day header never had this bug, and it keeps
lastField.The start/stop button is unchanged at
ACTION_W×ACTION_Wand now sets thecard's height rather than the text column doing it.
What the reviews changed
Two reviews landed on this PR: source (
bar.luau,panel.luau,viewmodel.luau,timew.luau,plugin.toml,en.json) andtest.luauunder46 mutations. Neither found a defect in the shipped behaviour; both found
claims wrapped around it that did not survive checking. Applied:
bar.luau:4-9andplugin.toml:24-37, per the correction above. Behaviour unchanged.(see UPGRADE.md),a file this repository does not contain. The rewritten comment cites
widget.cpp:265-270instead, which any reader can open.widget_action.cpp:66-85→66-123. Lines 66-85 areresolve()'sopening and the
applyDefaultslambda; the half that actually makesmiddlesurvive a user
rightisapplyConfigat 88-123.Fmt.spanandFmt.clockarebyte-identical at or above 3600s, and every
parseRangeTotalinput in thesuite was at or above an hour — so substituting
Fmt.clockforFmt.spaninside
Timew.parseRangeTotalpassed the whole suite, which is exactly theregression
Fmt.spanwas split out to prevent. One sub-hour case(
"0:35:00"→"0:35", whereFmt.clockwould print"35:00") now failsthat mutation at
test.luau:372. Pre-existing onmain; this PR added theline that claimed to check it.
compare()built the expected rowtooltip as
if r.tooltipTag == nil then nil else tr("panel.filter_by", …).Both the field and the key are deleted by this commit, so the
elsearm wasunreachable, and a model-only reinstatement died inside the suite's locale
guard in
tr(test.luau:1694as the file stood then,:1699now) with"the EN copy above has no key panel.filter_by … the
copy has drifted from en.json" — pointing the next reader at the locale copy
rather than at the reinstated field. It is now a flat nil comparison; the
same mutation fails at
test.luau:2196naming the field.Documentation is not in this PR, by the repository's own choice
.git/info/excludeholds*.md.README.md,UPGRADE.mdandREFACTOR.mdexist in the working tree and are deliberately untracked, so no markdown is
in this diff and none can be — on the forge this repository has no README.
The original description's "For a reviewer" note claimed "the paint-count claim
in README moved 25 → 22", which described an edit that only ever existed on
disk. That claim is withdrawn. For the record, the working-tree copies were
corrected alongside this commit (right-click rationale in
README.md, theresolved precedence question in
UPGRADE.md, and the paint count, which is 21—
grep -c "paintsFor(" test.luauis 22 and one of those is the definition attest.luau:2426; the singlepaints ==assert lives insidepaintsFor, soit is not a 22nd path), but none of that ships here.
Verified live
Reloaded on the running shell, driven with real pointer events:
focus, right click againstops it, and middle click still opens the widget's settings
--:--+ tag field, and1:12+Learn C++, squarebutton in each
Gates:
viewModel: 12 states, 1739 field comparisons/ok, and lint0 errors, 0 warnings. No new orphaned translation keys (checked againstmain).For a reviewer
test.luau,timew/bar.luau,timew/panel.luau,timew/plugin.toml,timew/timew.luau,timew/translations/en.json,timew/viewmodel.luau. (The source review saidsix; it is seven.) No
.md, per the section above.panel.all_time,panel.scope_day,panel.scope_day_short,panel.scope_all_short. Leftalone deliberately, out of scope.
panel.idleis documented-unused.#info.children == 2), not an absencecheck. A reinstated third line in the card column has to come back through
that comparison; a label appended as a sibling of the info column is not
covered, which is a pre-existing property of the positional tree comparison.
plugin.toml:39-43still calls its gesture-key list exhaustive (seven keys)when the shell accepts nine, and still says an unknown key "fails only at
dispatch" when only the verb is deferred. Both are pre-existing wording, both
are real, and both are left for a follow-up rather than widened into this
commit.
Review — source half (bar / panel / viewmodel / timew / plugin.toml / en.json / README)
Scope: everything except
test.luau. Shell claims checked against/nix/store/7pgw3p2dhkn8sfpnli7qiyzs9zyhr01d-source. The behaviour in this PR iscorrect and I could not break it. Three of the four claims wrapped around it do not
survive checking, and one of them is the justification for the whole change.
Verified
Gates, run here, exact output
nix shell nixpkgs#luau -c luau test.luau→viewModel: 12 states, 1739 field comparisons/ok, exit 0.noctalia plugins lint timew→yuris/timew/ok/0 errors, 0 warnings, exit 0.Both run in a throwaway clone at
904c065, not in the live checkout.1. The click rebinding is complete
onClickopens the panel —bar.luau:188-190→openPanel()→noctalia.togglePanel("yuris/timew:panel")atbar.luau:184-186.onRightClickglobal survives.grep -rn onRightClick timew/returns fourcomment lines and one hit at
panel.luau:1046, which is the row's node prop forthe context menu — a different mechanism, correctly kept.
grep -n '^function ' timew/bar.luaugives exactly
onConfigChanged,update,onClick,onIpc.plugin_widget.cpp:160-173mapsBTN_RIGHT → the string
"onRightClick"and enqueues it;script_runtime.cpp:702-705is
if (!host->hasGlobal(...)) return std::nullopt;— no else, silent drop. No defaultclaims it either: builtin defaults bind Middle only (
widget_gesture_defaults.cpp:15-18),the
right → control-centerdefault is dead-zone-only (:20-23), andpluginis not inkTypeReserved(:130-139, four built-in types). The plugin's InputArea still acceptsBTN_RIGHT, so the click is consumed and cannot leak to the dead zone.
toggleTrackingis still reachable and still sends the tag as one argv element —bar.luau:174-182,{ "timew", "start", Config.get("tag") }, reached fromonIpc:197-203. IPC shape confirmed:plugin_ipc.cpp:14grammarplugin <author/plugin:entry> <target> <event> [payload], event passed verbatim(
:136-137) toplugin_widget.cpp:531enqueueCallStrings("onIpc", event, payload, …)— two strings, matching
onIpc(event, _payload).grep -rn -i 'left click|click toggle|toggles tracking|starts/stops tracking' timew/returns only the three updated prose lines.
2. Per-gesture-key resolution, and the
middlesurvival claimTrue.
widget_action.h:48-52documents four last-wins layers;m_actionsisstd::array<std::optional<WidgetAction>, kGestureCount>(widget_action.h:88) and everylayer writes individual slots —
applyDefaultsatwidget_action.cpp:71-85,applyConfigat
:88-123. A userrightoverwrites onlyright; the manifest'smiddlesurvives (andwould anyway, via the layer-1 builtin at
widget_gesture_defaults.cpp:15-18). Nowhole-table override path exists.
The
[widget.timew.actions]example form is right for a plugin widget:widget.cpp:222builds the context as
widget.{m_configName}, andm_configNameis the user's bar-configkey (
bar.cpp:2477), declared alongsidetype— same shape as this machine's own[widget.cat] / type = "noctalia/bongocat:cat".3. The deletions left nothing dangling
git greponfeat/click-bindingsforparseTotal,todayTotal,state.total,m.todayTotal,tooltipTag,panel.today,panel.filter_byfinds no reader intimew/. (panel.today_shortis a different, live key.)lastFieldkeeps a live callerat
timew.luau:173;parseRangeTotalis byte-identical tomainand still guards with^%d+:%d%d:%d%d$(timew.luau:172-175).Translation keys:
en.jsonis valid JSON with no duplicate keys on either side (41 → 39raw = 41 → 39 unique). The key-set diff is exactly
panel.todayandpanel.filter_byremoved, nothing added — the rest of that hunk is reordering noise. Every
panel.*/settings.*literal intimew/is declared; everysettings.*_keyinplugin.tomlresolves.
Orphan claim confirmed, and confirmed as not a regression. Declared-but-unreferenced,
computed identically on both refs:
panel.all_time,panel.scope_day,panel.scope_day_short,panel.scope_all_short, pluspanel.idle— five onmain, thesame five on the PR. The four named in the description were already dead, and
panel.idleis documented-unused at
panel.luau:735-736. This PR adds no orphan.4. The root-cause story — reproduced, not taken on faith
Real
timew1.9.1 against an emptyTIMEWARRIORDB:main's parser run on exactly those bytes:Exactly as described, trailing period included, and
parseRangeTotal's guard is why itwas immune. The characterisation is accurate and not overstated.
5. The layout claim
Holds, and the width arithmetic is exact.
ACTION_W = 76,LEFT_W = 220,SUBJECT_H = 24(panel.luau:98-100);subjectW = LEFT_W - ACTION_W - 24 = 120, and the- 24is precisely the row'spadding = 8on both sides plusgap = 8, so8 + 120 + 8 + 76 + 8 = 220 = LEFT_W. The button is untouched at76 x 76(
panel.luau:774-779).Height: the left column was
clock(28pt) + gap4 + subject(24) + gap4 + today(12pt), nowclock(28pt) + gap4 + subject(24)≈ 65px, which is underACTION_W76 — so the buttondoes now set the card height, at
8 + 76 + 8 = 92. The card gets ~9px shorter. It doesnot look wrong: the row is
align = "center", so the shorter text column centres againstthe square button. Nothing else in the left column depended on the third line —
subjectWnever did, and the two survivors keep their own props verbatim.
Not checked
description's live-verification bullets are consistent with it, but I did not drive the
compositor.
constants plus the shell's box model, not a screenshot.
en.test.luau—ReviewSuiteowns it.Findings
Blocking
1. The stated reason for deleting
onRightClickis false — the shell does establish precedence.timew/bar.luau:8-9andtimew/plugin.toml:24-26Both added comments say which of a gesture and a widget-level global wins "is not
established". It is established, deterministically, with a source comment saying so.
widget.cpp:265-270:Any gesture-bound button is stripped from the plugin's own InputArea, and
input_dispatcher.cpp:55-61selects exactly one innermost accepting area — so the two aremutually exclusive by construction. A config binding wins and the Lua global never
fires; they never both run. Verified for left as well as right (
widget.cpp:269isbutton-agnostic).
Why it matters: this is the load-bearing justification for the change, repeated in three
places, and it is backwards. Keeping
onRightClickwould not have created a contest —a user's
right = …would have overridden it cleanly, which is exactly the outcome thecomment says cannot be relied on. The behaviour shipped here is a legitimate product
choice; the reason given for it is not, and the next person to touch this will believe a
plugin cannot ship an overridable Lua default for a rebindable button. It can.
Fix: keep the behaviour if that is the intent, and restate the rationale — right-click is
left unbound because the plugin does not want a default there, not because precedence is
unknown. Same edit in
README.md:9-11.2. The README half of this PR is not in this PR.
README.md(whole file).git/info/exclude:7is*.md, soREADME.md,UPGRADE.mdandREFACTOR.mdare alluntracked.
git ls-tree -r --name-only feat/click-bindingslists no.mdfile at all, andgit diff main...feat/click-bindings --name-onlyis six files, none of them the README.The description's "For a reviewer" note — "The paint-count claim in README moved 25 → 22" —
describes an edit that exists only on your disk.
Why it matters: the entire user-facing instruction for getting start/stop back (the
[widget.timew.actions] right = …block atREADME.md:14-17) is the mitigation for abehaviour this PR removes, and it ships nowhere. On the forge this repo has no README.
Either track the
.mdfiles or drop the claim from the description.3. Added comment points at a file that is not in the repository.
timew/bar.luau:9The line added by this commit ends
(see UPGRADE.md). Per finding 2,UPGRADE.mdisexcluded and absent from the tree —
git grep -n "UPGRADE.md" feat/click-bindings -- timew/is this one hit, and no
.mdfile exists on either ref. A reader who clones this repo hasno way to follow it. (If finding 1 is taken, this sentence probably goes away with it.)
Advisory
4. README's paint count is off by one — 21, not 22.
README.md:194grep -c "paintsFor(" test.luau= 22, buttest.luau:2421is the definitionlocal function paintsFor(what, expected, fn); the other 21 are call sites. The singlepaints ==assert attest.luau:2424sits insidepaintsFor, so it runs once per callsite rather than being a 22nd path — the recipe in the description counts it twice. The
old 25 was stale; 22 is stale by one in the other direction. Not landed anyway, per
finding 2.
5. "both fail only at dispatch" is right for the verb, wrong for the key.
timew/plugin.toml:41-42— pre-existing wording, but this commit re-flowed the lineThe lint half is correct:
plugin_lint.h:11-16has only four settings-only finding kindsand
plugin_lint.cpp:344-492never touches gestures. But only the verb is deferred todispatch (
widget_action.h:36-40, deliberately). An unknown key is rejected atbar-build time —
widget_action.cpp:93-96for user config,widget_gesture_defaults.cpp:156-160for the manifest — andnoctalia config validaterejects it too (
config_validate.cpp:325-341). Cheap to correct while the line is alreadybeing touched.
6. The accepted-gesture list is missing two keys.
timew/plugin.toml:38-39— pre-existing, unchanged context lines"Gesture keys the shell accepts: left, right, middle, forward, back, scroll_up,
scroll_down" reads as exhaustive. The shell accepts nine:
widget_gesture.cpp:26-36kGesturesalso hasscroll_leftandscroll_right, anddocs/user/bar/actions.mdx:33-40lists all nine. No listed key is invalid. Flagged only because the sentence claims
completeness.
7. Citation points at the mechanism, not at the consequence.
timew/plugin.toml:35-36"Bindings resolve per gesture key rather than per table (widget_action.cpp:66-85), so
setting
rightthere leaves thismiddlein place." The behaviour is true (verifiedabove). Lines 66-85 are
resolve()'s opening and theapplyDefaultslambda — layers 1-2only — which do show per-key slot writes but not the user-vs-manifest merge. The half that
actually makes
middlesurvive a userrightisapplyConfigatwidget_action.cpp:88-123. Suggest citing66-123.Not mine
test.luaustill carries the deleted field. Onfeat/click-bindings, lines 1961-1962build
tr("panel.filter_by", { tag = r.tooltipTag })and lines 2190/2192 assertrows[N].tooltipTag == nil— butViewmodel.buildno longer assignstooltipTag, sothose asserts compare nil to nil and cannot fail whatever the viewmodel does, and
panel.filter_byis no longer a key this plugin ships. Raised withReviewSuite, who ownsthat file; flagging here only so it is not lost between the two halves.
Review:
test.luau(233 changed lines)Scope:
test.luauonly. Plugin source is ReviewSource's half.Method: every claim below was mutation-tested. The main checkout was never touched — all
work ran in
cp -r-copy/tmp/suite-copy. 46 mutations, one full gate run each.Gates
Verified
Each new or rewired assert, against the smallest source mutation that kills it.
1.
onClick->onIpc("toggle"): nothing lost in the swap:864argv pin, bar idleonIpctoggle branch made a no-op ->471: not an argv array but a nil:871START_TAGShostile-tag loop"focus"->871: argv[3] is "focus", expected "deep work":871(same loop)toggleTrackingreadsConfig.get("icon_size")->864: argv[3] is "17", expected "focus":864/:1013config readConfig.getalways returns the default ->737:913toast wiring loopnotifyError->913; callback builds its own body fromresult.stderr->913; callback toasts on success too ->bar.luau:178via3351:3352sync re-entrysync()->bar.luau:178via3352:1043+:3357cross-entry stop--dry->1400: no queued command matching timew stop:1017literal backstopbarandpanel->timew stop --dry) ->472: 3 elements, expected 2That last row is the one worth calling out: the block comment claims nothing below compares
against a literal, so a coordinated drift needs a literal somewhere.
:1017is it, and it holds.2. The click-route block: every assert is load-bearing
:992rawget(barEnv, "onRightClick") == nilfunction onRightClick() openPanel() endput back in bar.luau ->992:999left click opens, idleonClickcallstoggleTracking->999;onClickemptied ->999;openPanelopensyuris/timew:other->999:1000and runs nothingonClickopens and toggles ->1000:1004/:1005same, tracking:1013toggle starts the configured tagonIpctoggle no-op ->471:1017toggle stops what runs--dry->1017; state branch inverted ->472:1021unknown verb runs nothingif event == "toggle"->if true->1021:1028onIpc("panel")"open"->1028:1031unknown verb opens nothingelseif event == "panel"->else->1031On the vacuity question for
:992: it is a real check, not an accident of the wrong table orthe wrong spelling.
barEnvisgetfenv(bar.elapsed)(:643), which is the bar entry's ownglobals table — the exact table a reinstated file-scope
function onRightClick()writes to.Proved by putting one back: the suite goes red at
:992, not somewhere downstream.rawgetrather than plain indexing is the right spelling here, since
barEnvcarries an__indexchain to the CLI globals.
The two
lastCmd == nilasserts are bracketed by positive controls —:864before and:1013after both provelastCmdis still being recorded — so they cannot pass by theharness having gone deaf.
3.
parseTotalblock replacementlastField's two documented cases survived the rewrite verbatim and both are live::362blank tail ("a b\nc d \n\n"->"d")lastFieldtakes the first field of the line ->361; drops the%s*$trailing-space tolerance ->362:363whitespace-only line ->""lastseeded with"0:00:00"->363:361summary shapelastFieldkeeps the first field it finds ->361:366prose floors to0:00parseRangeTotalguard removed ->366:367a real clock convertsparseRangeTotalalways returns"0:00"->367Neither new
parseRangeTotalassert compares two literals.:366compares a computed valueto a literal;
:367is discussed under Findings.4. Row tooltip sweep
:1522tips == "nil,nil"1522:1522(coordinated)tooltipTagback in viewmodel and thetr("panel.filter_by")prop back inbuildList->1522:1525controltooltipprop ->1525The control does its job, but it is not the only thing standing between the sweep and a dead
harness — and it is not even the strongest.
rowProp("tooltip")readsrow.children[3].props.tooltip(:1332-1343), the same node and the same props table thatrowTexts()reads eight lines earlier at:1513, whose contents are pinned at:1514. So aharness that stopped reading row props fails at
:1514first, with a better message. TheactionNode()control covers a different node and is worth keeping, but the real defence isupstream.
On
"nil,nil"pinning the row count as a side effect: it does, and it is harmless. Shrinkingthe list to one row fails at
:1484— well before:1522— so the confusing failure thecount-pinning could cause is unreachable. The row set is pinned by contents at
:1514before it is pinned by arity at
:1522.5. Card shape
:1837#info.children == 2ui.labelback in the card column ->1837: 3 vs 2:1829clock identity1829: nil vs "--:--"; clock label stops readingm.clockText->1442:1831action wiringonClick->1831A count is the right invariant for the reinstatement it is aimed at: the faithful one — the
deleted
ui.labelput back where it was — is caught, and a reorder is caught separately by:1829readingchildren[1]as the clock. Boundary noted under Findings.6. Census edits
The
sedthat droppedtotal = "..."from everyopen()spec did not corrupt anything.Checked mechanically, not by eye: every changed
open({...})line on the branch isbyte-identical to its
maincounterpart withtotal = "<value>",removed —No
spec.total,state.total,todayTotal,parseTotalorpanel.todayreference is leftanywhere in the file except one comment at
:351.The counts are right, not merely green:
:3193closed state field settotal = "0:00:00"put back in panel state ->3193:2938#panelJobs == 3run()refreshes twice ->2938: 6 commands, not 3:2941-2944+:2945leftover checkrun()skips the refresh ->1400commitpaints twice ->2424;refreshdropsfetchDays->2053refresh()isfetchActive + fetchTags + fetchDaysin day scope, so 3 is arithmeticallyright, and the block is closed on both ends —
== 3at the top,== 0leftover at:2945— so a fetch added or dropped fails one or the other.
Not checked
bar.luau,panel.luau,viewmodel.luau,timew.luau,plugin.toml,en.json,README.md) — ReviewSource's ownership. Source files weremutated here only as instruments for judging asserts, never reviewed as code.
Not reproducible from the suite; taken on the author's report.
ENat:1671againsttimew/translations/en.json— the suite says itself that the luauCLI has no
io, so this is a by-eye copy with no automated drift check. Out of scope fora mutation pass.
Mutations
46 total, 43 killed, 3 survived.
function onRightClick()in bar.luau:992onClickcallstoggleTracking:999onClickbody deleted:999onClickopens and toggles:1000onIpctoggle branch no-op:471onIpctoggles on any verb:1021onIpcopens the panel on any other verb:1031"focus":871--dry:1017openPanelopensyuris/timew:other:999Config.getalways returns the default:737Config.get("icon_size"):864:472onIpcpanel verb renamed to"open":1028:913result.stderrdirectly:913bar.luau:178@:3351sync()bar.luau:178@:3352:472(@:1017)lastFieldkeeps the first field:361lastFieldtakes the first field of a line:361lastFieldseedslastwith"0:00:00":363lastFielddrops%s*$tolerance:362parseRangeTotalguard removed:366parseRangeTotalalways"0:00":367parseRangeTotalusesFmt.clockforFmt.spanparseRangeTotalguard loosened to^%d+:%d%d:1522tooltipTagback together (coordinated):1522:1525tooltipTagback, tagged rows only:1694(see F2)tooltipTagback on every row:1694tooltipTagback as"":1694tooltipTagback, with:1522/:1961neutered:2191(reachability probe)ui.labelback in the card column:1837:1829m.clockText:1442onClick:1831ui.labelas a sibling in the card rowstate.totalreinstated:3193refreshdropsfetchDays:2053commitpaints twice:2424run()skips the refresh:1400run()refreshes twice:2938--dry:1400:1484(ordering probe)All three survivors are analysed below. None of them is a regression this PR introduced —
26 and 27 were checked against
mainand survive there too — but 26 is the one the PRwrites a new assert about.
Findings
Blocking
None. No assert this patch adds or rewires is vacuous, and no coverage was lost in the
onClick->onIpc("toggle")swap.Advisory
F1 —
test.luau:367: the newparseRangeTotalassert cannot see the conversion it names.Fmt.spanandFmt.clockare byte-identical for every input at or above 3600s: both returnstring.format("%d:%02d", s // 3600, (s % 3600) // 60)on that branch. 3723 is above it, soFmt.span(3723)andFmt.clock(3723)are both"1:02". EveryparseRangeTotalinput in thefile is at or above an hour —
"1:02:03"here,"5:20:00"at:431, and the panel fixtures"2:00:00","3:22:00","6:00:00","7:00:00","9:11:00". Result: substitutingFmt.clockforFmt.spaninsideTimew.parseRangeTotalsurvives the whole suite(mutation 26). That substitution is exactly the regression
Fmt.spanwas split out toprevent —
fmt.luau:22-24says so in as many words:"35:00"for a 35-minute span was beingread as 35 hours.
On the specific question of
Fmt.span(3723)versus the literal"1:02": the literal alonewould not fix it —
Fmt.clock(3723)is"1:02"too. What computing the expected sidethrough the same function costs is legibility: the line reads as a check on the conversion
while it actually pins only
Parse.clockand the guard branch, both of which:366and:431already hold. The input is the fixable half. Verified: adding one sub-hour case killsthe mutation.
Pre-existing on
main, so not a regression — butmaindid not have a line claiming tocheck it.
F2 —
test.luau:1961: a live assert routes its failure through a key this commit deleted.compare()still builds the expected row tooltip asif r.tooltipTag == nil then nil else tr("panel.filter_by", { tag = r.tooltipTag }).Viewmodel.buildno longer setstooltipTag, and the same commit removedpanel.filter_byfrom
timew/translations/en.jsonand from theENmirror at:1671. Theelsearm isunreachable while the suite is green, and the moment a regression makes it reachable it dies
inside
trinstead of at the comparison. PuttingtooltipTagback inviewmodel.luaufailswith:
The guard still goes red, so this is diagnosis quality and not coverage: it points the next
reader at the locale copy rather than at the reinstated field, and the obvious first move —
add
panel.filter_byback toEN— is wrong. Now that the field is gone, the conditionalhas nothing left to express.
F3 —
test.luau:2190-2194: live, but dominated; no change needed.Recording this because it reads as vacuous and is not. The pair asserts
tooltipTag == nilon a field nothing assigns, which looks like
nil == nil. It is reachable: with:1522andthe
sameat:1959-1963neutered, reinstatingtooltipTagdoes fail at:2191(mutation 34). In the shipped file it can never be the first failure — a model-only
reinstatement dies at
:1694/:1960during state 1, a model-plus-builder one at:1522—so the pair is documentation with a real assert behind it. Keeping it costs nothing.
F4 —
test.luau:1837:#info.children == 2is a container pin, not a card-wide absence check.The PR body already scopes it this way; recording the measured boundary. The faithful
reinstatement — a third
ui.labelin the card column — dies at:1837with3 vs 2. Aui.labelappended as a sibling of the info column, inside the card row, survives the entiresuite (mutation 39).
compare()reads the card positionally(
left.children[2].children[1]and[2]), so any non-clickable node appended past the lastindexed child of any container is unasserted — a pre-existing property of the tree
comparison, not something this patch changed. The count is the right invariant for the shape
the deleted line actually had.
Not mine
timew/viewmodel.luauandtimew/translations/en.jsonare ReviewSource's; F2 is filedagainst
test.luaubecause the stale reference lives there, not in the source.Correction to my review above — the "Not mine" paragraph was wrong
ReviewSuitedisproved it with mutation testing, which beats the static reasoning I used.I claimed the surviving
tooltipTagasserts attest.luau:2190/2192"compare nil to niland cannot fail whatever the viewmodel does". That is not true. Reinstating
tooltipTagin
Viewmodel.buildthree different ways (tagged-rows-only, every-row, empty-string) turnsthe suite red every time, and with the two earlier guards at
test.luau:1522and thesame()at1959-1963both neutered, the same mutation still fails at2191. Thoseasserts are live guards against reinstatement — dominated by earlier checks, not vacuous.
I should have run the mutation before asserting otherwise.
What does survive from that paragraph is narrower and is
ReviewSuite's to report: thesuite's inline EN copy still calls
tr("panel.filter_by")attest.luau:1961, a key thisPR removed from
en.json, so a model-only reinstatement dies at1694with "the EN copyabove has no key panel.filter_by -- the copy has drifted from en.json" rather than at the
tooltip comparison. The guard fires; the message points at the wrong thing.
Everything in the source half of the review above stands unchanged — the three blocking
findings are
bar.luau:8-9/plugin.toml:25-26(precedence is established atwidget.cpp:265-270, so the stated rationale is false), the README not being in the commitat all, and the added
UPGRADE.mdreference pointing at a file the repository does notcontain.
904c065982to1f92814a1eReviews actioned —
904c065amended to1f92814Both reviews read in full. No behaviour changed: every finding below was about a
claim, a test input, or a diagnosis message. Amended rather than stacked, force-pushed
with
--force-with-lease.Gates, exact output, run in the live checkout after the last edit
Accepted
Source B1 — the precedence rationale was false. Applied, and I confirmed the
mechanism independently before rewriting anything.
widget.cpp:265-270runsm_innerArea->setAcceptedButtons(m_innerBaseButtons & ~mask)with the source comment"This is what lets a config binding override a gesture the widget still handles itself,
for scroll as much as for clicks", and
input_dispatcher.cpp:55-61selects exactly oneinnermost accepting
InputArea. A configured binding always wins, the Lua global neverfires for that button, and the two cannot both run.
bar.luau:4-9andplugin.toml:24-37now say that, and give the real reason right click is unbound: thebar wants no default there and it is opt-in by config. Same correction in the
working-tree
README.md:9-13.The reviewer's wider point is the one I kept: a plugin can ship an overridable Lua
default for a rebindable button. That is now written down in both files, so the next
person does not re-derive the wrong conclusion.
Source B3 — the
(see UPGRADE.md)pointer. Dropped. The rewritten comment citeswidget.cpp:265-270, which anyone who clones can open, instead of a file the repositorydoes not contain.
Source A7 —
widget_action.cpp:66-85→66-123. Applied; checked first. 66-85 isresolve()'s opening plus theapplyDefaultslambda. The half that actually makesmiddlesurvive a userrightisapplyConfig, which spans 88-123 and writesindividual slots at
:119.Suite F1 — the surviving
Fmt.spanmutation. Applied, with proof.Mutation, in a
cp -rcopy at/tmp/pr18-mut(never the live checkout —timew/is hot-reloaded by the running shell):Before (that mutation against
904c065'stest.luau):Green. The whole suite passes with
parseRangeTotalprinting 35 minutes as 35 hours.Added at
test.luau:372:After (same mutation, new suite):
I used the literal
"0:35"rather thanFmt.span(2100)deliberately. Both kill themutation, but the literal also pins the byte string, so it cannot be neutered by a later
mutation of
Fmt.spanitself — and the review's own argument was that computing theexpected side through the function under test is what made
:367unable to see theconversion it names.
Suite F2 — the dead tooltip conditional. Applied.
compare()now reads:Verified the guard still goes red, and for the right reason. Mutation:
tooltipTagreinstated in
Viewmodel.build(tagged rows only, matchingmain).Before, with
904c065's conditional in place:Dies inside the suite's own
trstub, telling the reader the locale copy has drifted —so the obvious first move is to add
panel.filter_byback toEN, which is wrong.After:
Names the reinstated field. That is F3's pair earning its keep, which is a nice
confirmation of the review's own reachability probe.
Two more probes on the collapsed line, since "collapsed to a constant" invites the
question of whether it can still fail:
tr("panel.filter_by")prop back inbuildList) dies upstream attest.luau:1527:no row offers a tooltip to cover its context menu: nil,panel.filter_by. Correctguard, correct message.
:1527neutered and a literal tooltip put on the row tag button, the collapsedline is the failure:
./test.luau:1968: tracking a multi-word tag: rows[1] carries no tooltip -- "Filter by focus" vs nil.So it is live, not decorative — same standing as before, better message.
Source A4 — the paint count. Corrected to 21 in the working-tree
README.md:195,and the sentence now says what it counts: 21
paintsFor(call sites, each wrapping oneaction.
grep -c "paintsFor(" test.luauis 22;test.luau:2426is the definition, andthe single
paints ==assert at:2429sits insidepaintsFor, so it runs once percall site rather than being a 22nd path.
Source B2 — the README half of this PR was not in this PR. Accepted as stated, and
the description is fixed rather than the exclusion.
.git/info/excludeholding*.mdisthe repository owner's deliberate decision, so the correct resolution of "either track
the
.mdfiles or drop the claim" is to drop the claim. The PR body now says plainlythat documentation lives in untracked working-tree files by choice and ships nowhere in
this diff, and the withdrawn "paint-count moved 25 → 22" claim is called out as
withdrawn. Working-tree copies were corrected alongside the commit; none of it is in the
diff, and the body says so.
Also recorded in the working-tree
UPGRADE.md: the gesture-vs-global entry that had beenopen since #16 is now a closed item citing
widget.cpp:265-270andinput_dispatcher.cpp:55-61. The other closed items are untouched.Pushed back on
One factual correction to the source review. It says
git diff main...feat/click-bindings --name-onlyis "six files, none of them the README". It is seven:
test.luau,timew/bar.luau,timew/panel.luau,timew/plugin.toml,timew/timew.luau,timew/translations/en.json,timew/viewmodel.luau. The conclusion is untouched — noneof them is a
.md— but since I was rewriting the body to make every claim in it true ofthe actual diff, it had to say seven.
Source A5 and A6 — real, deliberately not in this commit. Both are correct and I
verified A5 against source (
widget_action.cpp:93-96andwidget_gesture_defaults.cpp:156-160reject an unknown key at bar-build time; only theverb is deferred to dispatch per
widget_action.h:36-40). Both are pre-existing wordingin
plugin.toml:39-43that this commit only re-flowed, and neither touches theprecedence block I was asked to rewrite. Widening a behaviour PR to fix adjacent prose
that was already wrong on
mainmakes the diff harder to review for no gain — they areflagged in the body's "For a reviewer" section for a follow-up.
Suite F3 — no change, agreed. The
tooltipTag == nilpair attest.luau:2195-2199reads vacuous and is not; my F2 mutation above makes it the first failure now, which is
a better outcome than when it was dominated. Keeping it.
Suite F4 — no change, agreed.
#info.children == 2is the right invariant for theshape the deleted line had. The sibling-append blind spot is a property of positional
tree comparison, not of this patch; it is recorded in the body.
Not done
Nothing. Every blocking and advisory finding in both reviews is either applied above or
declined above with a reason.
What a reviewer should still challenge
widget.cpp:265-270mechanism is read from source, not driven with a compositor.The original description's live-verification bullets (right click bound by config wins,
middle still opens settings) are the empirical half and they are consistent with it,
but nobody has watched a Lua global lose to a binding on the same button — because
after this commit there is no Lua global on a rebindable button to lose. If you want
that observed rather than derived, it needs a throwaway widget declaring both.
"0:35:00"input is synthetic.timewemitsH:MM:SStotals, so a sub-hour dayreally does come back as
0:35:00, but I did not runtimewto confirm that exactbyte string for a sub-hour range — the assert pins the parser's contract, not timew's
output shape.
.mdcorrections are unverifiable from the forge, by construction.Anyone reviewing them has to be on this machine.
Not merging — that is the integrator's call.