Add GetRealPath to document portal #1269

JakobDev · 2024-01-24T09:02:21Z

This new function allows Apps to get the Real Path from a Document ID. IF you are using the FileChooser Portal, you get the a /run/user/1000/doc path. That's a Problem for Apps who display the Path in the UI e.g. a recent files menu.This new function allows Apps to get the real Path, so it be displayed probably in the UI.

Sandboxed Apps can only get the real path for those document IDs they have access to.

This PR is currently missing tests, as I don't know if this new function gets accepted. Please also note that I don't have much experience in C.

Fixes #475

document-portal/document-portal.c

grulja · 2024-01-24T10:09:06Z

Also, should there be some sanity checks, like only accept the path with the file:// scheme (or not?) and mention it in the docs? It's always passed like that in the FileChooser portal so we should make sure there is some consistency.

JakobDev · 2024-01-29T15:18:30Z

Also, should there be some sanity checks, like only accept the path with the file:// scheme (or not?)

You just pass the ID, not the path. If you path is e.g. /run/user/1000/doc/123 the ID is 123. It's the same usage as the Info function.

and mention it in the docs?

This should be generated automatically from the XML file.

grulja · 2024-01-29T15:23:14Z

Also, should there be some sanity checks, like only accept the path with the file:// scheme (or not?)

You just pass the ID, not the path. If you path is e.g. /run/user/1000/doc/123 the ID is 123. It's the same usage as the Info function.

I see, didn't realize that and it makes sense.

and mention it in the docs?

This should be generated automatically from the XML file.

I meant in case we were going to pass path with file:// suffix, that it should be mentioned in the docs, but as you pointed out it's just the document id and not the path. No action needed then.

document-portal/document-portal.c

data/org.freedesktop.portal.Documents.xml

JakobDev · 2024-02-14T11:39:59Z

I have now added a test

document-portal/document-portal.c

grulja · 2024-02-27T08:48:55Z

Can you please squash your commits into one?

data/org.freedesktop.portal.Documents.xml

swick · 2024-02-27T12:03:21Z

So, we are not going for extended file attributes then? Would be nice to have some rationale for why this path has been chosen. I'm worrying a bit about the performance here because every single file lookup will incur multiple dbus messages between 4 different processes.

grulja · 2024-02-27T13:06:50Z

So, we are not going for extended file attributes then? Would be nice to have some rationale for why this path has been chosen. I'm worrying a bit about the performance here because every single file lookup will incur multiple dbus messages between 4 different processes.

Maybe a stupid question as this is something I know nothing about. Wouldn't extended file attributes as proposed in the original bug be GIO specific?

Why multiple DBus messages between 4 different processes?

method call time=1709039124.206214 sender=:1.910 -> destination=:1.187 serial=61 path=/org/freedesktop/portal/documents; interface=org.freedesktop.portal.Documents; member=GetRealPath
   string "c37f0689"
method return time=1709039124.206323 sender=:1.187 -> destination=:1.910 serial=240 reply_serial=61
   string "/var/home/jgrulich/development"

swick · 2024-02-27T13:18:51Z

Maybe a stupid question as this is something I know nothing about. Wouldn't extended file attributes as proposed in the original bug be GIO specific?

No, extended file attributes are a standard linux feature. They are exposed in GIO via GFileAttribute currently, so this is only about how Glib/GIO would provide a nice API for it.

Why multiple DBus messages between 4 different processes?

App, xdg-dbus-proxy, dbus broker, portal.

grulja · 2024-02-27T13:44:35Z

Maybe a stupid question as this is something I know nothing about. Wouldn't extended file attributes as proposed in the original bug be GIO specific?

No, extended file attributes are a standard linux feature. They are exposed in GIO via GFileAttribute currently, so this is only about how Glib/GIO would provide a nice API for it.

It would still require all the other counterparts (e.g. Qt) to implement support for it, right? I also think this would be less discoverable, even though it's probably a better solution. Maybe this solution can be a temporary one? Most importantly because it would take way more time to get this available for clients. You would need this to land in GLib first in order to support it in xdg-desktop-portal and it would also require the most recent versions of Glib for clients to use it.

Why multiple DBus messages between 4 different processes?

App, xdg-dbus-proxy, dbus broker, portal.

That's 4 processes involved, but I think it's still one message for App → Portal and one message back for Portal → App. It's also not like this is going to be used frequently.

swick · 2024-02-27T14:58:42Z

One could write code to read the extended attribute just like they could write code to call the portal. There is no need to wait for GIO or anything else really. It would just be more convenient.

That being said, I'm not against this portal in principle.

JakobDev · 2024-02-27T15:02:14Z

The problem with file attributes is, that it is not very well supported. Qt for example don't seem to have native support for reading it. On most programming languages you'll need to find and add a additional library just for getting the correct file path. This would make xdg-desktop-portal an even more nightmare to use than it already is.

We could however support both: This Function and a file attribute,. But adding a file attribute is outside the scope of this PR.

grulja · 2024-02-27T19:11:43Z

One could write code to read the extended attribute just like they could write code to call the portal. There is no need to wait for GIO or anything else really. It would just be more convenient.

Right, but it's more low-level and not something clients will adopt as easily as simple DBus call.

We could however support both: This Function and a file attribute,. But adding a file attribute is outside the scope of this PR.

I agree, making this one as first option, while having file attributes as something to use in the future along this one.

Mikenux · 2024-02-28T01:42:25Z

As written (one doc ID), does this mean the app should send multiple requests for multiple files? If that's the case, wouldn't it be better to be able to request multiple document IDs at once so there is only one request instead of multiple ones? (also have only one response instead of multiple ones)

Also, is it possible to handle this with a file chooser option? (returning files and their paths at the same time, or is it the extended file attributes?)

grulja · 2024-02-28T09:39:32Z

As written (one doc ID), does this mean the app should send multiple requests for multiple files? If that's the case, wouldn't it be better to be able to request multiple document IDs at once so there is only one request instead of multiple ones? (also have only one response instead of multiple ones)

I think one will be the most common use-case, but I guess this PR can be changed to support more at once to be future-proof.

Also, is it possible to handle this with a file chooser option? (returning files and their paths at the same time, or is it the extended file attributes?)

This would not work in case when you are not opening the file first using file chooser portal. The path from the document portal can be already saved in an app as part of settings. In such case you just need to get the host path without going through file chooser portal first.

JakobDev · 2024-03-01T09:48:54Z

with a file chooser option

Most Apps don't call the file chooser portal directly. They just use the function the Toolkit provides and the Toolkit uses the Portal. So most Apps can't take advantage of such a Option.

but I guess this PR can be changed to support more at once

I could add a second function which takes an array. Many D_Bus libraries are not in a good state, so I think we should also provide a solution which just takes a string.

Mikenux · 2024-03-03T02:44:37Z

Most Apps don't call the file chooser portal directly. They just use the function the Toolkit provides and the Toolkit uses the Portal. So most Apps can't take advantage of such a Option.

Yes, but it is expected that the toolkits will implement the function (as you said, they already integrate portals).

grulja · 2024-03-05T10:05:19Z

but I guess this PR can be changed to support more at once

I could add a second function which takes an array. Many D_Bus libraries are not in a good state, so I think we should also provide a solution which just takes a string.

It wouldn't make sense to have two same calls, where one does the same thing just for more document IDs. It should not be a problem to make this accept an array of strings and return something like map<docId, hostPath> as a result.

That said, I believe most of the cases will be using this to translate just one path and that's even my use case so I don't really insist on this.

+1 from me on this MR, with just a suggestion to change the naming of that method

JakobDev · 2024-03-15T11:10:16Z

It should not be a problem to make this accept an array

As I said, there are many D-Bus Bindings out there who are not in a good state. Having a Method that only takes a string and not an Array would also be easier to use on the command line.

swick · 2024-03-15T11:36:16Z

We already have much more complicated method signatures and things work fine. This is really not a good argument at all.

JakobDev · 2024-03-22T09:38:29Z

and things work fine

Depends on what binding you use. I mainly program, in PyQt and QtDBus is completely broken. I need to use jeepney for most Portals. Adding a new dependency just to show the real path is something I want to avoid.

hfiguiere · 2024-03-22T13:47:17Z

If the bindings are the problem the bindings should be fixed instead. Otherwise you are just pushing for more technical debt.

grulja · 2024-04-25T08:43:26Z

@JakobDev are you going to update this change to take more document IDs? I can look into it myself if you don't have time to do so. I really want this change landed as soon as possible.

JakobDev · 2024-04-26T09:49:56Z

Sorry, I forgot this PR. I try to look into it next Week, but I can't promise I'll have time.

JakobDev · 2024-04-29T17:27:06Z

I have now updated the Code with help from @ebassi to use a Array. The test is not updated yet.

grulja · 2024-05-09T09:56:35Z

I think safer would be to return a map<doc_id, translated_path> to not rely on the order of document ids you pass. This way you can also skip those document ids that cannot be translated to the real path and just process and return those that can be.

grulja

I wrote some suggestions how I think this should be changed to correctly document the method and to make the method return map<string, bytearray>. I tested it and it works. I haven't tried to update the test.

grulja · 2024-05-20T09:57:18Z

data/org.freedesktop.portal.Documents.xml

@@ -273,5 +273,21 @@
 <arg type='s' name='app_id' direction='in'/>
 <arg type='a{say}' name='docs' direction='out'/>
 </method>
+
+ <!--
+ GetRealPath:


Suggested change

GetRealPath:

GetRealPaths:

grulja · 2024-05-20T09:57:35Z

data/org.freedesktop.portal.Documents.xml

+
+ <!--
+ GetRealPath:
+ @doc_id: the ID of the file in the document store


Suggested change

@doc_id: the ID of the file in the document store

@doc_id: the IDs of the files in the document store

grulja · 2024-05-20T09:57:54Z

data/org.freedesktop.portal.Documents.xml

+ <!--
+ GetRealPath:
+ @doc_id: the ID of the file in the document store
+ @path: the path for the file in the host filesystem


Suggested change

@path: the path for the file in the host filesystem

@path: a dictionary mapping application IDs to the paths in the host filesystem

grulja · 2024-05-20T09:58:06Z

data/org.freedesktop.portal.Documents.xml

+ @doc_id: the ID of the file in the document store
+ @path: the path for the file in the host filesystem
+
+ Gets the filesystem path for a document store entry.


Suggested change

Gets the filesystem path for a document store entry.

Gets the filesystem paths for document store entries.

grulja · 2024-05-20T09:58:18Z

data/org.freedesktop.portal.Documents.xml

+
+ This method was added in version 5 of this interface.
+ -->
+ <method name="GetRealPath">


Suggested change

<method name="GetRealPath">

<method name="GetRealPaths">

grulja · 2024-05-20T09:58:57Z

document-portal/document-portal.c

