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

filechooser: Support current_folder with OpenFile #874

Closed
wants to merge 2 commits into from
Closed

filechooser: Support current_folder with OpenFile #874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9e70c7
filechooser: Support current_folder with OpenFile
sophie-h Aug 29, 2022
3ba1e11
Merge branch 'main' into wip/sophie-h/open-file-current-folder
sophie-h Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions data/org.freedesktop.impl.portal.FileChooser.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,12 @@
Whether the file is opened with write access. Default is no.
</para></listitem>
</varlistentry>
<varlistentry>
<term>current_folder ay</term>
<listitem><para>
A suggested folder to open the files from.
</para></listitem>
</varlistentry>
</variablelist>
-->
<method name="OpenFile">
Expand Down
9 changes: 9 additions & 0 deletions data/org.freedesktop.portal.FileChooser.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,15 @@
</para>
</listitem>
</varlistentry>
<varlistentry>
<term>current_folder ay</term>
<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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

</para>
</listitem>
</varlistentry>
</variablelist>
-->
<method name="SaveFile">
Expand Down
3 changes: 2 additions & 1 deletion src/file-chooser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,8 @@ static XdpOptionKey open_file_options[] = {
{ "directory", G_VARIANT_TYPE_BOOLEAN, NULL },
{ "filters", (const GVariantType *)"a(sa(us))", validate_filters },
{ "current_filter", (const GVariantType *)"(sa(us))", validate_current_filter },
{ "choices", (const GVariantType *)"a(ssa(ss)s)", validate_choices }
{ "choices", (const GVariantType *)"a(ssa(ss)s)", validate_choices },
{ "current_folder", G_VARIANT_TYPE_BYTESTRING, NULL }
};

static gboolean
Expand Down