GNOME Bugzilla – Bug 768669
Add a permission dialog system modal
Last modified: 2016-07-20 15:15:06 UTC
We want to show a number of portal dialogs as system modals: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/shell/access-control/wires-access-dialogs.png I've created a fallback gtk implementation for this dialog here: https://github.com/flatpak/xdg-desktop-portal-gtk/commit/b9a8b7157eefbd3097a4e14122e9449e661ddbc9 It implements this api: https://github.com/flatpak/xdg-desktop-portal/blob/a6513a30739fe30219c5bd85be37f9ccb1dc62f0/data/org.freedesktop.impl.portal.Access.xml The dialog is purely the ui part, all the portal logic (providing text/icons for use cases, dealing with permissions, etc) will be handled by the portal frontend. It would be great if gnome-shell could provide us a similar (or even identical) d-bus interface for system modals.
As mentioned on IRC to Allan, I don't think that we want per-device access requests. The application should probably have access to the device types, eg. you can easily switch between internal microphone and Bluetooth headset microphone, or front and rear cameras, without another access request. So no radio buttons needed to start with ;)
(In reply to Bastien Nocera from comment #1) > So no radio buttons needed to start with ;) Good, we don't have those in the shell :-) (In reply to Matthias Clasen from comment #0) > It implements this api: > > https://github.com/flatpak/xdg-desktop-portal/blob/ > a6513a30739fe30219c5bd85be37f9ccb1dc62f0/data/org.freedesktop.impl.portal. > Access.xml I've done an initial implementation (without choices), and noticed one oddity in that API: The interface provides a single 'AccessDialog' method that returns the user's response. That means that unless there's an error, the time the message invocation takes depends on a user action - in other words, by the time the user clicks "Grant Access", the method call may already have timed out and the access denied (in an completely non-obvious way). That is, unless the caller disables the timeout (e.g. via g_dbus_proxy_set_default_timeout(..., G_MAXINT)) - can we assume that this is the case, or should we tweak the API a bit?
(In reply to Florian Müllner from comment #2) > > I've done an initial implementation (without choices), and noticed one > oddity in that API: > The interface provides a single 'AccessDialog' method that returns the > user's response. That means that unless there's an error, the time the > message invocation takes depends on a user action - in other words, by the > time the user clicks "Grant Access", the method call may already have timed > out and the access denied (in an completely non-obvious way). That is, > unless the caller disables the timeout (e.g. via > g_dbus_proxy_set_default_timeout(..., G_MAXINT)) - can we assume that this > is the case, or should we tweak the API a bit? The way the portal frontend api works in general is as follows: App makes a portal request which involves user interaction Portal respond right away with the object path for a request object Portal shows dialog, and does whatever else it needs When the user interaction is done, the portal sends a Response signal directly to the app For portal backends, we've gone a different route, and instead make a single long method call. And yes, the portal frontend is the expected disable timeouts. The portal frontend is the only caller ever calling
OK, sounds good then. After writing the comment yesterday, I added support for checkboxes and updated the style to something that I hope doesn't make the designers cringe immediately. So all I'm still missing now is a proper commit message :-)
Created attachment 331408 [details] [review] Provide org.freedesktop.impl.portal.access implementation If a sandboxed app requests access to some system resource (camera, microphone, location), the portal frontend needs to ask the user for permission. In GNOME, we want this to be a system modal dialog, so provide an org.freedesktop.impl.portal.access implementation that exposes a generic system modal permission dialog on the bus.
Created attachment 331409 [details] [review] Provide org.freedesktop.impl.portal.access implementation Corresponding gnome-shell-sass commit.
Created attachment 331600 [details] [review] Provide org.freedesktop.impl.portal.access implementation Minor tweak to only send the reply after the close animation has finished and we are ready to handle another request.
(In reply to Bastien Nocera from comment #1) > As mentioned on IRC to Allan, I don't think that we want per-device access > requests. The application should probably have access to the device types, > eg. you can easily switch between internal microphone and Bluetooth headset > microphone, or front and rear cameras, without another access request. > > So no radio buttons needed to start with ;) I've updated the mockups to reflect this. Let me know if they're OK.
Created attachment 331605 [details] Test dialogs I've adjusted the options in my test script to match the new mockups. It's useful if you want to test the patch, so I'm attaching it.
I've now made xdg-desktop-portal load .portal files in alphabetic order, and prefer the first match, so gnome.portal should be preferred over gtk.portal, when both are present and the desktop is gnome.
Review of attachment 331600 [details] [review]: ::: js/ui/accessDialog.js @@ +11,3 @@ +const RequestIface = '<node> \ +<interface name="org.freedesktop.impl.portal.Request"> \ +<method name="Close"/> \ we don't need to implement the Response signal ? @@ +41,3 @@ + Extends: ModalDialog.ModalDialog, + + _init: function(invocation, handle, appId, title, subtitle, body, options) { appId is unused, did you plan to do something with it here? @@ +179,3 @@ + invocation.return_error_literal(Gio.DBusError, + Gio.DBusError.LIMITS_EXCEEDED, + 'Already showing a system access dialog'); is it really intended that these dialogs are session wide singletons? @@ +183,3 @@ + } + + let [handle, appId, parentWindow, title, subtitle, body, options] = params; I wonder if we should at least ensure that parentWindow is the focused window and perhaps also that it is at the top of the window stack?
(In reply to Rui Matos from comment #11) > we don't need to implement the Response signal ? The gtk fallback doesn't send it either, so I don't think we do - also see the explanation in comment #3. > @@ +41,3 @@ > + Extends: ModalDialog.ModalDialog, > + > + _init: function(invocation, handle, appId, title, subtitle, body, > options) { > > appId is unused, did you plan to do something with it here? Possibly check it against ShellWindowTracker:focus-app, though using the window ID is better long-term (see comment below). > @@ +179,3 @@ > is it really intended that these dialogs are session wide singletons? Not according to the API or gtk implementation, but system modal dialogs are intrusive, and stacking them does hardly improve on that. I don't think there's a legitimate case for opening more than one system modal at a time, in particular when we start enforcing that requests need to come from the focused app (i.e. multiple concurrent requests would then all come from the same app/window), so I'd like to keep enforcing that. > @@ +183,3 @@ > I wonder if we should at least ensure that parentWindow is the focused > window and perhaps also that it is at the top of the window stack? I think that makes sense, but we currently don't have an accessor for that - as far as I know, Jonas is working on window IDs on wayland, when that lands we'll have something we can expose. Or if you prefer, I can add that now for X11 and use it, and we can fill in the functionality on wayland later.
(In reply to Florian Müllner from comment #12) > Not according to the API or gtk implementation, but system modal dialogs are > intrusive, and stacking them does hardly improve on that. I don't think Right, not trying to imply that that would be a good idea :-) > there's a legitimate case for opening more than one system modal at a time, > in particular when we start enforcing that requests need to come from the > focused app (i.e. multiple concurrent requests would then all come from the > same app/window), so I'd like to keep enforcing that. I was thinking of attaching at most one of these dialogs to their requesting window. It would be a new style that we haven't used so far, so I just wanted to be sure that it wasn't the intention here. > > @@ +183,3 @@ > > I wonder if we should at least ensure that parentWindow is the focused > > window and perhaps also that it is at the top of the window stack? > > I think that makes sense, but we currently don't have an accessor for that - > as far as I know, Jonas is working on window IDs on wayland, when that lands > we'll have something we can expose. Or if you prefer, I can add that now for > X11 and use it, and we can fill in the functionality on wayland later. If you want to land the patch as is and then add that part later, adding a TODO comment wouldn't hurt.
Created attachment 331825 [details] [review] Provide org.freedesktop.impl.portal.access implementation - don't pass parameters from the dbus method to the dialog if not used - fail if we are passed an AppId, but it doesn't correspond to the currently focused app
Review of attachment 331825 [details] [review]: looks good ::: js/ui/accessDialog.js @@ +200,3 @@ + dialog.open(); + + dialog.connect('closed', Lang.bind(this, this._onDialogClosed)); could use the arrow syntax for this
Comment on attachment 331409 [details] [review] Provide org.freedesktop.impl.portal.access implementation Attachment 331409 [details] pushed as 7ab2789 - Provide org.freedesktop.impl.portal.access implementation
Attachment 331825 [details] pushed as 4fc0c51 - Provide org.freedesktop.impl.portal.access implementation