Skip to content

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented May 6, 2024

Description of Change

Closes #32857
Closes #41903
Refs #19159

This PR replaces our dialog implementation on Linux with upstream //shell_dialogs. This both streamlines and simplifies our dialog logic, and is ideally a first step towards using //shell_dialogs cross-platform. Per #32857 (comment), this is the easiest to port over as it overlaps most completely with what's available upstream. By using Chromium's implementation, we also gain access to kde and portal-specific implementations of dialogs on Linux.

Notes:

  • We should deprecate showOverwriteConfirmation as @aiddya noted in the chart comment, but that should be done in a discrete PR (it's the default on GTK 4+)

cc @bpasero

Checklist

Release Notes

Notes: none

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/29-x-y PR should also be added to the "29-x-y" branch. target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. labels May 6, 2024
@codebytere codebytere requested a review from deepak1556 May 6, 2024 08:24
@codebytere codebytere requested a review from a team as a code owner May 6, 2024 08:24
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label May 6, 2024
@codebytere codebytere added target/29-x-y PR should also be added to the "29-x-y" branch. target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. and removed target/29-x-y PR should also be added to the "29-x-y" branch. target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. labels May 6, 2024
@ckerr
Copy link
Member

ckerr commented May 6, 2024

@codebytere also noticed https://issues.chromium.org/issues/41469294#comment10, which says that upstream likely wants to support GtkFileChooserNative on GTK4

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea, I love it. Especially with upstream indicating they will support the native dialogs in GTK4.

I have some implementation suggestions here

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label May 7, 2024
@codebytere codebytere force-pushed the shell-dialog-linux branch from a5020e0 to 09f223d Compare May 7, 2024 14:02
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@trop
Copy link
Contributor

trop bot commented May 9, 2024

I have automatically backported this PR to "31-x-y", please check out #42109

@trop trop bot added in-flight/31-x-y and removed target/31-x-y PR should also be added to the "31-x-y" branch. labels May 9, 2024
@trop
Copy link
Contributor

trop bot commented May 9, 2024

I have automatically backported this PR to "30-x-y", please check out #42110

@trop trop bot added in-flight/30-x-y merged/31-x-y PR was merged to the "31-x-y" branch. merged/30-x-y PR was merged to the "30-x-y" branch. and removed target/30-x-y PR should also be added to the "30-x-y" branch. in-flight/31-x-y in-flight/30-x-y labels May 9, 2024
@trop
Copy link
Contributor

trop bot commented May 13, 2024

@codebytere has manually backported this PR to "29-x-y", please check out #42144

@drankinatty
Copy link

If this is the change where you search for command line utilities to use as your file-chooser, like kdialog, etc.., this breaks fileChoose display on Trinity Desktop Environment TDE/KDE3. It further breaks any chooser where multiple versions of a chooser like kdialog are installed and the $PATH ordering places the older first.

Any fileChooser scheme should not depend on how the $PATH variable may or may not be set and should not utilize command-line utilities to generate fileChoosers without validating the version of the utility and whether or not it supports the options it is sent. Ultimately, this breakage is one of basic validation and providing a fallback that was not done in this change. If the fileChooser selected does not display and instead throws an error, then that needs to be handled and the default Gtk choosers need to be shown as they were up until vscode ver: 1.89.

See the bug linked by @daiyam above Linux - vscodium no longer displayes the file-dialog after update to Ver. 1.9.0. This affects more than just TDE/KDE3 as noted by the comments in that bug report. It also effects vscode/vscodium identically. Several work-arounds (ugly hacks) are included in the linked report that either involve intercepting the call to the command-line utility and rewriting the command or to alter the $PATH to remove the search-path to the utility forcing the use of the normal Gtk dialogs. Neither are realistic.

It may be convenient to simply search the Linux PATH for a set of executable names to use as fileChoosers, but there is no telling what you actually find. So there needs to be additional validation that the fileChoose found is actually what you think it is, and if not, provide the Gtk fallback.

Lastly, there needs to be a clear way to disable this new behavior and restore the default Gtk dialogs on desktop that this breaks fileChooser display on. Mozilla provides widget.use-xdg-desktop-portal.file-picker which when set to 0 disables searching for native chooser utilities and restores default Gtk dialog use. Something like that in the vscode preferences would work, or a command line option. Because, above all, a New Feature to one, is a BUG to another -- if it cannot be turned off.

Hopefully the behavior of this commit can be made more robust to prevent the dialog failures it causes. Absent that a clear option to disable it and restore default dialogs would be greatly appreciated.

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

Labels

merged/29-x-y PR was merged to the "29-x-y" branch. merged/30-x-y PR was merged to the "30-x-y" branch. merged/31-x-y PR was merged to the "31-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: File picker not always on top [Bug]: showOpenDialog / showSaveDialog opens _behind_ the app after the second call onward

7 participants