fix(panel): the things a first real session with the app found - #94
Merged
Merged
Conversation
Seven reports from using the 3.0 for an afternoon. Two of them were not what they looked like. **No switch in the app had ever looked on.** `Knob` renders `aria-checked`, which is what `role="switch"` takes, and the stylesheet keyed off `aria-pressed`. So «Ocultar al hacer clic fuera» read as off while `cp-config` defaulted it to `true` and the config on disk said `true`. The unit test asserted the attribute and the stylesheet asserted the other one, so each passed on its own and nobody compared them. **The tray's left click opened Settings instead of the panel.** It calls `panel::show` now; the menu already had both. **What you paste rises to the top, without moving the retention clock.** `record_paste` wrote `last_used_at` and the count, and the list orders by `modified_at`, so the number went up and the item stayed put. Writing `modified_at` there was the obvious fix and the wrong one: it is also what `expire` compares, what `keep_at_most` and the eviction pick victims by, what the `d:`/`since:` search operator reads, and the keyset cursor `Order::Recent` pages on. A pasted item would have outlived its keep window forever while the settings copy promises that only pinning does that. So the order key is now `MAX(modified_at, COALESCE(last_used_at, 0))` with an index to match, and `record_paste` is untouched. A test holds the clock still and another holds the expression next to the index that was built for it — they have to be spelled the same or sqlite plans a scan. There was a test asserting the opposite behaviour («pasting counts, but it does not reorder the history»). That was a decision written down, not an oversight, and it is reversed on purpose. **A shortcut the system refuses now says what is free.** The refusal was already detected and shown, but `useKeys` asked once on mount, so the warning outlived the fix. `change` returns its promise now and the row asks again when the backend has actually answered — before, the optimistic `setKept` re-ran the effect ahead of `keep`, so the answer was about the shortcut that had not been bound yet. A `spare` command offers the combinations the system will still give, probed with a handler attached rather than a bare `register`, so a probe never leaves a combination grabbed and dead for every other application. The comparison against the current one is between parsed combinations, not strings: the picker emits `Shift+Cmd+V` and a list spelling `Cmd+Shift+V` would have offered the user the shortcut they already had. **The «Copia de seguridad» page was eight amber boxes stacked.** `.said` paints a warning, and what it was painting is mostly a plain account of what comes over. One quiet list now; the two that really are warnings keep their alarm. **The panel can be moved.** It is `no-frame`, so there was nothing to drag it by. The footer strip is a handle, and the buttons on it keep their own clicks. **The footer's sliders button opens Settings**, and the shortcut reference it used to open lives there now, as a table, in both languages. Three guards moved with it and all three fail when broken: every key the table promises is one `panel.slint` looks for, every row says something different in each language, and the `#imagen · #carpeta` example names kinds the search box knows. The first one is stricter than the Rust test it replaces, which skipped a name it did not recognise instead of refusing it. **And the double click does paste.** The report said it did not; `cp-panel.log` had no attempt at all, which ruled out the clipboard, and a probe on the two callbacks answered `moved(0)`, `moved(0)`, `paste(20)`. Nothing was changed for it. Two repairs to `038adfe`, both found by a review of it: lifting group G out of the probe's `main` took the battery's title line with it, so the output started with no idea what was running; and G5's `waited == 0` could not happen, because `waited` moved on the same path as `placed` three lines below and `placed == 0` was checked first. It prints the shortest wait it measured now — 38.7 ms against a 25 ms floor — which is the number that says the guard held. `main` in the probe and `wire` in the panel both shrank, and .github/oversized.txt records the smaller numbers. 865 tests, two runs. The Windows probe: 45 ok, 0 fallan. fmt, clippy with `-D warnings` on the workspace and against aarch64-apple-darwin, tsc, biome and 45 frontend tests.
Six findings a review left against `cc891d6` and `038adfe`, both already in main, plus the three checks this PR had red. **The write-priority half of the protocol was dead code.** Of the three ways to open the clipboard, only `Clipboard::within` has a production caller, and it was the one that never looked at `WRITES_COMING`. So the capture path barged past a write that had already announced itself, and the deference `open()` adds was exercised by nothing but the probe. It defers now. **And the const assert next to it proved nothing.** `CLEARING_MS` was byte-identical to `BACKOFF_MS`, so `clearing >= backoff` reduced to `x >= x` while implying the two budgets were tied. They were not: a writer sleeps before each probe and a reader probes before each sleep, so the reader's last try landed 400 ms after the writer had given up. The writer's table is genuinely longer now and the assert is strict. **`Clipboard::within` took its guard as a token it never used**, and `Self` carried no lifetime, so `let held = Clipboard::within(&reading());` compiled: the guard dropped at the end of the statement while a read-open handle stayed live, which is exactly the hole #92 closed. It borrows the guard now, and that line is refused with «temporary value dropped while borrowed». **`TooSlow` meant two things and the engine could only log one of them.** `capture_counted` answered `TooSlow` when `OpenClipboard` lost the race — a busy resource, not a deadline — and `insisting` promoted it to `Superseded` whenever the sequence had moved, which is the one outcome `engine.rs` drops without a line in the log. There is a `Captured::Busy` now: `Nothing` and `Busy` promote to `Superseded`, a real timeout stays `TooSlow` and is logged as one, and the two read differently in cp-panel.log. **The restart loop was written twice and only ran on one platform.** `cp_mac::capture` never returns `TooSlow`, so on macOS the arm that restarts an abandoned read could not match. `Pending`, `Waited`, `begin` and the loop itself now live once in `cp-core`, which both platform crates already depended on, and `cp-mac-sys/src/reading.rs` is gone. The whole change is 88 lines shorter than what it replaces. **A read that never comes back no longer costs the wait twice.** It still holds its count — that part is correct, because `EmptyClipboard` under an in-flight `IDataObject::GetData` is a use-after-free — but `to_write()` recognises the condition instead of sleeping through its whole budget on the UI thread, and the panel says «an app stopped answering about what it copied» rather than «the clipboard is busy». One correction to the review, which got the mechanism right and the multiplier wrong: guards do not pile up every 60 ms. The watcher is one thread and calls `capture_insisting` synchronously, and on `Answered` or `Gone` the worker has already dropped its guard before anything is relaunched. It takes a second copy arriving while the first read is stuck. **The three red checks.** `fmt`: restoring the probe's title line in the last commit put a real line break inside the string instead of `\n`. `frontend`: the last biome run happened before `bindings.test.ts` existed, so its formatting went unchecked — 18 files then, 19 now. `sonarcloud`: coverage on new code at 78 against a gate of 80, from seven lines — the rail listener and the paths where the backend refuses. They are covered now, and writing that test found a defect of mine: `look()` opens with `setTrouble(null)`, so calling it before setting the reason wiped the reason. The user would have seen a shortcut silently revert. 871 tests, two runs. The Windows probe: 44 ok, 0 fallan. fmt, clippy with `-D warnings` on the workspace and against aarch64-apple-darwin, the ceiling, tsc, biome, and 50 frontend tests with lines at 88.6%. Two things this does not fix, both named rather than hidden. A provider that hangs for good still makes pasting impossible until CopyPaste is restarted; refusing is the correct answer there, and what changed is that it is fast and explained. And `hand_over` still runs on the Slint event loop, so a paste during a live capture waits for it; moving it off needs `Store` out of its `Rc`.
Two more from using the app, and the first was my fault twice over. **The panel could already be dragged; nobody could tell.** The handle went on the 32 px footer strip, under the icon, the count and the hint text. The mechanism was wired and nothing repositions the window on show, so the drag worked — it was simply unfindable. There is a grab bar at the top now: 14 px with a short rounded tab in the middle that lights up under the pointer and turns the cursor to «move», which is what a frameless window is expected to offer. The footer handle stays. **And `set_focus()` alone does not raise a window on Windows.** `SetForegroundWindow` is refused when the foreground belongs to another process, and here it belonged to the panel, which is a separate process — so opening Settings from the panel left it behind whatever was in front. It goes to the top of the z-order for an instant, takes the focus and returns to how it was; a window already pinned on top stays pinned. 871 tests, fmt, clippy with `-D warnings` across the workspace, the ceiling, and the frontend's lint, build and 50 tests.
**No chip in the filter strip could be clicked.** The `TouchArea` that turns the wheel into a horizontal scroll was declared after the `Flickable` that holds them, and a later sibling in Slint sits on top: it covered the whole strip and swallowed every press before a `Pill` saw it. It wraps the `Flickable` now instead of covering it, so the chips get their clicks and the wheel handler gets what they do not take. **And the shortcuts have a section of their own.** The table of what the panel answers to was at the bottom of General, under the settings nobody scrolls past, and the one shortcut you can actually change sat several rows above it. Both are in «Atajos de teclado» now, second in the rail, so the combination you edit and the keys it opens read as one page. The panel's gear lands there rather than on General, which is what it replaced. The three suites that assumed the shortcut lived in General now walk to the section first, and one new test holds the move: the heading, the editable combination and a row of the table, all in the same place. 51 frontend tests, lint and build; fmt, and both touched crates compile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Seven reports from a first real session with the 3.0, plus two repairs to
038adfethat a review of it found.Knobrendersaria-checkedand the stylesheet keyed offaria-pressed. «Ocultar al hacer clic fuera» was alreadytrueincp-configand on disk.MAX(modified_at, COALESCE(last_used_at, 0))rather than by writingmodified_aton every paste.no-frame, so it had nothing to drag it by: the footer strip is a handle.The eighth report — that a double click does not paste — turned out not to be a defect and nothing was changed for it.
cp-panel.loghad no paste attempt at all, which ruled out the clipboard, and a probe on the two callbacks answeredmoved(0),moved(0),paste(20).Why?
Because writing
modified_aton a paste is the obvious fix and the wrong one: that column is also whatexpirecompares, whatkeep_at_mostand the eviction pick victims by, what thed:/since:operator reads, and the keyset cursorOrder::Recentpages on. A pasted item would have outlived its keep window forever while the settings copy promises only pinning does that.A test asserted the opposite behaviour — «pasting counts, but it does not reorder the history». That was a decision written down, not an oversight, and it is reversed deliberately.
The two repairs to
038adfe: lifting group G out of the probe'smaintook the battery's title line with it, and G5'swaited == 0could never happen, so the branch that claimed the guard was never exercised was unreachable.How was it tested?
cargo test --workspace: 865 tests, two runs, no failures.cargo fmt --check;cargo clippy --workspace --all-targetswith-D warnings; the same againstaarch64-apple-darwinfor the macOS crates.scripts/oversized.pyand--inline.mainin the probe andwirein the panel both shrank and.github/oversized.txtrecords the smaller numbers.tsc,biome, 45 tests.#kindthe search box does not know, both fail the suite. The first is stricter than the Rust test it replaces, which skipped an unrecognised name instead of refusing it.Screenshots (if UI change)
The «Copia de seguridad» page and the switches are worth a look side by side with the previous build.