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 portal #94

Merged
merged 16 commits into from
Apr 24, 2024
Merged

Screenshot portal #94

merged 16 commits into from
Apr 24, 2024

Conversation

davidmhewitt
Copy link
Member

@davidmhewitt davidmhewitt commented Dec 11, 2023

Fixes elementary/gala#1807
Fixes elementary/screenshot#140

Implements the Screenshot and Colour Picking portals.

This mostly copies the Screenshot dialog code from the Screenshot app (and ports it to GTK4). But now it runs in the portal process, because the Screenshot app is packaged as a Flatpak itself, so it's not appropriate to have the portal there. There is an additional dialog that prompts the user if they want to share the taken Screenshot with the requesting application.

@danirabbit Could I get you to look at the UX of this and hack on it a bit if you like?

Can use ASHPD to test.

Still to do:

  • Make conceal text feature work
  • Remember previously selected screenshot options?
  • Think about what we do with the actual screenshot app now that this is implemented here.
  • Think about where/how these temporary screenshots are stored

@danirabbit
Copy link
Member

Yeah I kinda wish there was a way for the app to communicate some kind of intent over the portal so we select a good screenshot option. I'm honestly not sure what are the kinds of use cases for the screenshot portal, otherwise maybe it would be easier to know what a good default is here. For now, I think we can forgo saving options and then see if it comes up in practical use

Re: Duplicating the screenshot app, yeah it would probably be nice to figure out a way to not completely dupe this whole UI 😅. There's a number of issues/discussions around changing the way the save workflow happens in Screenshot so might be low priority until that's more figured out?

I was trying to make the previews/dialog resizable but kept running into weird issues with how Gtk is allocating space for the widget. There's a new content sizing property in newer Gtk, so might just be something to revisit once we have installable Noble builds to hack from.

@Marukesu
Copy link
Contributor

Make the approval dialog look nicer?

Assuming that we won't make another release for OS 7, i don't think we need the approval dialog at all. The portal fronted will check and ask for permission before calling us already.

Think about where/how these temporary screenshots are stored

i would say that portal screenshots should be saved in $XDG_RUNTIME_DIR, it will be sent to the application as read-only and the application will be the one defining the actual final location.

Think about what we do with the actual screenshot app now that this is implemented here.

i think we should remove the flatpak. AFAIK, it don't work outside of pantheon since GNOME closed they screenshot interface. but the app still has they use for "showing a save dialog".

@davidmhewitt
Copy link
Member Author

Just a note here that I looked at where GNOME's portal saves screenshots and it doesn't store them in a temporary location.

It treats them as actual screenshots and saves them to the screenshots folder, which I think makes sense too.

@danirabbit
Copy link
Member

I confirmed that the flow in OS 8 is that you get prompted from the request portal, if you disallow screenshot taking apps cannot take a screenshot in non-interactive mode. It doesn't even send the approval dialog. Otherwise the behavior for interactive mode is always allow, which makes sense because by using the screenshot dialog that's an implicit approval

So removed the dialog because it's never called

@danirabbit
Copy link
Member

Proposed elementary/icons#1249 to fix missing icon in the request dialog

@danirabbit
Copy link
Member

Opened elementary/switchboard-plug-applications#220 to address the setting we need after this is merged

@danirabbit
Copy link
Member

@davidmhewitt is there anything still blocking this? Would be great to get this in :)

@davidmhewitt
Copy link
Member Author

I think I'd like to change this from using /tmp to either the user's actual screenshot folder, or at least a user specific temporary folder.

I'm not sure when I'll next be at a PC that's set up enough to make and test those changes though.

@danirabbit
Copy link
Member

@davidmhewitt @Marukesu since merging that cache dir branch, any opposition to getting this in?

@davidmhewitt
Copy link
Member Author

No opposition, I think it's good to go. But I haven't looked at this or tested in a long while.

@danirabbit danirabbit marked this pull request as ready for review April 24, 2024 00:49
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I touched this code :p but it is working completely as expected afaict

@danirabbit danirabbit enabled auto-merge (squash) April 24, 2024 22:37
@danirabbit danirabbit disabled auto-merge April 24, 2024 22:37
@danirabbit danirabbit enabled auto-merge (squash) April 24, 2024 22:38
@danirabbit danirabbit merged commit 8ba0fe1 into main Apr 24, 2024
4 checks passed
@danirabbit danirabbit deleted the screenshot-portal branch April 24, 2024 22:38
@danirabbit danirabbit mentioned this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Color Picker portal Implement Screenshot portal
3 participants