Utils: fall back when a Linux file dialog errors#6274
Open
patrickwehbe wants to merge 1 commit into
Open
Conversation
On Linux the native file pickers (open_filename, save_filename and open_directory) shell out to kdialog or zenity and read only the helper's stdout, ignoring its exit code. kdialog/zenity exit 0 on a selection, 1 on a user cancel, and >=2 on an error. Because the exit code was discarded, an errored helper produced empty stdout that was indistinguishable from a user cancel, so the dialog silently returned None and the tk fallback was never reached. Add a _run_file_dialog helper that interprets the exit code and returns (handled, selection): handled is True for a selection (0) or user cancel (1), and False on any error so the caller falls through to the next backend. The three pickers now stop only when the helper reports a conclusive result; a broken kdialog now correctly falls back to zenity and then tk. messagebox is left untouched since it is not a file picker. Fixes ArchipelagoMW#5991.
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
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 is this fixing or adding?
Fixes #5991.
On Linux the native file pickers
open_filename,save_filenameandopen_directoryshell out tokdialogorzenityvia_run_for_stdout, which reads only the helper's stdout and discards its exit code. These helpers exit0when the user picks a path,1when the user cancels, and>= 2on an error (e.g. a missing display, a malformed argument, or the binary failing to start).Because the exit code was thrown away, an errored helper returned empty stdout that looked identical to a user cancellation: the picker silently returned
None, the error was swallowed, and the existing tk fallback was never reached. The user just saw "nothing happened".This PR adds a small
_run_file_dialoghelper next to_run_for_stdoutthat interprets the exit code and returns(handled, selection):0->(True, <path>)-- a selection was made;1->(True, None)-- the user cancelled (conclusive, do not fall back);(False, None)so the caller falls through to the next backend.Each of the three pickers now replaces its
return _run_for_stdout(...)calls for both kdialog and zenity with a fall-through pattern that returns only when the helper reports a conclusive result. A brokenkdialogtherefore now correctly falls back tozenity, and then to tk.messageboxis intentionally left unchanged since it is not a file picker.How was this tested?
test/general/test_utils.pywith mock-based tests that patchUtils.is_linux,Utils.env_cleared_lib_path,shutil.whichandsubprocess.runto exercise the Linux branch on any OS:0returns the chosen path and never consults the next backend;1(user cancel) returnsNoneand does not fall back;2(tool error) falls back to the next backend (zenity), which then succeeds.python -m pytest test/general/test_utils.py -q-> 3 passed.python -m py_compile Utils.py-> clean.python -m flake8 --max-line-length=120 Utils.py test/general/test_utils.py-> no new warnings on the changed lines.