Skip to content

fix(ui): a tool chosen with no document runs once the picked file opens - #1681

Closed
marcoarnulfo wants to merge 1 commit into
storytold:mainfrom
marcoarnulfo:fix/open-then-run-async
Closed

marcoarnulfo wants to merge 1 commit into
storytold:mainfrom
marcoarnulfo:fix/open-then-run-async

Conversation

@marcoarnulfo

Copy link
Copy Markdown
Contributor

What this changes

Since #1493, a running app opens files on the background worker. open_then_run (a tool chosen with no document open, e.g. Export ▸ Text) still checked for the new tab right after open_path. By then the file hadn't opened yet, so it opened a frame or two later and the tool never ran. a_tool_chosen_with_no_document_asks_for_one_and_runs_on_it has been red on main since then.

The fix reuses PendingOpen.then, which already carried the command through the password prompt. open_path becomes open_path_then(path, None), and open_then_run calls open_path_then(path, Some(command)):

  • Worker path: the command goes into the OpenRequest, and finish_pending_open runs it once the document is committed. An encrypted file hands it to the password prompt, as before.
  • Converted files: a picked text or image file comes back as ResultKind::Conversion, which dropped then. It now runs the command once the converted PDF is open.
  • Inline path (run_inline, or before the first frame): the behaviour is unchanged. It runs straight after the open, or hands the command to the password prompt.

I also checked the other open_path callers that look at views.len() straight afterwards. control.rs already handles a pending open. reopen_last_files runs before the first frame, so it is always inline. Reopen Closed Tab does have the same pattern, but it only decides whether to pop the history entry, so I left it alone (see below).

How you verified it

Commands run on macOS (Apple silicon):

  • cargo test -p pdfcraft-ui-egui --test commands: 22 passed. The two tests below also pass on their own with --exact.
  • cargo test -p pdfcraft-ui-egui --lib -- open, and -- tab_context password: green.
  • cargo clippy -p pdfcraft-ui-egui --all-targets -- -D warnings: clean.
  • cargo fmt --all -- --check: clean.

Tests. The two tests now run frames until the open lands, with a limit of 200 frames × 10 ms, instead of a fixed 3–4 frames:

  • a_tool_chosen_with_no_document_asks_for_one_and_runs_on_it covers a PDF on the worker, a text file on the worker (conversion) and a PDF inline. Each case checks that the Export dialog is up.
  • the_open_recent_menu_lists_files_and_opens_one failed on the Windows runner in Desktop: prepare large PDFs off the UI thread #1493's CI for the same timing reason.

On main the first test fails with and the tool runs on it: None. I reverted each part of the fix on its own, and each one turns the test red on its own case:

  • then: None in the request
  • dropping the command in the Conversion arm
  • dropping the inline execute

Anything a reviewer should look at first

Reopen Closed Tab (tab_context.rs) pops its history entry only when views.len() grew right after open_path. On the worker path the entry now stays until a later reopen finds the tab already open. Popping it whenever the open is merely scheduled would lose the entry if the read then fails, and the comment there asks to keep it until a tab actually opens. That needs a decision on its own, so it isn't in this PR.

Since storytold#1493 a live app opens files on a background worker, so the file is
not open yet when open_then_run looks for it right after open_path, and
the tool never ran. The command now travels with the pending open (and
with a converted text or image file), as the password prompt already did.
@marcoarnulfo

Copy link
Copy Markdown
Contributor Author

A note on the red CI here, since it looks like this PR breaks document_links: it doesn't, those tests fail on main too. cargo test stops at the first failing test binary, and on main that's commands.rs (the bug this PR fixes), so document_links never got to run there. With this PR commands.rs passes and CI reaches it for the first time.

Locally on 7437bde, --test document_links fails 3 or 4 tests depending on the run (:178, :194, :423, :510), the same ones CI shows here. It looks like timing rather than the password or consent flow: since #1493 a remote link opens on the background worker, and the tests check for the password prompt or the new tab a few frames after the click, before the worker has finished. Waiting until the pending opens drain (or opening those links inline under test, the way open_path does with run_inline) should make them deterministic. Happy to send that separately if useful.

@marcoarnulfo

Copy link
Copy Markdown
Contributor Author

Follow-up to my note above: #1764 by @LabDennis already makes the document_links tests wait for the background open (and covers this PR's case too), so I won't send a separate fix. Happy to close this one if you'd rather take #1764.

@marcoarnulfo

Copy link
Copy Markdown
Contributor Author

Closing: #1914 (thanks @skylartaylor) landed the same continuation, on the worker, conversion and inline paths, and goes further with the one-tab-per-file case. Nothing left here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant