RFC: Add desktop portal diagnostic interface #1669
Conversation
tytan652
force-pushed
the
diagnotic_iface
branch
from
d82d04a to
80f1d2f
Compare
tytan652
force-pushed
the
diagnotic_iface
branch
2 times, most recently
from
06cf532 to
95fdc07
Compare
tytan652
force-pushed
the
diagnotic_iface
branch
from
95fdc07 to
3f2475e
Compare
|
I honestly do not understand the purpose of this, and what problem it tries to solve. |
|
I guess you never met a dozen of users with portal issue without any proper way to help them. Because the whole point is to enable a way to know the state of xdp portals and create tools over it, even the DE itself right now can't know.
|
|
Okay, so this is for diagnostics from the outside; kind of missed that. I guess that makes sense, but just having a dbus service by itself isn't that useful. If there was a tool that we can ship to users which prints some stuff, I would be in. I'm also not entirely sure on the information that we would want to expose there, but I guess it's a good start. |
| @@ -74,6 +74,10 @@ background_monitor_sources = files( | |||
| 'org.freedesktop.background.Monitor.xml', | |||
| ) | |||
|
|
|||
| diagnostic_portal_sources = files( | |||
| 'org.freedesktop.diagnostic.portal.Desktop.xml', | |||
There was a problem hiding this comment.
We now have the org.freedesktop.host.portal namespace, might make sense to re-use that.
| Indicate if the version of the service is unstable. | ||
| Assume unstable if ``name``, ``version`` or ``stable`` is not present |
There was a problem hiding this comment.
It's xdp own version, not everyone is aware of "odd minor means unstable".
There was a problem hiding this comment.
I don't see why this needs to be encoded in the API though; especially because versioning schemes can change.
| Version of the implementation if the interface is versioned. | ||
| Assume 1 if not present | ||
| --> | ||
| <property name="lockdown-impl" type="a{sv}" access="read"> |
There was a problem hiding this comment.
Any reason why there are a bunch of properties for specific impls instead of exposing a single property with all the impls?
There was a problem hiding this comment.
Access and lockdown state affect various portals (and don't have non-impl counterpart).
One being missing will cause some portals to be missing.
There was a problem hiding this comment.
I'm aware of how that works. This doesn't answer the question. Why only expose some of the impls and not all?
There was a problem hiding this comment.
Because those are excluded from the portals property, since it only list impls meant to specific portal.
| The list of portal with details about their state: | ||
|
|
||
| * ``name``` (``s``) |
There was a problem hiding this comment.
From looking at the signature we have a tuple where the first element is probably the name, and the second element is probably the vardict documented below. So ``name`` implies this is a vardict, but it's not, it's just a string.
Could just make this an array of vardicts with name as one possible property.
There was a problem hiding this comment.
IIRC, I made it this way so you can just check for the key to know if the portal has an impl and not loop to find it (even if internally it may loop anyway).
There was a problem hiding this comment.
That doesn't seem very useful to me. Vardicts are also just arrays. Duplicating all the data here just seems unnecessary.
|
|
||
| Each portal can contain the following properties: | ||
|
|
||
| * ``implementation`` (``a{sv}``) |
There was a problem hiding this comment.
Could just provide the name of the impl and let the caller correlate that with the impl data from another property.
There was a problem hiding this comment.
It's made that way to avoid having to update the interface for every new portal, unless it's an impl like Access or Lockdown.
There was a problem hiding this comment.
You don't have to add a new property for every impl. Just one property that enumerates all the impls.
I thought about how to improve Door Knocker like:
--talk-name=org.freedesktop.Flatpakandflatpak-spawnbut add a--talk-nameto a specific interfaceAt the same time I tried to make sure that this interface is not meant only for my application but also could be used by desktop environment to have a good insight of the state of the portal service.
This interface is not meant to be accessible directly by Flatpak application.