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.

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

filechooser: Support current_folder with OpenFile #874

Closed

Conversation

sophie-h
Copy link
Contributor

@sophie-h sophie-h commented Aug 29, 2022

gnomesysadmins pushed a commit to GNOME/xdg-desktop-portal-gnome that referenced this pull request Aug 29, 2022
sophie-h added a commit to sophie-h/ashpd that referenced this pull request Aug 29, 2022
sophie-h added a commit to sophie-h/xdg-desktop-portal-gtk that referenced this pull request Aug 30, 2022
sophie-h added a commit to sophie-h/xdg-desktop-portal-gtk that referenced this pull request Aug 30, 2022
sophie-h added a commit to sophie-h/xdg-desktop-portal-gtk that referenced this pull request Aug 30, 2022
@sophie-h sophie-h marked this pull request as ready for review August 30, 2022 17:22
@matthiasclasen
Copy link
Contributor

I think current_folder is problematic in portal api - What filesystem view does this refer to, the one inside the sandbox, or the one outside? Sandboxed apps may not have any idea about the outside view.

@matthiasclasen
Copy link
Contributor

Depending on the use case, it may be possible to pass a reference to a file in the document store, and say: 'open in the folder that this file comes from'

@sophie-h
Copy link
Contributor Author

What filesystem view does this refer to

I mean this is not any different from the current_folder option for SaveFile. It's the host location.

Sandboxed apps may not have any idea about the outside view.

That's not really true. For example, $HOME is identical to the host location. Therefore, things like get_user_special_dir should work, I guess? That would allow a music app to open the file chooser in the Music folder for example. There are also some HOST_ variables, but those are probably less useful in this case.

Further, there are suggestions to use the portal for GtkFileChooserNative on the host. We will lose this feature on the host if it is not supported by the portal.

Also, there have been requests about exposing the file path for opened files #475 #788. This would make this option and the existing options more useful.

I do agree that we need some more sophisticated solutions for file portals, see #847 for example. But I think those things are more like long-shot and should involve some careful design work.

@matthiasclasen
Copy link
Contributor

An alternative suggestion that does not need any new api, and will still address a number of related use cases: Make the portal backend remember the last folder, per-app.

@sophie-h
Copy link
Contributor Author

An alternative suggestion that does not need any new api, and will still address a number of related use cases: Make the portal backend remember the last folder, per-app.

Probably better than no pre-selection at all. However, it would not cover the case of having a new app and using the open folder for example. Also, there are cases where more fine-grained control could be needed. For example, a text editor with several tabs/windows, and the developer wants to open the folder that contains the current file.

Realistically, apps currently do things like --filesystem=host (Text Editor of GNOME does that) for being able to show the file path. I currently don't see how we can get around providing the host file path for opened files as well. Or otherwise, all this discussion about tight sandboxing will not apply to many apps anyways.

@felinira
Copy link

There are also applications like Bottles that allow you to choose a wine executable from within the applications data directory (to run it). Expecting users to ever find the sandbox folder is not really an option though as it is hidden well out of sight.

Preselecting folders is expected behavior for many users and I have already seen countless examples where users were confused about what to open and where it is.

@btzy
Copy link

btzy commented Sep 22, 2022

Given that SaveFile already supports current_folder, I think it's only reasonable that OpenFile does the same thing. This is probably also the way that gives the application developer the most flexibility in deciding which folder to show, whether it is some specific system or data directory, the directory containing some previously opened file, or something else.

@ebassi
Copy link
Collaborator

ebassi commented Feb 23, 2023

An alternative suggestion that does not need any new api, and will still address a number of related use cases: Make the portal backend remember the last folder, per-app.

This was implemented in the GNOME portal, incidentally. I wonder if we should hoist the implementation in xdg-desktop-portal, but that has some implications about storing settings.

@felinira
Copy link

felinira commented Feb 23, 2023

I don't really understand what opening the file chooser with the last selected folder has to do with this. Those features are orthogonal to each other and have very different use cases and reasons to exist.

To me at least it is very clear that there are multiple apps that would benefit from such a feature. I also don't see any obvious security implications. So I don't really understand what's missing here. Can someone explain?

@redtide
Copy link

redtide commented Mar 10, 2023

I don't really understand what opening the file chooser with the last selected folder has to do with this. Those features are orthogonal to each other and have very different use cases and reasons to exist.

Yes, please forget the last used, just suggest a path like in SaveFile*.

Also in other platforms are used to set the directory from where to open a file, and this should be done from the app, which it will take care of whatever path it is and tell the filechooser to select it, last used or else.

* I noticed that in SaveFile, selecting an inexistent directory, it selects its path anyway. Now IDK if this is part of the specification or just an implementation exception. I think this shouldn't happen with OpenFile.

@Nantris
Copy link

Nantris commented Mar 27, 2023

This would be a really big help to have available for OpenFile. Is there any chance that might happen?

@GeorgesStavracas @alexlarsson can you tell who would need to review this? Is there anything the community can do to help get the ball rolling on this?

btzy added a commit to btzy/nativefiledialog-extended that referenced this pull request Apr 13, 2023
Portals are now fairly well-tested by a variety of users of NFDe.

A link is now also added to flatpak/xdg-desktop-portal#874 that adds support for setting a default folder in the OpenFile() method.
@ebassi
Copy link
Collaborator

ebassi commented May 4, 2023

See also: #1007, which tries to convert paths known to the document storage.

@ebassi
Copy link
Collaborator

ebassi commented May 4, 2023

Considering that SaveFile is already set up to do this, I think we should merge this pull request; though adding the same mechanism that PR #1007 uses to map paths passed to the portal to paths in the document storage would probably be a good idea as well. Maybe we want a separate PR for that, though.

@Nantris
Copy link

Nantris commented May 4, 2023

Thanks for looking into this @ebassi! As a Kubuntu user this is my #1 wishlist item.

<listitem>
<para>
Suggested folder to select the files from. The byte array is
expected to be null-terminated.
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation would be a lot more useful if it stated what filesystem view will be used to resolve this path - is it a host path ? Or something visible inside the sandbox?

It would also be good to clarify 'suggested' a bit more. I am reading it to mean:

  • portal implementations may ignore this
  • this is not meant to limit the users freedom to provide whatever file he chooses (e.g. it is fine for him/her to navigate to another folder before making a selection)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, aren't byte arrays always null-terminated?

Copy link

@btzy btzy May 18, 2023

Choose a reason for hiding this comment

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

Also, aren't byte arrays always null-terminated?

I don't think D-Bus requires byte arrays to be null-terminated, just as integer arrays aren't required to end with a zero.

Only strings are required to be null-terminated, but a byte array isn't a string as far as D-Bus is concerned.

Copy link

@btzy btzy May 18, 2023

Choose a reason for hiding this comment

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

Though, I don't know why we want to require null termination of the byte arrays. The null at the end of a string is not considered to be part of the string itself, so similar logic dictates that byte arrays should not semantically include a trailing null byte, if any. (Whether or not the marshalling format will place a null at the end of the byte array is a different matter, but I don't think D-Bus users should rely on that.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation would be a lot more useful if it stated what filesystem view will be used to resolve this path - is it a host path ? Or something visible inside the sandbox?

I agree that to make this interface useful, the documentation needs to answer this question.

It would also be good to clarify 'suggested' a bit more

I agree.

Also, aren't byte arrays always null-terminated?

D-Bus doesn't require byte arrays to be zero-terminated. A byte array can be any arbitrary blob of bytes, similar to any other integer array but with 1-byte elements; it's like a struct { unsigned char *location, size_t length } in C.

As a higher-level, application-layer thing, an interface can define specific semantics for a byte array, like "it contains the bytes of a valid JPEG". In this particular case, the interface is using the convention for representing "bytestrings" (conceptually a string in an arbitrary ASCII superset, similar to (type filename) in GObject-Introspection), which is to encode them in the D-Bus message as a byte array with length strlen(str) + 1, containing the strlen(str) non-zero bytes of the string, followed by exactly one byte with value zero.

In the D-Bus Specification, the LinuxSecurityLabel is currently the only standardized bytestring, and is described as "the non-zero bytes of [whatever] in an unspecified ASCII-compatible encoding, followed by a single zero byte". I think that's a good way to phrase it.

"Null-terminated" is a somewhat misleading term for this, because this is byte value 0 (ASCII mnemonic NUL, not a NULL pointer.

It's correct to use the bytestring convention for Unix filenames, environment variable names/values, and any similar string-like thing that (unfortunately) cannot be guaranteed to be UTF-8 (and might instead be a legacy national character set like Latin-1).

Whether or not the marshalling format will place a null at the end of the byte array is a different matter

The underlying marshalling format does not guarantee that there will be a zero after the end of a byte array, and also does not guarantee that there will not be a zero in the middle of the byte array. The higher-level (application-level) bytestring convention is that there is exactly one zero in the length-counted content of the array, as the last item.

Copy link
Collaborator

@smcv smcv May 19, 2023

Choose a reason for hiding this comment

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

The null at the end of a string is not considered to be part of the string itself, so similar logic dictates that byte arrays should not semantically include a trailing null byte, if any

If we were optimizing for theoretical correctness over pragmatism, then maybe; but D-Bus is designed to be reasonably efficient and reasonably easy to bind into common programming languages. The bytestring convention is that the length-counted byte array does include the trailing zero, because that means C/C++ bindings can read the bytestring directly out of the message payload without copying, like they can for the (UTF-8) D-Bus string type (for which the marshalling format does guarantee to put a zero immediately after the semantic content of the string).

You can think of it as "it's semantically a byte array containing a serialized data structure that we have chosen, and the serialized data structure we have chosen is a zero-terminated bytestring" if that makes you happer.

@Sodivad
Copy link
Contributor

Sodivad commented May 22, 2023

Support in Qt and KDE portal

https://codereview.qt-project.org/c/qt/qtbase/+/478997
https://invent.kde.org/plasma/xdg-desktop-portal-kde/-/merge_requests/199

@ebassi
Copy link
Collaborator

ebassi commented Jun 29, 2023

I opened #1045 which contains:

  • documentation updates
  • mapping from document storage to host paths

@ebassi
Copy link
Collaborator

ebassi commented Jun 30, 2023

PR #1045 was merged, so we can close this one.

Thanks to @sophie-h for the work, and thanks everyone for the reviews.

@ebassi ebassi closed this Jun 30, 2023
gnomesysadmins pushed a commit to GNOME/xdg-desktop-portal-gnome that referenced this pull request Jun 30, 2023
gnomesysadmins pushed a commit to GNOME/xdg-desktop-portal-gnome that referenced this pull request Jul 4, 2023
smcv pushed a commit to flatpak/xdg-desktop-portal-gtk that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileChooser: Add current_folder option to OpenFile
9 participants