Screenshot: Allow selecting the type of screenshot #1415
Conversation
leolost2605
force-pushed
the
screenshot-type
branch
from
22ecfd0
to
acf0f6f
Compare
src/screenshot.c
Outdated
| GVariant *options, | ||
| GError **error) | ||
| { | ||
| uint32_t type = g_variant_get_uint32 (value); |
There was a problem hiding this comment.
This returns a guint32. Please use that.
There was a problem hiding this comment.
We've actually to some degree been migrating away from glib types that have standard equivalent variants, like gint guint32, gint64 to int, uint32_t and int64_t, etc. They are for all intents and purposes equivalent, but there is no reason to avoid the standard ones. They also have a higher chance of resulting in looking more sensible in editors syntax highlighting.
There was a problem hiding this comment.
I noticed in the rest of the file the glib types are used so maybe keep that for consistency but you tell me what to use 🤷
There was a problem hiding this comment.
What we've been doing, as @jadahl mentioned, is that new code should use standard types (uint32_t) instead of GLib types, so I think this PR should use uint32_t etc
leolost2605
force-pushed
the
screenshot-type
branch
from
acf0f6f
to
3e993c1
Compare
This option allows to select the type (screen, window, area) of screenshot to be taken. Useful for apps that want to do the interactive parts themselves but still allow the user these different types of screenshots. This is only a hint, backends may not implement all of these.
leolost2605
force-pushed
the
screenshot-type
branch
from
3e993c1
to
1352f38
Compare
|
How is this supposed to work with non-interactive mode? I think the ability for apps to check which values are supported would be useful so they can gray out actions (e.g. menu items). |
It would take a screenshot of the requested kind right away and send it, allowing to take more sophisticated screenshots without having to go through additional UI, like the interactive dialog. All of that of course given that permission was granted by the user.
Yeah that would certainly be useful but AFAICT it would require some more severe API breakage since the best way I can think of would probably be a bit mask dbus property. Not sure how this should be handled/was handled in other cases but I'll look into it :) |
|
I took a look at previous issues regarding the addition of the automatic permission. It seems it has been added for having:
That makes one reason to not have selecting the type of screenshot for the automatic permission. Also:
|
There was a problem hiding this comment.
I think the current code is alright, but incomplete: usually for features like this, it's important that we expose to apps which screenshot types the system supports. This can be done by adding a new property AvailableModes to the Screenshot portal.
You can look at the ScreenCast portal's AvailableSourceTypes for a reference code
| static XdpOptionKey screenshot_options[] = { | ||
| { "modal", G_VARIANT_TYPE_BOOLEAN, NULL }, | ||
| { "interactive", G_VARIANT_TYPE_BOOLEAN, NULL } | ||
| { "interactive", G_VARIANT_TYPE_BOOLEAN, NULL }, | ||
| { "type", G_VARIANT_TYPE_UINT32, validate_type } |
There was a problem hiding this comment.
Nitpick:
| { "type", G_VARIANT_TYPE_UINT32, validate_type } | |
| { "type", G_VARIANT_TYPE_UINT32, validate_type }, |
| @@ -50,6 +50,17 @@ | |||
| Hint whether the dialog should offer customization before taking a screenshot. | |||
| Defaults to no. | |||
|
|
|||
| * ``type`` (``u``) | |||
There was a problem hiding this comment.
I think type is not very descriptive, because many parts of D-Bus and GLib and etc already use "type" for other things. When it comes to screenshots, I feel like mode fits better. What do you think?
|
Also missing tests |
This option allows to select the type (screen, window, area) of screenshot to be taken. Useful for apps that want to do the interactive parts themselves but still allow the user these different types of screenshots.
This is only a hint, backends may not implement all of these.
The use case I'm coming from is that at elementary we would like our screenshot app to have app actions for screen, window and area that bypass any UI (given that permission was granted previously) and just take the screenshot when launched.
pantheon (elementary OS) backend implementation: elementary/portals#105
If this receives some positive feedback I will do the GNOME implementation as well and libportal too I guess.
Fixes #1065