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.

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

Conversation

GeorgesStavracas
Copy link
Member

@GeorgesStavracas GeorgesStavracas commented Aug 5, 2022

This PR implements a new permission table and value for screenshots, which allows portal implementations to skip the sharing dialog.

Closes #649

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.
@GeorgesStavracas GeorgesStavracas force-pushed the gbsneto/non-interactive-screenshot-permission branch 2 times, most recently from 7244440 to 18bbfc1 Compare August 5, 2022 22:10
gnomesysadmins pushed a commit to GNOME/gnome-control-center that referenced this pull request Aug 5, 2022
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
@GeorgesStavracas
Copy link
Member Author

GNOME Settings can handle this permission now: https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/1412

Captura de tela de 2022-08-05 19-37-30

(Still WIP but it works)

@GeorgesStavracas GeorgesStavracas force-pushed the gbsneto/non-interactive-screenshot-permission branch from 18bbfc1 to 2473b74 Compare August 6, 2022 00:00
@GeorgesStavracas
Copy link
Member Author

GNOME portal implementation ready: https://gitlab.gnome.org/GNOME/xdg-desktop-portal-gnome/-/merge_requests/50
KDE doesn't seem to need this, they always skip the dialog for non-interactive requests anyway.

This somehow seems to be working, and that's highly suspicious:

2022-08-05.20-50-25.webm

@GeorgesStavracas GeorgesStavracas marked this pull request as ready for review August 6, 2022 00:06
@GeorgesStavracas GeorgesStavracas force-pushed the gbsneto/non-interactive-screenshot-permission branch from 2473b74 to 235e7a9 Compare August 6, 2022 12:31
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.
gnomesysadmins pushed a commit to GNOME/gnome-control-center that referenced this pull request Aug 6, 2022
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.
gnomesysadmins pushed a commit to GNOME/gnome-shell that referenced this pull request Aug 8, 2022
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
gnomesysadmins pushed a commit to GNOME/gnome-control-center that referenced this pull request Aug 8, 2022
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
@GeorgesStavracas
Copy link
Member Author

Let's go with this for now.

@GeorgesStavracas GeorgesStavracas merged commit 55f4234 into flatpak:main Aug 8, 2022
@GeorgesStavracas GeorgesStavracas deleted the gbsneto/non-interactive-screenshot-permission branch August 8, 2022 19:37
@@ -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.
Copy link
Collaborator

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?

Copy link
Member Author

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

@jadahl
Copy link
Collaborator

jadahl commented Aug 8, 2022

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".

@GeorgesStavracas
Copy link
Member Author

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.

@jadahl
Copy link
Collaborator

jadahl commented Aug 8, 2022

I didn't see any asking for reviews, and 3 days is extremely short for API changes like this.

@GeorgesStavracas
Copy link
Member Author

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.

@jadahl
Copy link
Collaborator

jadahl commented Aug 8, 2022

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.

@GeorgesStavracas
Copy link
Member Author

As you wish, sir: #853

gnomesysadmins pushed a commit to GNOME/gnome-control-center that referenced this pull request Aug 8, 2022
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
gnomesysadmins pushed a commit to GNOME/gnome-control-center that referenced this pull request Aug 10, 2022
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
gnomesysadmins pushed a commit to GNOME/gnome-control-center that referenced this pull request Aug 10, 2022
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
@All3xJ
Copy link

All3xJ commented Nov 2, 2022

Love this! Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screenshot portal without prompt
4 participants