diff --git a/docs/developer/gui.md b/docs/developer/gui.md index 2fbcbf2e..01c82444 100644 --- a/docs/developer/gui.md +++ b/docs/developer/gui.md @@ -60,7 +60,8 @@ about 250 lines, and the rest of the difference is user-facing wording in page a rewrite has just added keeps what it holds, and a page a rewrite drops is cleared. 6. **One worker, and no callback touches a page that is gone.** `work.run` - refuses to start a second operation, delivers its result through + queues a second operation behind one gate, so an import waits for the + once-a-second wallet-list poll instead of overlapping it, delivers its result through `GLib.idle_add`, and drops it unless `work.showing` finds the page still on the stack — being rooted in the window is not the test, because a page keeps its root for the length of the navigation animation. It delivers from @@ -164,7 +165,6 @@ library's. - A 74- or 127-character card scrolls sideways in the entry field, because it is one `Gtk.Entry`. A 48-character card fits. -- The list of empty Bitcoin Core wallets refreshes when asked, not on its own. - Widget construction is not covered by pytest, because it needs a display. `tools/gui_walkthrough.py` covers it instead, under `xvfb-run`. - Selecting text inside the entry field hands it to the primary selection for diff --git a/src/codex32/_bitcoin_core.py b/src/codex32/_bitcoin_core.py index b1eae17c..13b3479e 100644 --- a/src/codex32/_bitcoin_core.py +++ b/src/codex32/_bitcoin_core.py @@ -9,7 +9,7 @@ import string import subprocess import time -from collections.abc import Callable +from collections.abc import Callable, Iterable from dataclasses import dataclass from time import sleep from typing import Literal @@ -360,11 +360,16 @@ def _target(self, name: str) -> tuple[bool, bool] | None: unlocked = info.get("unlocked_until") return (unlocked is not None, unlocked == 0) if eligible else None + # One observation, never a wait: (name, encrypted, locked) for each eligible wallet among `names`, + # sorted. The CLI's menu and wait below and the GUI's wallet list all decide eligibility here. + def _eligible_wallets(self, names: Iterable[str]) -> tuple[tuple[str, bool, bool], ...]: + return tuple((name, *state) for name in sorted(names) if (state := self._target(name)) is not None) + def _select(self, ask: Callable[[str], str], tell: Callable[[str], None]) -> str: def agree(name: str) -> bool: return ask(f"Use blank wallet {json.dumps(name)}? [y/N]").lower() in ("y", "yes") - choices = tuple(name for name in sorted(self._names()) if self._target(name) is not None) + choices = tuple(row[0] for row in self._eligible_wallets(self._names())) if len(choices) == 1 and agree(choices[0]): return choices[0] while True: @@ -375,8 +380,7 @@ def agree(name: str) -> bool: tell(f" {len(choices) + 1}. Create another wallet in Bitcoin Core") answer = ask("Choose a wallet number") if answer.isdecimal() and 1 <= int(answer) <= len(choices): - name = choices[int(answer) - 1] - if agree(name): + if agree(name := choices[int(answer) - 1]): return name continue if answer != str(len(choices) + 1): @@ -388,13 +392,12 @@ def agree(name: str) -> bool: "private keys enabled." ) tell("Waiting; press Ctrl-C to stop.") - while True: + appeared: tuple[str, ...] = () + while not appeared: sleep(1) names = set(self._names()) - appeared = tuple(name for name in sorted(names - before) if self._target(name) is not None) - if appeared: - break - choices = tuple(name for name in sorted(names) if self._target(name) is not None) + appeared = tuple(row[0] for row in self._eligible_wallets(names - before)) + choices = tuple(row[0] for row in self._eligible_wallets(names)) if len(appeared) == 1 and agree(appeared[0]): return appeared[0] diff --git a/src/codex32_gui/pages.py b/src/codex32_gui/pages.py index 6ed80813..d2a6a5dd 100644 --- a/src/codex32_gui/pages.py +++ b/src/codex32_gui/pages.py @@ -45,7 +45,10 @@ (3, 5, "Five cards, any three recover"), (0, 1, "One card"), ) -CREATE_WALLET = "Create a new wallet" +CREATE_ROW = ( + "Create a new wallet", + "codex32 asks Bitcoin Core for a blank wallet, with a passphrase you choose", +) HANDWRITING = ( "Mark the look-alikes as you write: slash every 0, cross 7 and Z, write S like $, close the loops of 6 " "and 9, and give G an open, obvious bar. Then 5 and S, 6 and G, and 2 and Z stay apart." @@ -338,17 +341,17 @@ def _guessed(observed: str, corrected: str) -> frozenset[int]: return frozenset(changed) -def _radio_group(group: Adw.PreferencesGroup, rows: Sequence[tuple[str, str]]) -> list[Gtk.CheckButton]: - """Build one radio row per choice. Rows carry Bitcoin Core's text, so none of it is markup.""" - first: Gtk.CheckButton | None = None +def _radio_group( + group: Adw.PreferencesGroup, rows: Sequence[tuple[str, str]], action: Gtk.Button +) -> list[Gtk.CheckButton]: + """Build one radio row per choice, none chosen; `action` waits for one. Core's text is never markup.""" + # A hidden leader groups even one row, so it is a radio that cannot be unticked. Handlers keep it alive. + leader = Gtk.CheckButton() + action.set_sensitive(False) buttons: list[Gtk.CheckButton] = [] for label, detail in rows: - choice = Gtk.CheckButton() - if first is None: - first = choice - choice.set_active(True) - else: - choice.set_group(first) + choice = Gtk.CheckButton(group=leader) + choice.connect("toggled", lambda _choice, _leader=leader: action.set_sensitive(True)) row = Adw.ActionRow(title=label, subtitle=detail, activatable_widget=choice, use_markup=False) row.add_prefix(choice) group.add(row) @@ -422,16 +425,16 @@ def _network_page( then: Callable[[BitcoinCore], Adw.NavigationPage], ) -> Adw.NavigationPage: group = Adw.PreferencesGroup(title="Bitcoin Core is running on more than one network") - buttons = _radio_group(group, [(label, "") for label in options]) - content = _column( - _title("Which network?", "Practise on signet. Use mainnet only for coins you cannot replace."), - group, - ) action = _button( "Continue", lambda: _connect(view, options[_selected(buttons)], then), style="suggested-action", ) + buttons = _radio_group(group, [(label, "") for label in options], action) + content = _column( + _title("Which network?", "Practise on signet. Use mainnet only for coins you cannot replace."), + group, + ) return _page("Network", content, actions=_actions(action)) @@ -578,7 +581,8 @@ def _layout_page(view: Adw.NavigationView, core: BitcoinCore) -> Adw.NavigationP "Something else": "Choose the numbers yourself, and the size of the seed.", } group = Adw.PreferencesGroup() - buttons = _radio_group(group, [(label, details[label]) for label in details]) + action = _button("Continue", lambda: begin(), style="suggested-action") + buttons = _radio_group(group, [(label, details[label]) for label in details], action) custom = Adw.PreferencesGroup(title="Your own combination", visible=False) needed = Adw.SpinRow( title="Cards needed to recover", @@ -626,9 +630,7 @@ def begin() -> None: group, custom, ) - return _page( - "New wallet", content, actions=_actions(_button("Continue", begin, style="suggested-action")) - ) + return _page("New wallet", content, actions=_actions(action)) def _ready_page( @@ -798,10 +800,43 @@ def _wallet_page( restoring: bool, ) -> Adw.NavigationPage: """Name the wallet that will hold the keys. The library confirms that name again.""" - group = Adw.PreferencesGroup(title="Empty wallets Bitcoin Core has ready") - rows = [(item.name, "Empty, encrypted" if item.encrypted else "Empty, not encrypted") for item in found] - rows.append((CREATE_WALLET, "codex32 asks Bitcoin Core for a blank wallet, with a passphrase you choose")) - buttons = _radio_group(group, rows) + # Like the CLI, ask Bitcoin Core again every second, so a wallet made there appears here. + holder = Gtk.Box(orientation=Gtk.Orientation.VERTICAL) + action = _button("Continue", lambda: go(), style="suggested-action") + buttons: list[Gtk.CheckButton] = [] + + def show(listed: tuple[wallet_setup.Wallet, ...]) -> None: + nonlocal found, buttons + # A refresh keeps the operator's choice by name; None stands for the create row. + kept = next((n for n, b in zip([w.name for w in found] + [None], buttons) if b.get_active()), False) + found = listed + group = Adw.PreferencesGroup(title="Empty wallets Bitcoin Core has ready") + rows = [(w.name, "Empty, encrypted" if w.encrypted else "Empty, not encrypted") for w in found] + buttons = _radio_group(group, [*rows, CREATE_ROW], action) + for choice, name in zip(buttons, [item.name for item in found] + [None], strict=True): + choice.set_active(name == kept) + if (old := holder.get_first_child()) is not None: + holder.remove(old) + holder.append(group) + + polling = False + + def refreshed(listed: tuple[wallet_setup.Wallet, ...] | Exception) -> None: + nonlocal polling + polling = False + # A failed poll keeps the list shown; Continue reports Bitcoin Core's error if it persists. + if isinstance(listed, tuple) and listed != found: + show(listed) + + def poll() -> bool: + nonlocal polling + # One poll at a time: a slow Bitcoin Core must not pile up queued polls behind the gate. + if view.get_visible_page() is page and not polling: + polling = True + work.run(view, page, lambda: wallet_setup.eligible(core), refreshed) + return work.showing(view, page) + + show(found) def go() -> None: # By position, so that a wallet named like the create row is still reachable. @@ -820,10 +855,10 @@ def go() -> None: "Which wallet should hold your keys?", f"Bitcoin Core {wallet_setup.version_text(core)} is running on {wallet_setup.network(core)}.", ), - group, + holder, _note( - "Only empty wallets are listed, so no wallet you already use can be overwritten. You may also " - "create one in Bitcoin Core yourself and check again." + "Only empty wallets are listed, so no wallet you already use can be overwritten. A wallet you " + "create in Bitcoin Core yourself appears here." ), *( ( @@ -837,16 +872,9 @@ def go() -> None: else () ), ) - return _page( - "Wallet", - content, - actions=_actions( - _button( - "Check again", lambda: _wallets(view, core, secret, timestamp, expected, restoring=restoring) - ), - _button("Continue", go, style="suggested-action"), - ), - ) + page = _page("Wallet", content, actions=_actions(action)) + GLib.timeout_add_seconds(1, poll) + return page def _new_wallet_page( diff --git a/src/codex32_gui/wallet_setup.py b/src/codex32_gui/wallet_setup.py index b8e540d6..14d7d7e0 100644 --- a/src/codex32_gui/wallet_setup.py +++ b/src/codex32_gui/wallet_setup.py @@ -174,12 +174,7 @@ def version_text(core: BitcoinCore) -> str: def eligible(core: BitcoinCore) -> tuple[Wallet, ...]: """List the empty descriptor wallets Bitcoin Core would accept an import into.""" - found = [] - for name in sorted(core._names()): - state = core._target(name) - if state is not None: - found.append(Wallet(name, state[0], state[1])) - return tuple(found) + return tuple(Wallet(*row) for row in core._eligible_wallets(core._names())) def _transferable(text: str, subject: str) -> None: diff --git a/src/codex32_gui/work.py b/src/codex32_gui/work.py index 3bcfa947..626f1464 100644 --- a/src/codex32_gui/work.py +++ b/src/codex32_gui/work.py @@ -14,8 +14,7 @@ "That operation stopped in a way this program did not expect. If it was writing to Bitcoin " "Core, check there what state the wallet is in before trying again." ) -_BUSY = "Another operation is still finishing. Try again in a moment." -_running = False +_gate = threading.Lock() def showing(view: Adw.NavigationView, page: Adw.NavigationPage) -> bool: @@ -38,8 +37,8 @@ def run[Result]( """Run one blocking library call off the main loop and deliver its result back. `correct` runs for up to ten seconds and every Bitcoin Core call waits on a - subprocess, so neither may run on the main loop. The page that starts an - operation shows a spinner and cannot be left, so only one is ever in flight, + subprocess, so neither may run on the main loop. Operations take one gate in + turn, so an import queues behind a wallet-list poll instead of overlapping it, and a result for a page that is gone anyway is dropped. The result is delivered from `finally`, so a failure no screen anticipated still releases the program instead of leaving it spinning. @@ -49,36 +48,20 @@ def run[Result]( blocks in daemon threads while the interpreter is shutting down, so closing the window during an import would otherwise leave that wallet open. """ - global _running - if _running: - GLib.idle_add(_busy, view, page, done) - return - _running = True def worker() -> None: outcome: Result | Exception = RuntimeError(_UNFINISHED) try: - outcome = work() + with _gate: + outcome = work() except (CodexError, BitcoinCoreError, OSError, TypeError, ValueError) as error: outcome = error finally: GLib.idle_add(deliver, outcome) def deliver(outcome: Result | Exception) -> bool: - global _running - _running = False if showing(view, page): done(outcome) return False threading.Thread(target=worker).start() - - -def _busy[Result]( - view: Adw.NavigationView, - page: Adw.NavigationPage, - done: Callable[[Result | Exception], None], -) -> bool: - if showing(view, page): - done(RuntimeError(_BUSY)) - return False diff --git a/tests/test_bitcoin_core.py b/tests/test_bitcoin_core.py index 557f7962..38ae9f3c 100644 --- a/tests/test_bitcoin_core.py +++ b/tests/test_bitcoin_core.py @@ -317,6 +317,21 @@ def ask(prompt: str) -> str: assert messages[1] == "Waiting; press Ctrl-C to stop." +def test_eligible_wallets_observe_once_and_see_a_wallet_become_eligible( + monkeypatch: pytest.MonkeyPatch, +) -> None: + client = BitcoinCore("bitcoin-cli", "main", 300000) + states = iter((None, (True, True))) + names: list[int] = [] + monkeypatch.setattr(BitcoinCore, "_names", lambda _client: names.append(1) or ("later",)) + monkeypatch.setattr(BitcoinCore, "_target", lambda _client, _name: next(states)) + + assert client._eligible_wallets(("later",)) == () + assert client._eligible_wallets(("later",)) == (("later", True, True),) + assert client._eligible_wallets(()) == () + assert names == [] + + @dataclass class _ImportRPC: private: bool = True diff --git a/tests/test_gui_before_you_start.py b/tests/test_gui_before_you_start.py index 36ce9c5d..0631a2d3 100644 --- a/tests/test_gui_before_you_start.py +++ b/tests/test_gui_before_you_start.py @@ -45,6 +45,7 @@ def test_a_layout_choice_leads_to_the_checklist_not_to_a_card(monkeypatch: pytes view = Adw.NavigationView() core: Any = object() view.push(pages._layout_page(view, core)) + _widgets(view.get_visible_page(), Gtk.CheckButton).__next__().set_active(True) _button(view.get_visible_page(), "Continue").emit("clicked") ready = view.get_visible_page() @@ -115,3 +116,105 @@ def launch(self, _parent: object, _cancellable: object, _callback: object) -> No _button(page, "Open the recovery card form").emit("clicked") assert opened == ["/home/amnesia/Tor Browser/codex32 forms/recovery-card.html"] + + +def _wallet(name: str) -> Any: + return pages.wallet_setup.Wallet(name=name, encrypted=True, locked=False) + + +@pytest.fixture(autouse=True) +def _core_text(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(pages.wallet_setup, "version_text", lambda _core: "32.1.0") + monkeypatch.setattr(pages.wallet_setup, "network", lambda _core: "signet") + + +def test_continue_waits_for_a_choice_and_a_lone_row_stays_a_radio() -> None: + view = Adw.NavigationView() + page = pages._layout_page(view, object()) # type: ignore[arg-type] + choices = list(_widgets(page, Gtk.CheckButton)) + assert not any(choice.get_active() for choice in choices) + assert not _button(page, "Continue").get_sensitive() + + page = pages._wallet_page(view, object(), object(), (), "now", None, False) # type: ignore[arg-type] + (choice,) = _widgets(page, Gtk.CheckButton) + assert not _button(page, "Continue").get_sensitive() + choice.activate() + choice.activate() + assert choice.get_active() and _button(page, "Continue").get_sensitive() + assert all(button.get_label() != "Check again" for button in _widgets(page, Gtk.Button)) + + +def test_the_wallet_list_refreshes_like_the_cli_and_keeps_the_choice(monkeypatch: pytest.MonkeyPatch) -> None: + polls: list[Any] = [] + listed = [(_wallet("b"),), (_wallet("a"), _wallet("b"))] + monkeypatch.setattr(pages.GLib, "timeout_add_seconds", lambda _seconds, poll: polls.append(poll)) + monkeypatch.setattr(pages.wallet_setup, "eligible", lambda _core: listed.pop(0)) + monkeypatch.setattr(pages.work, "run", lambda _view, _page, job, done: done(job())) + view = Adw.NavigationView() + view.push(pages.home(view)) + view.push(pages._wallet_page(view, object(), object(), (), "now", None, False)) # type: ignore[arg-type] + + assert polls[0]() is True + page = view.get_visible_page() + assert "b" in _texts(page) + next(_widgets(page, Gtk.CheckButton)).set_active(True) + assert polls[0]() is True + choices = list(_widgets(page, Gtk.CheckButton)) + assert "a" in _texts(page) and [choice.get_active() for choice in choices] == [False, True, False] + assert _button(page, "Continue").get_sensitive() + + view.pop() + assert polls[0]() is False + + +def _polled_page(monkeypatch: pytest.MonkeyPatch, found: tuple[Any, ...], *listed: tuple[Any, ...]) -> Any: + polls: list[Any] = [] + answers = list(listed) + monkeypatch.setattr(pages.GLib, "timeout_add_seconds", lambda _seconds, poll: polls.append(poll)) + monkeypatch.setattr(pages.wallet_setup, "eligible", lambda _core: answers.pop(0)) + monkeypatch.setattr(pages.work, "run", lambda _view, _page, job, done: done(job())) + view = Adw.NavigationView() + view.push(pages.home(view)) + view.push(pages._wallet_page(view, object(), object(), found, "now", None, False)) # type: ignore[arg-type] + return view, polls[0] + + +def test_a_wallet_that_disappears_takes_the_choice_with_it(monkeypatch: pytest.MonkeyPatch) -> None: + view, poll = _polled_page(monkeypatch, (_wallet("a"), _wallet("b")), (_wallet("b"),)) + page = view.get_visible_page() + next(_widgets(page, Gtk.CheckButton)).set_active(True) + poll() + assert not any(choice.get_active() for choice in _widgets(page, Gtk.CheckButton)) + assert not _button(page, "Continue").get_sensitive() + + +def test_a_wallet_named_like_the_create_row_stays_its_own_row(monkeypatch: pytest.MonkeyPatch) -> None: + named = _wallet("Create a new wallet") + imported: list[str] = [] + monkeypatch.setattr(pages, "_import", lambda _view, _core, _secret, name, *_rest: imported.append(name)) + view, poll = _polled_page(monkeypatch, (named,), (named, _wallet("z"))) + page = view.get_visible_page() + list(_widgets(page, Gtk.CheckButton))[-1].set_active(True) + poll() + choices = list(_widgets(page, Gtk.CheckButton)) + assert [choice.get_active() for choice in choices] == [False, False, True] + choices[0].set_active(True) + _button(page, "Continue").emit("clicked") + assert imported == ["Create a new wallet"] + + +def test_a_slow_core_never_queues_a_second_poll(monkeypatch: pytest.MonkeyPatch) -> None: + polls: list[Any] = [] + started: list[Any] = [] + monkeypatch.setattr(pages.GLib, "timeout_add_seconds", lambda _seconds, poll: polls.append(poll)) + monkeypatch.setattr(pages.work, "run", lambda _view, _page, job, done: started.append(done)) + view = Adw.NavigationView() + view.push(pages.home(view)) + view.push(pages._wallet_page(view, object(), object(), (), "now", None, False)) # type: ignore[arg-type] + + polls[0]() + polls[0]() + assert len(started) == 1 + started[0](()) + polls[0]() + assert len(started) == 2 diff --git a/tests/test_gui_boundaries.py b/tests/test_gui_boundaries.py index 1d73c9c7..7d6d0d20 100644 --- a/tests/test_gui_boundaries.py +++ b/tests/test_gui_boundaries.py @@ -132,7 +132,7 @@ def test_the_gui_keeps_its_own_size_budget() -> None: def test_every_restore_way_to_a_wallet_passes_the_identity_choice() -> None: tree = ast.parse((_package() / "pages.py").read_text()) - assert _callers(tree, "_wallets") == {"_fingerprint_page", "_identity", "_wallet_page"} + assert _callers(tree, "_wallets") == {"_fingerprint_page", "_identity"} assert _callers(tree, "_fingerprint_page") == {"_identity", "_restore", "_fingerprint_page"} assert _callers(tree, "_identity") == {"_unshared_page", "_card_confirmed", "_fingerprint_page"}