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

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

 +// 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

@@ -181,6 +191,11 @@ The `window` argument allows the dialog to attach itself to a parent window, mak
 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

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

jkleinsc

One more grammar nit.

docs/api/command-line-switches.md

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

Concerns were addressed.

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

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

Open

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 in the last 24 hours 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 samuelmaddock, ckerr and VerteDinde 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

deepak1556 and others added 6 commits November 14, 2024 01:29

feat: add support for configuring xdg portal version at runtime

3891576

doc: update command-line-switches.md

2b2d490

doc: update command-line-switches.md

981a55f

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

doc: required portal version for defaultPath support

97d0d2c

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

doc: update more occurrances

517183d

fix: remove warning from save dialogs

fde1bdc

deepak1556 force-pushed the robo/support_configuring_xdg_portal_version branch from 097811d to fde1bdc Compare November 13, 2024 16:42

jkleinsc requested changes Nov 13, 2024

View reviewed changes

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

doc: update command-line-switches.md

a870ace

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

deepak1556 requested a review from jkleinsc November 13, 2024 22:45

jkleinsc approved these changes Nov 15, 2024

View reviewed changes

jkleinsc merged commit 4fb5aab into main Nov 15, 2024
54 checks passed

jkleinsc deleted the robo/support_configuring_xdg_portal_version branch November 15, 2024 14:31

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

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Conversation

Description of Change

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment