-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
filechooser: Support current_folder with OpenFile #874
Conversation
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. |
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' |
I mean this is not any different from the
That's not really true. For example, 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. |
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 |
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. |
Given that |
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. |
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? |
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. |
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? |
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.
See also: #1007, which tries to convert paths known to the document storage. |
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. |
<listitem> | ||
<para> | ||
Suggested folder to select the files from. The byte array is | ||
expected to be null-terminated. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I opened #1045 which contains:
|
current_folder
option for OpenFileCloses #796