Repository navigation
gui: Navigate card read-back groups - #109
BenWestgate wants to merge 1 commit into
Conversation
|
nit: It should also lock from editing the groups that are already correct. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1027e599cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| start = next((i for i in range(same) if old[i] != new[i]), same) | ||
| tail = next((i for i in range(same - start) if old[-1 - i] != new[-1 - i]), same - start) |
There was a problem hiding this comment.
When an editable group contains adjacent identical symbols, this first-difference calculation cannot determine which copy the operator deleted and ignores the GTK cursor position already captured by _reformat. For example, deleting the first B from ABBC produces ABC? with the cursor after the remaining B, so typing X yields ABXC rather than the intended AXBC. This breaks the promised in-place correction for valid cards containing repeated characters; use the actual edit/cursor location when padding the deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d17822a550
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._dropped, self._locked = "", frozenset() | ||
| self.set_text(PREFIX) |
There was a problem hiding this comment.
When a read-back page is left or confirmation succeeds, clear() replaces only the GTK text and unlocks the entry, but _shown still references the complete recovery card last assigned by _reformat. set_text(PREFIX) merely schedules another idle reformat, so the card remains reachable after the page cleanup—and indefinitely if that idle callback does not run—contrary to the recovery-text clearing guarantee in docs/security/model.md. Reset _shown to PREFIX directly in clear() as part of the synchronous cleanup.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4f96aea5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while start and len(new) - tail > cursor and new[-1 - tail] == old[-1 - tail]: | ||
| start, tail = start - 1, tail + 1 |
There was a problem hiding this comment.
When a selection is replaced with text sharing a suffix with the old group, this adjustment only handles inferred edit boundaries to the right of GTK's cursor. For example, replacing a selected editable QQQQ group with WQQQ leaves the text correct but infers only W as typed and moves the cursor after W instead of after the replacement; subsequent typing then edits the wrong position. Fresh evidence beyond the earlier deletion case is that the current fix still fails for multi-character replacements whose trailing symbols match the old text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think once the user has submitted their text, correct groups should be locked and only individual boxes with mistakes remain editable.
characters should no longer shift between boxes at that point, and tab can navigate between the remaining wrong boxes. We preserve the left most groups as correct when an extra or missing character falls between two groups. We also prefer the interpretation that leaves fewer red groups and more correct and locked groups.
There was a problem hiding this comment.
Agreed. Current head still uses one shifting Gtk.Entry plus cursor/suffix inference, so it does not implement this requested post-submit model. Rework the mismatch state into fixed editable boxes/groups with matched boxes locked and Tab navigation; that removes the repeated-symbol cursor ambiguity instead of extending the heuristic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 728db2b835
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if first // GROUP in self._locked or len(old) - tail > end: | ||
| return old, start |
There was a problem hiding this comment.
When two or more consecutive groups are highlighted, selecting those groups and pasting their corrected text changes only editable groups, but this condition compares the edit against the end of the first group and restores the entire old card whenever the edit continues into the next group. For example, an eight-character replacement spanning mismatched groups 2 and 3 is rejected even though neither group is locked, forcing the operator to edit each group separately despite the UI saying all highlighted groups can be changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We should prevent pasting as a security measure. It makes it easier to not backup the cards.
There was a problem hiding this comment.
Agreed. Current a458f9e still accepts pasted replacement text—indeed the latest commit explicitly lets one edit span adjacent open groups—so this requested behavior is not implemented. Disable paste in the post-submit correction state and keep edits confined to the remaining open boxes.
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head re-review: NACK a458f9e. The synchronous secret-cache cleanup is fixed, but the remaining single-entry cursor heuristics still conflict with the maintainer's requested mismatch UX: matched boxes should become fixed/locked, only wrong boxes editable with Tab navigation, and paste disabled in that state. The existing repeated-symbol/cursor findings should be resolved by that rework rather than more inference. No additional finding.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at a458f9e: existing P2s remain in the post-submit correction UI. Repeated-symbol edits still depend on cursor/suffix inference, paste remains accepted across open groups, and the requested fixed/locked mismatch boxes with Tab navigation are not implemented.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04aeb87633
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.add_css_class("card-entry") | ||
| self.set_input_hints(Gtk.InputHints.NO_SPELLCHECK | Gtk.InputHints.NO_EMOJI) | ||
| refuse_paste(self) | ||
| self.connect("changed", lambda _entry: GLib.idle_add(self._capitals)) |
There was a problem hiding this comment.
When the operator selects text in a highlighted mismatch box—for example, before replacing the group—GroupEntry lacks the notify::selection-bound and notify::cursor-position hooks used by Codex32Entry to clear GTK's primary selection. The selected recovery fragment therefore remains published long enough for a clipboard manager to persist it, rather than being reclaimed on the next main-loop turn as promised by the GUI security model; reuse the same unpublishing behavior for these new entries.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
| if not result.accepted: | ||
| groups = result.mismatched_groups | ||
| where = f"Group {min(groups)}" if groups else "What you typed" | ||
| _say(status, f"{where} does not match. Check it against your card.", "error") | ||
| correct(reading.aligned(card.text.upper(), entered)) | ||
| update() |
There was a problem hiding this comment.
On every failed retry, this globally realigns the concatenated box contents and rebuilds all boxes, so an edit can move into—and unlock—a previously matched group. For example, after groups 1 and 3 are open while group 2 (NAME) is locked, correcting group 1 to MS12N causes the next retry to redraw group 1 as locked MS12 and group 2 as editable NNAME; the extra character moved across the supposedly fixed boundary. Fresh evidence beyond the earlier single-entry cursor reports is that the new fixed-box implementation discards its boundaries here; retries should realign only existing editable boxes while retaining locked labels.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
Correction boxes did not take their selection back from the primary clipboard (Codex review on #109). The field that did watched only the selection bounds, but GTK also republishes the selection on focus and when the same range is selected again, so a copy could stay published. Both entries now share PrivateEntry, which watches the primary clipboard while the entry is realized and takes back any local copy made while it holds a selection. GTK only publishes from realized text, so nothing is missed, and the handler is dropped on unrealize. Security: narrows the window in which a clipboard manager can persist selected card text, for the read-back field, its boxes and the restore fields. Validation: new primary-clipboard test passed 20 of 20 runs (the old selection watch failed it about a third of the time); full suite normal and -O (998 passed); ruff, mypy. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
A failed retry realigned the joined text of every box and rebuilt them, so an extra character typed into an open box could move across into a locked group and unlock it (Codex review on #109). A retry now keeps every box where it is and only rechecks the open ones against their own groups; the cross-group alignment runs once, on the first submission from the single field. Its regression test, test_a_retry_keeps_every_box_where_it_is, landed in the previous commit by mistake and passes from this one. Validation: the retry test fails before and passes after; full suite normal and -O (998 passed). Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6505f47b02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| retried = tuple( | ||
| (box.get_text().upper(), box.get_text().upper() == want) for box, want in zip(boxes, groups) | ||
| ) |
There was a problem hiding this comment.
When multiple boxes remain open and the operator fixes one using spaces (which the page explicitly says do not matter), confirm() evaluates the whitespace-stripped value, but this raw comparison still marks that box as wrong. For example, entering AB CD for an expected ABCD group while another group remains incorrect causes the repaired group to stay highlighted and editable on the retry. Compare each box using the same whitespace/case normalization used for confirmation while preserving the fixed group boundaries.
Useful? React with 👍 / 👎.
| ) | ||
| CORE_ADAPTER = "codex32._bitcoin_core" | ||
| BUDGET = 2000 | ||
| BUDGET = 2050 |
There was a problem hiding this comment.
Raising the enforced budget to 2,050 leaves the repository's review contracts stale: docs/developer/gui.md still states that this test enforces 2,000 lines, while docs/developer/api.md still records a separate 1,800-line GUI budget. Future reviewers therefore cannot tell which limit was authorized, and changes between 2,000 and 2,049 lines silently pass despite the current GUI documentation. Update the matching durable documentation together with this enforcement change.
Useful? React with 👍 / 👎.
The page says spaces and capitals do not matter, and confirm() strips them, but a retry compared each box's raw text with its group, so a group typed as "AB CD" stayed highlighted while another was still wrong (Codex review on #109). Boxes are now compared as confirm() reads them. Validation: new retry test fails before, passes after; GUI tests, ruff, mypy. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
gui.md still said 2,000 and api.md still said 1,800 after the enforced budget rose to 2,050 for the card read-back rework (Codex review on #109). Both now state the enforced figure, and gui.md notes why it rose. Claude-Session: https://claude.ai/code/session_018HrKoywz6PT9c91jvKou7Q
This comment has been minimized.
This comment has been minimized.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI current-head review: ACK c57dd55. The fixed-box mismatch model removes the earlier cursor/repeated-symbol ambiguity, retry comparison stays within the existing group boundaries, whitespace is normalized before a repaired group locks, and PrivateEntry reclaims primary-selection contents for both the original entry and correction boxes. Paste/drop paths are disabled and the GUI budget documentation matches 2,050. The older unresolved threads are against superseded implementations; I found no current-head correctness issue.
|
Could you squash your commits according to CONTRIBUTING.md before human code review? |
c57dd55 to
243c627
Compare
BenWestgate
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 243c627380
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| field.set_visible(True) | ||
| state = reading.read(reading.normalize(typed()), length=len(card.text)) if boxes else field.reading() | ||
| # A missing or extra character may be submitted: its group is then the one highlighted. | ||
| accept.set_sensitive(abs(len(state.text) - state.expected) <= reading.SLACK) |
There was a problem hiding this comment.
When B, I, O, or 1 is typed instead of a card symbol, _reformat removes it while retaining the specific lookalike warning; this new length-only condition nevertheless enables submission because the normalized value is only one character short. check() then aligns that normalized value, permanently discarding the offending glyph and replacing its useful “probably 8/J/L/0” diagnostic with the generic mismatch message. Require the lookalike fault to be clear before enabling confirmation, while still allowing genuine missing or extra symbols.
Useful? React with 👍 / 👎.
| 2,000 until the card read-back rework of 2026-10-07, which added fixed | ||
| correction boxes, group alignment and paste refusal. |
There was a problem hiding this comment.
This note records the read-back rework, but the review map immediately above still lists reading.py and entry.py at 131 and 73 logical lines; using the test's counting rule they are now 154 and 111. It also still says only the first three modules make decisions even though entry.py now owns the primary-selection and paste-refusal security behavior, so reviewers following the documented order can under-review the newly added boundary logic. Update the table, totals, and review guidance alongside the budget rationale.
Useful? React with 👍 / 👎.
243c627 to
62e1a08
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: code ACK 62e1a08. Retry boxes retain matched-group boundaries, normalize spaces, reject look-alike faults, and share the primary-selection guard. The review map now covers the new security-sensitive entry code. Before human review: satisfy the requested atomic commit cleanup and exercise GTK read-back on Tails.
|
I'm not crACKing the size of this PR +300 lines to fix something that already works properly in the CLI. That's a sign we probably aren't putting the Library API in charge of enough that they can share their good UX.
cACK, nit: arrow (by character) and Ctrl + arrow should also jump to next/last non-matching groups.. Tab having the same function as Ctrl+right arrow (except looping), might be nice if Home and End worked also.
nit: leftmost group stays right. elsewhere in the mixed correct incorrect data you may minimize incorrect groups.
Why 8 characters? Why force them to fix invalid characters? Just show the groups as wrong, right? Or do you want to educate the users about the charset here rather than just get them setup as quickly and easily as possible?
cACK
Thanks
This is causing maintenance and review difficulty. Lets just get everything Bails needs (and I asked for) into gui-reviewability-v1 and pin Bails to that. I can't stand this having two different staging branches for the GUI. |
Consolidate the existing Bails restore safeguards on gui-reviewability-v1. Verify the recorded fingerprint before listing, creating or unlocking a destination. Restoring without a record remains an explicit informed choice; fresh creation records its new fingerprint. Replays the established Core, CLI and GUI safeguards together so Bails can use one GUI staging branch. refs #109
Move the existing CLI alignment into a toolkit-free library module so the graphical interface can reuse the same grouping and tie-breaking rules. Return only entered characters; comparison ignores whitespace and ASCII case without treating Unicode lookalikes as printed card symbols. Exercise alignment with external strings, invalid characters, leading matches, and a property that alignment never completes the transcription. Existing CLI confirmation tests use the shared helper. refs #109
Use the library alignment shared with the CLI instead of maintaining a second alignment algorithm. Keep matching groups locked and let arrows, Ctrl+arrows, Tab, Home and End navigate the remaining boxes. Reuse read-back entry normalization and its clipboard and paste controls for each box. Compare invalid glyphs and missing groups directly, without the former eight-character tolerance. Preserve oversized input and report the comparison limit explicitly. Clear retained correction widgets synchronously, and leave boundary-arrow behavior to GTK when no adjacent editable group exists. Validation: GTK regression tests and the graphical walkthrough; library/CLI confirmation tests, Ruff and mypy. An independent code review found two issues, both fixed with regressions. refs #109
62e1a08 to
6f9e3ef
Compare
BenWestgate
changed the title
💡 Codex Reviewpython-codex32/src/codex32_gui/pages.py Line 534 in 6f9e3ef Move read-back alignment off the GTK main loop
When a mismatched transcription approaches the advertised 1,024-character comparison limit, this synchronous alignment blocks the GTK event handler for a substantial time. Calling the same helper with 1,024 observed characters and a 127-character expected card took approximately 58 seconds locally; even 512 characters took 13 seconds. During that interval the window cannot repaint, navigate, or respond to requests to hide the card. Run alignment through the background-work mechanism or bound its computation sufficiently to keep the interface responsive. python-codex32/src/codex32_gui/entry.py Lines 107 to 108 in 6f9e3ef Report dropped lookalikes even when the remaining card validates
In Check a card and the ordinary import flows, entering an otherwise valid card followed by ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Use the library alignment shared with the CLI instead of maintaining a second alignment algorithm. Keep matching groups locked and let arrows, Ctrl+arrows, Tab, Home and End navigate the remaining boxes. Reuse read-back entry normalization and its clipboard and paste controls for each box. Compare invalid glyphs and missing groups directly, without the former eight-character tolerance. Preserve oversized input and report the comparison limit explicitly. Clear retained correction widgets synchronously, and leave boundary-arrow behavior to GTK when no adjacent editable group exists. Validation: GTK regression tests and the graphical walkthrough; library/CLI confirmation tests, Ruff and mypy. An independent code review found two issues, both fixed with regressions. refs #109
6f9e3ef to
fcb1a01
Compare
BenWestgate
changed the title
|
@codex review |
💡 Codex Reviewpython-codex32/src/codex32/correction.py Lines 983 to 986 in fcb1a01 Share admission limits across both case interpretations
For mixed-case damage, passing the same AGENTS.md reference: AGENTS.md:L79-L81 Fit the expanded GUI within its enforced size budget
The updated GUI contains 2,276 logical review lines, so the new 2,250-line limit still makes ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Keep matching groups locked and edit mismatches using arrows, Ctrl+arrows, Tab, Home, and End. Preserve entered text and keep the original card hidden.
Based on #171, which follows #118. This diff contains only GUI changes and their tests.
1,068 tests pass; one is skipped. The size gate fails at 2,276 lines against 2,250; awaiting the maintainer's budget decision. Ruff and library/GUI mypy pass. Supported-Tails testing remains pending.