fix: revert required portal version for file chooser dialogs by deepak1556 · Pull Request #44426 · electron/electron

deepak1556 · 2024-10-28T12:29:20Z

Description of Change

Revert the version bump to 3, to help address [Bug]: Electron 32.1 does not use the portals inside a flatpak application #43819
Provide xdg-portal-required-version flag for application developers to control the portal usage (ex: requires support of current_folder)
If portal code path is used and a call to dialog api with default_path is performed then log a warning pointing to the above flag.

Release Notes

Notes: fix file chooser dialogs for flaptak applications

ckerr

Need to tweak the lambda capture to fix a the build, but otherwise looks good

2024-10-28T16:06:16.0232728Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:304:26: error: variable 'xdg_portal_required_version' cannot be implicitly captured in a lambda with no capture-default specified
2024-10-28T16:06:16.0234398Z   304 |                          xdg_portal_required_version) {
2024-10-28T16:06:16.0235038Z       |                          ^
2024-10-28T16:06:16.0236274Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:279:18: note: 'xdg_portal_required_version' declared here
2024-10-28T16:06:16.0237693Z   279 |     unsigned int xdg_portal_required_version) {
2024-10-28T16:06:16.0238343Z       |                  ^
2024-10-28T16:06:16.0239107Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:11: note: lambda expression begins here
2024-10-28T16:06:16.0257715Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0258347Z       |           ^
2024-10-28T16:06:16.0259550Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: capture 'xdg_portal_required_version' by value
2024-10-28T16:06:16.0260754Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0261332Z       |            ^
2024-10-28T16:06:16.0261795Z       |            xdg_portal_required_version
2024-10-28T16:06:16.0263318Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: capture 'xdg_portal_required_version' by reference
2024-10-28T16:06:16.0264786Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0265383Z       |            ^
2024-10-28T16:06:16.0265861Z       |            &xdg_portal_required_version
2024-10-28T16:06:16.0266833Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: default capture by value
2024-10-28T16:06:16.0267775Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0268299Z       |            ^
2024-10-28T16:06:16.0268662Z       |            =
2024-10-28T16:06:16.0269428Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: default capture by reference
2024-10-28T16:06:16.0270436Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0271011Z       |            ^
2024-10-28T16:06:16.0271410Z       |            &
2024-10-28T16:06:16.0273104Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:311:68: error: variable 'xdg_portal_required_version' cannot be implicitly captured in a lambda with no capture-default specified
2024-10-28T16:06:16.0275101Z   311 |             VLOG(1) << "File chooser portal expected version: " << xdg_portal_required_version;
2024-10-28T16:06:16.0276082Z       |                                                                    ^
2024-10-28T16:06:16.0277430Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:279:18: note: 'xdg_portal_required_version' declared here
2024-10-28T16:06:16.0278864Z   279 |     unsigned int xdg_portal_required_version) {
2024-10-28T16:06:16.0279470Z       |                  ^
2024-10-28T16:06:16.0280276Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:11: note: lambda expression begins here
2024-10-28T16:06:16.0281219Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0281756Z       |           ^
2024-10-28T16:06:16.0282850Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: capture 'xdg_portal_required_version' by value
2024-10-28T16:06:16.0283949Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0284514Z       |            ^
2024-10-28T16:06:16.0284949Z       |            xdg_portal_required_version
2024-10-28T16:06:16.0286220Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: capture 'xdg_portal_required_version' by reference
2024-10-28T16:06:16.0287410Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0287985Z       |            ^
2024-10-28T16:06:16.0288440Z       |            &xdg_portal_required_version
2024-10-28T16:06:16.0289419Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: default capture by value
2024-10-28T16:06:16.0290697Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0291290Z       |            ^
2024-10-28T16:06:16.0291669Z       |            =
2024-10-28T16:06:16.0292501Z ../../ui/shell_dialogs/select_file_dialog_linux_portal.cc:291:12: note: default capture by reference
2024-10-28T16:06:16.0293563Z   291 |           [](scoped_refptr<dbus::Bus> bus,
2024-10-28T16:06:16.0294133Z       |            ^
2024-10-28T16:06:16.0294535Z       |            &
2024-10-28T16:06:16.0294919Z 2 errors generated.

samuelmaddock · 2024-10-29T14:45:53Z

docs/api/command-line-switches.md

+
+Sets the minimum required version of XDG portal implementation to `version`
+inorder to use the portal backend for file dialogs on linux. Current
+default is set to `4`.


Who is the intended consumer for this switch? Application developers, users, or both? For the recent bump to v4, is this so application users can switch back the behavior if they require it?

I also wonder if we should consider reverting the default to v3. #43570 contained an undocument breaking change in behavior that application developers may not be expecting.

I don't think we should revert the default, since it looks like the change was accepted upstream (see https://chromium-review.googlesource.com/c/chromium/src/+/5786982), but we probably should document this as a breaking change, at least retroactively, so people know

@VerteDinde I do think (in agreement w/ deepak below) that we should change back the default, since as of now v4 is so rarely available that every app might be required to float this flag for potentially years.

deepak1556 · 2024-10-30T07:20:55Z

Sorry for the delay, I was testing some idea to see if we can repurpose the function SelectFileDialogLinuxPortal::CheckPortalAvailabilityOnBusThread to not only check for portal availability but also ensure the option current_folder is available on the relevant interfaces but I couldn't find a way to probe it without making the actual method call.

Rethinking this PR, based on the conversations from #43819 it seems the affected scenarios with the portal version bump is harder to recover (generic filechooser inside the flatpak application is opened without read/write access to any file on the host) than the scenario with defaultPath #43310. Thoughts on the following next steps,

Revert the version bump to 3, to help address [Bug]: Electron 32.1 does not use the portals inside a flatpak application #43819
Provide xdg-portal-required-version flag for application developers to control the portal usage (ex: requires support of current_folder)
If portal code path is used and a call to dialog api with default_path is performed then log a warning pointing to the above flag.

jkleinsc

API LGTM

samuelmaddock

API LGTM

Have a question for clarity, but won't block merging this.

docs/api/command-line-switches.md

itsananderson · 2024-11-07T19:29:34Z

patches/chromium/feat_add_support_for_missing_dialog_features_to_shell_dialogs.patch

-constexpr int kXdgPortalRequiredVersion = 3;
 +// Version 4 includes support for current_folder option to the OpenFile method via
 +// https://github.com/flatpak/xdg-desktop-portal/commit/71165a5.
-+constexpr int kXdgPortalRequiredVersion = 4;


There's a suggestion in the GH issue about querying for the available portal version rather than hardcoding to 3 or 4. Would that be a viable approach?

SelectFileDialogLinuxPortal::CheckPortalAvailabilityOnBusThread already performs this check, the hardcoded value is the minimum value we check against to use the portal path. The issue is we don't know if a version of portal supports the feature on an interface by querying and this availability check is performed once when the application initializes.

docs/api/command-line-switches.md

jkleinsc

Overall, LGTM, just a small grammar correction and I also wonder if we should be more explicit about explaining that the portal version needs to be 4 or greater to use defaultPath.

docs/api/dialog.md

codebytere · 2024-11-13T11:31:12Z

docs/api/dialog.md

 The `filters` specifies an array of file types that can be displayed, see
 `dialog.showOpenDialog` for an example.

+**Note:** On Linux `defaultPath` is not supported when using portal file chooser


defaultPath is only unsupported on versions <4 for open dialogs - save dialogs work as expected.

Thanks for catching.

For reference, flatpak/xdg-desktop-portal#796

docs/api/dialog.md

codebytere · 2024-11-13T15:50:29Z

@deepak1556 i think a rebase should fix windows

release-clerk · 2024-11-15T14:31:37Z

Release Notes Persisted

fix file chooser dialogs for flaptak applications

trop · 2024-11-15T14:31:44Z

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

trop · 2024-11-15T14:31:47Z

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

trop · 2024-11-15T14:32:21Z

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

trop · 2025-01-14T00:45:25Z

@razzeee has manually backported this PR to "33-x-y", please check out #45180

trop · 2025-01-14T01:44:53Z

@VerteDinde has manually backported this PR to "32-x-y", please check out #45193

razzeee · 2025-04-16T00:24:36Z

Testing with electron fiddle, this seems to be missing from 35? Is that possible?

deepak1556 · 2025-04-16T03:55:28Z

The changes are present but there could be a regression to the patch from a chromium version bump since portal code path was refactored in upstream. Can you open an issue with a repro and we can take a look into it.

razzeee · 2025-04-28T13:02:31Z

@deepak1556 I've opened that repro issue here #46652

deepak1556 added semver/minor backwards-compatible functionality target/33-x-y PR should also be added to the "33-x-y" branch. target/34-x-y PR should also be added to the "34-x-y" branch. labels Oct 28, 2024

deepak1556 requested a review from codebytere October 28, 2024 12:29

deepak1556 requested a review from a team as a code owner October 28, 2024 12:29

electron-cation bot added api-review/requested 🗳 new-pr 🌱 PR opened recently labels Oct 28, 2024

deepak1556 mentioned this pull request Oct 28, 2024

[Bug]: Electron 32.1 does not use the portals inside a flatpak application #43819

Closed

3 tasks

ckerr previously requested changes Oct 28, 2024

View reviewed changes

samuelmaddock reviewed Oct 29, 2024

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened recently label Nov 4, 2024

deepak1556 force-pushed the robo/support_configuring_xdg_portal_version branch from ebc72ab to ee73cf9 Compare November 5, 2024 13:08

deepak1556 changed the title ~~feat: add support for configuring xdg portal version at runtime~~ fix: revert required portal version for file chooser dialogs Nov 5, 2024

deepak1556 requested review from VerteDinde, ckerr and samuelmaddock November 6, 2024 02:11

jkleinsc reviewed Nov 6, 2024

View reviewed changes

samuelmaddock approved these changes Nov 7, 2024

View reviewed changes

docs/api/command-line-switches.md Outdated Show resolved Hide resolved

electron-cation bot added api-review/approved ✅ and removed api-review/requested 🗳 labels Nov 7, 2024

itsananderson reviewed Nov 7, 2024

View reviewed changes

jkleinsc reviewed Nov 12, 2024

View reviewed changes

docs/api/command-line-switches.md Outdated Show resolved Hide resolved

jkleinsc requested changes Nov 12, 2024

View reviewed changes

docs/api/dialog.md Outdated Show resolved Hide resolved

deepak1556 requested a review from jkleinsc November 13, 2024 03:16

deepak1556 added the target/32-x-y PR should also be added to the "32-x-y" branch. label Nov 13, 2024

codebytere requested changes Nov 13, 2024

View reviewed changes

deepak1556 requested a review from codebytere November 13, 2024 14:08

codebytere approved these changes Nov 13, 2024

View reviewed changes

trop bot removed the target/33-x-y PR should also be added to the "33-x-y" branch. label Nov 15, 2024

trop bot added needs-manual-bp/33-x-y needs-manual-bp/32-x-y and removed target/32-x-y PR should also be added to the "32-x-y" branch. labels Nov 15, 2024

trop bot mentioned this pull request Nov 15, 2024

fix: revert required portal version for file chooser dialogs #44681

Merged

trop bot added in-flight/34-x-y merged/34-x-y PR was merged to the "34-x-y" branch. and removed target/34-x-y PR should also be added to the "34-x-y" branch. in-flight/34-x-y labels Nov 15, 2024

Beryesa mentioned this pull request Nov 16, 2024

Flatpak build author-more/penpot-desktop#16

Merged

personalizedrefrigerator mentioned this pull request Dec 10, 2024

Desktop: Upgrade to Electron 32.2.0 laurent22/joplin#11200

Merged

Sn0wCrack mentioned this pull request Jan 8, 2025

Discord is staying on version 0.0.74 until the file chooser portal issue is fixed flathub/com.discordapp.Discord#483

Closed

razzeee mentioned this pull request Jan 12, 2025

fix: revert required portal version for file chooser dialogs #45180

Closed

trop bot added in-flight/33-x-y and removed needs-manual-bp/33-x-y labels Jan 14, 2025

VerteDinde mentioned this pull request Jan 14, 2025

fix: revert required portal version for file chooser dialogs #45193

Merged

trop bot added in-flight/32-x-y merged/32-x-y PR was merged to the "32-x-y" branch. and removed needs-manual-bp/32-x-y in-flight/32-x-y labels Jan 14, 2025

trop bot removed the in-flight/33-x-y label Apr 30, 2025

Conversation

deepak1556 commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Release Notes

Uh oh!

ckerr left a comment

Choose a reason for hiding this comment

Uh oh!

samuelmaddock Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VerteDinde Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

codebytere Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

deepak1556 commented Oct 30, 2024

Uh oh!

jkleinsc left a comment

Choose a reason for hiding this comment

Uh oh!

samuelmaddock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itsananderson Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

deepak1556 Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkleinsc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codebytere Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

deepak1556 Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codebytere commented Nov 13, 2024

Uh oh!

release-clerk bot commented Nov 15, 2024

Uh oh!

trop bot commented Nov 15, 2024

Uh oh!

trop bot commented Nov 15, 2024

Uh oh!

trop bot commented Nov 15, 2024

Uh oh!

trop bot commented Jan 14, 2025

Uh oh!

trop bot commented Jan 14, 2025

Uh oh!

razzeee commented Apr 16, 2025

Uh oh!

deepak1556 commented Apr 16, 2025

Uh oh!

razzeee commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

deepak1556 commented Oct 28, 2024 •

edited

Loading

samuelmaddock Oct 29, 2024 •

edited

Loading