+}
+
+static gboolean
+portal_get_real_path (GDBusMethodInvocation *invocation,


Suggested change

portal_get_real_path (GDBusMethodInvocation *invocation,

portal_get_real_paths (GDBusMethodInvocation *invocation,

grulja · 2024-05-20T09:59:15Z

document-portal/document-portal.c

@@ -1445,6 +1529,7 @@ on_bus_acquired (GDBusConnection *connection,
 g_signal_connect_swapped (dbus_api, "handle-lookup", G_CALLBACK (handle_method), portal_lookup);
 g_signal_connect_swapped (dbus_api, "handle-info", G_CALLBACK (handle_method), portal_info);
 g_signal_connect_swapped (dbus_api, "handle-list", G_CALLBACK (handle_method), portal_list);
+ g_signal_connect_swapped (dbus_api, "handle-get-real-path", G_CALLBACK (handle_method), portal_get_real_path);


Suggested change

g_signal_connect_swapped (dbus_api, "handle-get-real-path", G_CALLBACK (handle_method), portal_get_real_path);

g_signal_connect_swapped (dbus_api, "handle-get-real-paths", G_CALLBACK (handle_method), portal_get_real_paths);

grulja · 2024-05-20T09:59:35Z

document-portal/document-portal.c

+ g_variant_builder_add (&builder, "^ay", path);
+ }
+
+ g_dbus_method_invocation_return_value (invocation, g_variant_new ("(aay)", &builder));


Suggested change

g_dbus_method_invocation_return_value (invocation, g_variant_new ("(aay)", &builder));

g_dbus_method_invocation_return_value (invocation, g_variant_new ("(@a{say})", g_variant_builder_end(&builder)));

grulja · 2024-05-20T09:59:46Z

document-portal/document-portal.c

+ return FALSE;
+ }
+
+ g_variant_builder_add (&builder, "^ay", path);


Suggested change

g_variant_builder_add (&builder, "^ay", path);

g_variant_builder_add (&builder, "{s@ay}", id_list[i], g_variant_new_bytestring(path));

grulja · 2024-05-20T10:00:43Z

document-portal/document-portal.c

+
+ g_variant_get (parameters, "(^a&s)", &id_list);
+
+ GVariantBuilder builder = G_VARIANT_BUILDER_INIT (G_VARIANT_TYPE ("aay"));


Suggested change

GVariantBuilder builder = G_VARIANT_BUILDER_INIT (G_VARIANT_TYPE ("aay"));

GVariantBuilder builder = G_VARIANT_BUILDER_INIT (G_VARIANT_TYPE ("a{say}"));

JakobDev · 2024-05-27T08:49:09Z

return map<string, bytearray>.

You can implemnt that if you want. I ave little to no motivation in messing with glib further. The last one already took a few hours.

grulja · 2024-05-27T08:55:03Z

return map<string, bytearray>.

You can implemnt that if you want. I ave little to no motivation in messing with glib further. The last one already took a few hours.

I can take over if you don't mind. I wrote suggestions for the changes, but it still requires to finish the tests.

grulja · 2024-05-27T13:54:58Z

I have submitted #1364 to continue with the changes I suggested here.

Add GetRealPath to document portal

6dab11a

grulja reviewed Jan 24, 2024

View reviewed changes

document-portal/document-portal.c Outdated Show resolved Hide resolved

Remove static

dfe03e6

sonnyp mentioned this pull request Feb 8, 2024

Sandbox the application tchx84/Gameeky#66

Open

grulja reviewed Feb 12, 2024

View reviewed changes

document-portal/document-portal.c Outdated Show resolved Hide resolved

data/org.freedesktop.portal.Documents.xml Show resolved Hide resolved

JakobDev added 5 commits February 13, 2024 14:49

Do requested changes

64ed5b0

Fix typo

1794b06

try to fix test

6821356

Add version to docs

797ea01

Add test

7aab70f

grulja reviewed Feb 20, 2024

View reviewed changes

document-portal/document-portal.c Outdated Show resolved Hide resolved

JakobDev added 3 commits February 21, 2024 08:18

Remove g_autofree

8a4e19e

Update test

341f35c

Another fix

1dd61b9

grulja reviewed Feb 27, 2024

View reviewed changes

data/org.freedesktop.portal.Documents.xml Show resolved Hide resolved

JakobDev added 2 commits April 29, 2024 15:37

Merge branch 'main' into getrealpath

5ad6f8f

Use array

a1a7c8d

grulja reviewed May 20, 2024

View reviewed changes

Mikenux mentioned this pull request May 25, 2024

[Feature request]: A filesystem access permission for the user's home folder excluding dotfiles flatpak/flatpak#4632

Open

2 tasks

grulja closed this Jun 10, 2024

JakobDev deleted the getrealpath branch June 10, 2024 09:22

	@doc_id: the ID of the file in the document store
	@doc_id: the IDs of the files in the document store

	@path: the path for the file in the host filesystem
	@path: a dictionary mapping application IDs to the paths in the host filesystem

	Gets the filesystem path for a document store entry.
	Gets the filesystem paths for document store entries.

	portal_get_real_path (GDBusMethodInvocation *invocation,
	portal_get_real_paths (GDBusMethodInvocation *invocation,

	g_signal_connect_swapped (dbus_api, "handle-get-real-path", G_CALLBACK (handle_method), portal_get_real_path);
	g_signal_connect_swapped (dbus_api, "handle-get-real-paths", G_CALLBACK (handle_method), portal_get_real_paths);

	g_dbus_method_invocation_return_value (invocation, g_variant_new ("(aay)", &builder));
	g_dbus_method_invocation_return_value (invocation, g_variant_new ("(@a{say})", g_variant_builder_end(&builder)));

	g_variant_builder_add (&builder, "^ay", path);
	g_variant_builder_add (&builder, "{s@ay}", id_list[i], g_variant_new_bytestring(path));


		g_variant_get (parameters, "(^a&s)", &id_list);

		GVariantBuilder builder = G_VARIANT_BUILDER_INIT (G_VARIANT_TYPE ("aay"));

Add GetRealPath to document portal #1269

Add GetRealPath to document portal #1269

Conversation

JakobDev commented Jan 24, 2024

grulja commented Jan 24, 2024 • edited

JakobDev commented Jan 29, 2024

grulja commented Jan 29, 2024

JakobDev commented Feb 14, 2024

grulja commented Feb 27, 2024

swick commented Feb 27, 2024

grulja commented Feb 27, 2024

swick commented Feb 27, 2024

grulja commented Feb 27, 2024

swick commented Feb 27, 2024

JakobDev commented Feb 27, 2024

grulja commented Feb 27, 2024

Mikenux commented Feb 28, 2024

grulja commented Feb 28, 2024

JakobDev commented Mar 1, 2024

Mikenux commented Mar 3, 2024

grulja commented Mar 5, 2024

JakobDev commented Mar 15, 2024

swick commented Mar 15, 2024

JakobDev commented Mar 22, 2024

hfiguiere commented Mar 22, 2024

grulja commented Apr 25, 2024

JakobDev commented Apr 26, 2024

JakobDev commented Apr 29, 2024

grulja commented May 9, 2024

grulja left a 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

grulja May 20, 2024 • edited

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

JakobDev commented May 27, 2024

grulja commented May 27, 2024

grulja commented May 27, 2024

grulja commented Jan 24, 2024 •

edited

grulja May 20, 2024 •

edited