-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
Screenshot permission #851
Screenshot permission #851
Conversation
No functional changes, but will facilitate future patches.
Next commit will introduce a permission query for screenshots, so prepare for that by moving some of the code to a thread job.
This will be used by the Screenshot call to query the permissions of the application for screenshotting.
7244440
to
18bbfc1
Compare
Add support for managing the screenshot permission, used by apps who request non-interactive screenshots from the portal. See also flatpak/xdg-desktop-portal#851
GNOME Settings can handle this permission now: https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/1412 (Still WIP but it works) |
18bbfc1
to
2473b74
Compare
GNOME portal implementation ready: https://gitlab.gnome.org/GNOME/xdg-desktop-portal-gnome/-/merge_requests/50 This somehow seems to be working, and that's highly suspicious: 2022-08-05.20-50-25.webm |
2473b74
to
235e7a9
Compare
This commits makes the screenshot portal request a permission using the the access portal when a non-interactive screenshot is requested. This should be enough to let applications request permissions once, and be forever able to screenshot without much hassle. Portal implementations will need to be adapted to this change, even though no actual D-Bus API change is introduced. Basically they'll have to skip showing any kind of dialog if the screenshot portal version is equal or greater than 3. Fixes flatpak#649
Trivial, but helps debugging it.
Noticed while reading reference code for the screenshot changes. It's good hygiene to protect against this failure.
Add support for managing the screenshot permission, used by apps who request non-interactive screenshots from the portal. See also flatpak/xdg-desktop-portal#851
Implementations might want to fine tune the behavior of the access dialog based on what permission is being requested, but they don't currently know that. The screenshot portal, for example, is special because applications might want to hide themselves before taking a screenshot, in which case implementations of the access portal should skip any validation regarding the focused window.
Screenshot is a special case compared to other permissions because apps might want to hide themselves from the desktop when a screenshot is about to be taken. In that case, this heuristic of checking if the focus window corresponds to the application that is requesting screenshot permissions becomes problematic. Special case the screenshot permission to skip the focused window check. See also: flatpak/xdg-desktop-portal#851
Add support for managing the screenshot permission, used by apps who request non-interactive screenshots from the portal. See also flatpak/xdg-desktop-portal#851
Let's go with this for now. |
@@ -29,7 +29,7 @@ | |||
via the document portal, and the returned URI will point | |||
into the document portal fuse filesystem in /run/user/$UID/doc/. | |||
|
|||
This documentation describes version 2 of this interface. | |||
This documentation describes version 3 of this interface. |
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.
What's the purpose of this version bump?
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.
It's documented in the commit message:
Portal implementations will need to be adapted to this change,
even though no actual D-Bus API change is introduced. Basically
they'll have to skip showing any kind of dialog if the screenshot
portal version is equal or greater than 3.
In other words, even if the D-Bus API is intact, v3 of the portal asks for the screenshot
permissions, v2 doesn't, and implementations need to know this to act
A bit unfortunate that a pull request with API changes affecting all potential portal backends only get 3 days, 2 which are on the weekend for review, before being landed without any "lgtm". |
It is indeed unfortunate, but as you can see, when I'm given the silence treatment after asking for reviews, landing pull requests immediately make people act. |
I didn't see any asking for reviews, and 3 days is extremely short for API changes like this. |
This is not in any release yet, so post-merge review is also an option. If you think the commits here are not satisfactory, they can be reverted. |
That is usually not how API review or even code review is supposed to work. Disclaimer: I'm happy to see the issue being worked on, I only object to how it has been fast tracked without letting anyone have a fair chance to review the changes before they landed. |
As you wish, sir: #853 |
Add support for managing the screenshot permission, used by apps who request non-interactive screenshots from the portal. See also flatpak/xdg-desktop-portal#851
Add support for managing the screenshot permission, used by apps who request non-interactive screenshots from the portal. See also flatpak/xdg-desktop-portal#851
Add support for managing the screenshot permission, used by apps who request non-interactive screenshots from the portal. See also flatpak/xdg-desktop-portal#851
Love this! Thanks!! |
This PR implements a new permission table and value for screenshots, which allows portal implementations to skip the sharing dialog.
Closes #649