Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: use //ui/shell_dialogs on Linux #42045

Merged
merged 12 commits into from
May 9, 2024
Merged

refactor: use //ui/shell_dialogs on Linux #42045

merged 12 commits into from
May 9, 2024

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented May 6, 2024 •
edited
Loading

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 in the last 24 hours 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

ckerr
ckerr reviewed May 6, 2024
View reviewed changes
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

patches/chromium/feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch Outdated Show resolved Hide resolved
shell/browser/ui/file_dialog_linux.cc Outdated Show resolved Hide resolved
shell/browser/ui/file_dialog_linux.cc Outdated Show resolved Hide resolved
shell/browser/ui/file_dialog_linux.cc Outdated Show resolved Hide resolved
shell/browser/ui/file_dialog_linux.cc Outdated Show resolved Hide resolved
shell/browser/ui/file_dialog_linux.cc Outdated Show resolved Hide resolved
patches/chromium/feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch Outdated Show resolved Hide resolved
patches/chromium/feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch Outdated Show resolved Hide resolved
shell/browser/ui/file_dialog_linux.cc Outdated Show resolved Hide resolved
shell/browser/ui/file_dialog_linux.cc Outdated Show resolved Hide resolved
codebytere and others added 2 commits May 7, 2024 10:04
@codebytere @ckerr
Apply suggestions from code review
e8d475c 
Co-authored-by: Charles Kerr <charles@charleskerr.com>
@codebytere
refactor: use settings struct
2c5d00b
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 7, 2024
ckerr
ckerr approved these changes May 7, 2024
View reviewed changes
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.

deepak1556
deepak1556 approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

👏

@jkleinsc jkleinsc merged commit 865b049 into main May 9, 2024
17 checks passed
@jkleinsc jkleinsc deleted the shell-dialog-linux branch May 9, 2024 13:51
@release-clerk Release Clerk
Copy link

release-clerk bot commented May 9, 2024

No Release Notes

@trop
Copy link
Contributor

trop bot commented May 9, 2024

I was unable to backport this PR to "29-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/29-x-y and removed target/29-x-y PR should also be added to the "29-x-y" branch. labels May 9, 2024
This was referenced May 9, 2024
Merged
Merged
@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
@codebytere codebytere mentioned this pull request May 13, 2024
Merged
@trop
Copy link
Contributor

trop bot commented May 13, 2024

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

@trop trop bot added in-flight/29-x-y merged/29-x-y PR was merged to the "29-x-y" branch. and removed needs-manual-bp/29-x-y in-flight/29-x-y labels May 13, 2024
Mrnikifabio pushed a commit to Mrnikifabio/electron that referenced this pull request May 14, 2024
@codebytere @ckerr @Mrnikifabio
refactor: use //ui/shell_dialogs on Linux (electron#42045)
81632e6 
* refactor: use //ui/shell_dialogs on Linux

* fix: add proper filtering

* fix: add support for missing dialog features to //shell_dialogs

* fix: parent_window could be null

* chore: cleanup patch

* fix: use a OnceCallback in the sync implementation

* chore: remove stray debuglog

* Apply suggestions from code review

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use settings struct

* fix: show hidden file property checking

* chore: changes from review

* fix: multi selection for dialogs

---------

Co-authored-by: Charles Kerr <charles@charleskerr.com>
@ayumi-signal ayumi-signal mentioned this pull request Jun 24, 2024
Open
3 tasks
@codebytere codebytere mentioned this pull request Jul 3, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

@aiddya aiddya aiddya left review comments

@jkleinsc jkleinsc jkleinsc approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

Assignees
No one assigned
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
Milestone
No milestone
5 participants
@codebytere @ckerr @jkleinsc @deepak1556 @aiddya