[NRT-751] Disable the editor Suggest button when there's nothing to suggest#1297
Draft
RisingOrange wants to merge 20 commits into
Draft
[NRT-751] Disable the editor Suggest button when there's nothing to suggest#1297RisingOrange wants to merge 20 commits into
RisingOrange wants to merge 20 commits into
Conversation
Disable the editor's "Suggest a change" button (tooltip "No changes to suggest") when an existing AnkiHub note has nothing to suggest. Reuses the dialog's compute_note_diffs / any_suggestible_from_diffs so the button and the dialog's pre-open gate agree on what counts as suggestible (field edit outside the globally-protected denylist, or a tag change). Three layers: visual gate in _refresh_buttons, action gate in _on_suggestion_button_press (covers the keyboard shortcut), and a typing-timer hook so the state tracks edits live. Behind AUTO_PROTECT_FEATURE_FLAG.
…dentity Codex review follow-ups: - The keyboard-shortcut path now early-returns with the deleted-note tooltip for notes deleted on AnkiHub, matching the visual gate (the shortcut fires even when the button is disabled). - _on_editor_typing_timer matches the owning editor by object identity instead of note.id, so unsaved Add-Cards notes (all id 0) don't refresh every open editor.
…bled Pins down that a personal AnkiHub_Protect tag does not exclude an edited field from the gate (only globally-protected fields do) — the user can still suggest a locally-protected edit.
editor_did_fire_typing_timer only fires for field typing, so adding or removing a tag never re-ran the gate. Register the same refresh handler on editor_did_update_tags (same Note-only signature, available since 2.1.20) and rename it _refresh_buttons_for_edited_note to reflect that it now serves both signals.
The module had two editor-tracking mechanisms: the new `_tracked_editors` registry, and a pre-existing `editor` global that held only the last AddCards editor and was read solely by `_on_add_cards_did_change_notetype` (so a note-type change in an older of two Add windows refreshed the wrong editor). Drop the global and `_on_add_cards_init`; the note-type-change handler now refreshes all live tracked editors via the registry. Factored the prune-and-iterate into `_live_tracked_editors`.
A note-type change in AddCards goes through editor.loadNote, which fires editor_did_load_note -> _refresh_buttons on the affected editor (verified in aqt at 2.1.50 and 25.x). The dedicated add_cards_did_change_note_type handler was therefore redundant — and fired earlier (synchronously after loadNote() returns, before the webview callback), racing the DOM. Drop it and its now-unused `editor` global lineage; the live-editor registry now serves only the field-typing and tag-update hooks. Removes the unused NoteType import and the brittle note-type-change test (it failed against both the old and new handler — a webview-timing artifact, not a real regression; the non-AnkiHub-type disable path stays covered by test_suggestion_button_is_disabled_for_notes_without_ankihub_note_type).
The gate previously applied only to the change-note branch, so a new note (AnkiHub note type, not yet on AnkiHub) with an empty first field kept the button enabled while having nothing to suggest. Extend the gate to the new-note branch via a shared _apply_suggestion_button_gate helper; any_suggestible_from_diffs already classifies an empty-first-field new note as not suggestible. Still behind AUTO_PROTECT_FEATURE_FLAG.
A new note's only gate reason is an empty first field, but it showed the
"No changes to suggest" tooltip, which is wrong for a note that has no
"changes" to begin with. Report it distinctly: _should_block_for_no_changes
becomes _suggestion_gate_tooltip, returning the reason ("The first field
can't be empty" vs "No changes to suggest") or None when suggestible.
The apostrophe in that new string surfaced a latent bug: the button
label/tooltip setters interpolated values into single-quoted JS literals
with no escaping. Encode the value with json.dumps so quotes/apostrophes
can't break the script.
… registry Codex/review reconciliation: - _suggestion_gate_tooltip: new notes have no AH-DB row, so resolve the deck from the note type (ankihub_did_for_note_type) instead of leaving the globally-protected map empty. A new note whose first field is globally protected is now gated, matching the dialog. - Action gate (_on_suggestion_button_press): gate AnkiHub-note-type notes including new ones, so an empty new note's hotkey no longer falls through to add_current_note (which also avoids leaking the on_did_add_note callback). - _track_editor prunes sip-deleted editors on append so _tracked_editors can't accumulate across a session.
The button is visually disabled for non-AnkiHub notes, but the keyboard shortcut still fires (disables=False). Before this commit it fell through to open_suggestion_dialog_for_single_suggestion, which asserts the note's note type is an AnkiHub one — so the hotkey on a non-AnkiHub note raised AssertionError. Early-return from _on_suggestion_button_press in that case (and when editor.note is None) so the action matches the visual state.
…rade-off Codex pass-3 follow-ups: - Move the "note is None or non-AnkiHub note type" no-op above the login check in _on_suggestion_button_press, so a logged-out user pressing the hotkey on a non-AnkiHub note doesn't get a misleading login prompt. Parametrized the no-op test over logged_in. - Document in _suggestion_gate_tooltip that change_type is treated as non-DELETE by design: an unchanged existing AH note gets the "no changes" tooltip rather than enabling the button on the chance the user might DELETE. DELETE-from-editor for unchanged notes is intentionally not supported by NRT-508/751; the browser's "Suggest to delete" remains the path for that.
…on_dialog editor.py was inlining the same dict-comp that already exists as _globally_protected_fields_by_mid in suggestion_dialog.py. Drop the underscore and import it from editor.py so the editor button's gate and the dialog's pre-open gate share the same map-builder.
The helper builds the typed Dict[NotetypeId, Set[str]] shape that `_is_suggestible_from_diff` / `any_suggestible_from_diffs` and the dialog widget consume — i.e. it's an adapter for the suggestions API's contract, not a GUI concern. Move it next to those consumers in main.suggestions and re-import in suggestion_dialog.py and editor.py.
…refresh test - Replace the repeated dialogs.open + set_note + wait_suggestion_button_ready triple across the 12 new TestEditor cases with a single self._open_addcards(anki_note, qtbot, mocker) helper. - Add test_new_note_button_refreshes_live_as_first_field_is_filled, covering the previously-untested new-note path of the typing-timer refresh — the case the tracked.note-is-note identity match exists for (unsaved notes share id 0).
(a) Drop the standalone flag-off test; fold a flag-off row into the
consolidated existing-note gate test.
(c) Consolidate the three action-gate "blocks" tests (no-changes,
deleted note, empty-first-field new note) into one parametrized
test driven by setup callables.
(d) Consolidate the four visual-gate-existing-note tests into one
parametrized test driven by named mutator helpers
(_no_mutation, _set_front, _globally_protect_front_and_set,
_locally_protect_front_and_set).
(e) Consolidate the two visual-gate-new-note tests into one
parametrized test sharing the same mutator helpers.
Net: ~125 fewer test lines vs the previous state, same 26
TestEditor cases. The parametrize-id strings preserve diagnostic
clarity on failure.
Switch each consolidated parametrize from `pytest.param(...id=...)` to a plain tuple per row + a parallel `ids=[...]` list. The shorter tuple fits well under the line-length limit, so each scenario stays on a single line — and `# fmt: off` blocks guard against future widening. Diagnostic clarity is preserved (the `ids` strings are the same).
…kind The pre-existing tooltip "Send your request to AnkiHub (<hotkey>)" used "request", which doesn't match the feature's vocabulary anywhere else (button labels, dialog title, command enum all use "Suggest" / "Suggestion"). Make the wording match the dynamic button label: - change-note: "Suggest a change to AnkiHub (<hotkey>)" - new-note: "Suggest the note to AnkiHub (<hotkey>)" `_default_suggestion_button_tooltip` now takes an `is_new_note` flag threaded through `_apply_suggestion_button_gate`. The initial `tip=` on the button at creation time uses the change-note variant, matching the default `Editor.ankihub_command`.
RisingOrange
changed the title
AnkiHubNote.get_or_none(anki_note_id=0) already returns None for unsaved notes, so the `if note.id != 0 else None` guard was dead weight — fetch unconditionally, matching how _refresh_buttons looks up the AH-DB row.
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.
Related issues
NRT-749), whose diff-based suggestion API (compute_note_diffs/any_suggestible_from_diffs) this reuses. Merge [NRT-749] Redesign Suggest-a-change dialog with "Include in suggestion" section #1288 first; this PR targetsNRT-749.Proposed changes
When a user is in the editor for an AnkiHub note that has nothing to suggest (no field diff vs the AnkiHub-stored note, no tag change), the toolbar's "Suggest a change" button is now disabled with the tooltip "No changes to suggest". Previously the button was always enabled and submitting opened a dialog that couldn't produce a suggestion. Implements the NRT-508 spec line: "If the user didn't edit any field the 'Suggest a change' button should be disable[d]."
Gating lives in
ankihub/gui/editor.py, behind theauto_protect_fields_when_editedfeature flag (matching NRT-749's dark-merge strategy):_refresh_buttons, both the change-note (not-deleted) and new-note branches disableSUGGESTION_BTN_IDand set a tooltip explaining why when the note has nothing to suggest: "No changes to suggest", or "The first field can't be empty" (the latter is a new note's only possible gate reason). View / View-history buttons are managed per branch as before._on_suggestion_button_press, an early-return covers the keyboard-shortcut path (the shortcut fires even when the button is visually disabled): a no-op for non-AnkiHub note types (closing a pre-existingAssertionErrorfrom the dialog opener), the gate tooltip when nothing's suggestible (including empty/globally-protected-first-field new notes, so they don't fall through toadd_current_note), and the deleted-note tooltip for notes deleted on AnkiHub.editor_did_fire_typing_timer(field edits) andeditor_did_update_tags(tag edits) so the button tracks edits within the current note instead of only updating on note reload."Suggestible" is computed via
any_suggestible_from_diffs— the same check the bulk-suggest dialog uses — so the button and the dialog agree: a field edit outside the globally-protected denylist, or a tag change (and, for a new note, a non-empty first field). Locally/personally-protected fields (AnkiHub_Protect::Field) still count: the protect tag only stops sync from overwriting the local edit; the user can still suggest it. Only globally-protected (deck-owner-managed) fields are excluded.The gate predicate returns the tooltip reason (or
Nonewhen suggestible) rather than a bare bool, so each block reason shows the right message. For a new note (no AnkiHub-DB row) the deck is resolved from its note type, so a globally-protected first field gates the button just as it would block the dialog. The button's enabled-state tooltip was also rephrased from "Send your request to AnkiHub" to "Suggest a change to AnkiHub" (or "Suggest the note to AnkiHub" for new notes) — "request" was the odd word out vs the feature's "Suggest" vocabulary. The button label/tooltip JS setters now encode their value withjson.dumps— a latent bug surfaced by the apostrophe in the new tooltip (values were interpolated into single-quoted JS literals with no escaping).Editor-registry, and the removed note-type handler
The live-refresh hooks pass only a
Note, not theEditor, so this PR adds a_tracked_editorsregistry (populated oneditor_did_init, pruned lazily, matched by object identity) used by the field-typing and tag-update handlers.While here, the pre-existing
editormodule global and itsadd_cards_did_change_note_typehandler were removed: a note-type change in AddCards goes througheditor.loadNote, which already fireseditor_did_load_note -> _refresh_buttonson the affected editor (verified in aqt at 2.1.50 and 25.x). The explicit handler was redundant and actually fired earlier — synchronously afterloadNote()returns, before the webview callback — racing the DOM.Note: delete-from-editor for unchanged notes
The gate treats
change_typeas non-DELETE: an unchanged existing AnkiHub note shows "No changes to suggest" and the button is disabled, even though the dialog's change-type dropdown would let the user pickDELETE(suggest_note_updateexempts DELETE from its empty/no-changes checks). This matches the NRT-508/751 spec wording — "if the user didn't edit any field the button should be disabled" — and removes one minor pre-existing path: deleting unchanged notes via the editor's Suggest button. The browser's "Suggest to delete" menu remains the supported path for that flow.How to reproduce
With the
auto_protect_fields_when_editedflag on:With the flag off, the button behaves as before (always enabled for valid notes).
Tests
New tests in
TestEditor(tests/addon/test_integration.py), consolidated into three parametrized tests + four standalone ones: the existing-note visual gate (no-change disabled, field edit enabled, globally-protected-only edit excluded, locally-protected edit still enabled, flag-off no-op), the new-note visual gate (empty / filled / globally-protected-first-field), the action-gate blocks (no-changes, deleted, empty-new-note keyboard paths), live refresh on typing-timer + tag-update (existing notes) and on typing into a new note, and a no-op test for the non-AnkiHub hotkey path (logged-in / logged-out). All 26TestEditortests pass; mypy + ruff clean.