Add Take Screenshot to the header context menu

[JoseExposito]

Fix #955

There are some design decisions that need to be taken, so invoking @cassidyjames here:

• We need to decide if we want to flash the window when the screenshot is taken or not
• The screenshot sound is not being played, but the screenshot app doesn't play it for window screenshots 🤔
• Maybe we should send a notification when the screenshot is taken indicating the path it is saved at. Extra points if there is a way to open Files when clicking on the notification
• Menu order:
  

And one quick technical question: Is there a way to share the code that sets the file name with the screenshots app? I'd like to move the generation of the string _("Screenshot from %s").printf (date_time); to the D-Bus interface and share that code.

[hanaral]

Extra extra points for actually implementing a preview of it as the icon and way to drag and drop from the notification

[JoseExposito]

Those are a lot of extra points xD I don't even know if that is possible to implement

[danirabbit]

It would probably be good to flash the screen and make sure we play the screenshot sound any time a screenshot is taken, but that probably doesn't need to be done in the scope of this branch.

The menu order is indeed an awful mess. I think that could be tackled in another branch too. We need some separators in there. It's ugly lol

I agree that we should send a notification after taking screenshots. There's another issue report here: #137

I think that could wait for another PR, but it would definitely be a big improvement. Likewise, DnD for images in screenshots would be an issue for the notifications server and it is filed here: elementary/notifications#83

If we did that, I think it would make sense to remove the custom save dialog from the screenshot app and always use the notification method to handle saved screenshots. That would implicitly de-duplicate that file name code as well ;)

[danirabbit]

Ah actually RE: the file name, when using keyboard shortcuts I think it's actually GNOME Settings Daemon that handles this file name. So I'm not sure if there's a good way to de-duplicate it in all cases

[danirabbit]

It looks like taking screenshots with the keyboard shortcuts does send sounds, so my guess is that this is implemented in GNOME Settings Daemon, which means we probably need to implement it here too and we may not be able to always send sounds or we'll get double sounds with some clients

[hanaral]

Just a heads up - the names the gnome-settings-daemon uses do not at all align with the Screenshot app's own naming scheme in different locales. I should probably open an issue for that.

[JoseExposito]

It would probably be good to flash the screen

Well, at the moment it flashes only the window, because it is what screenshot_window allows to do, but it can be changed if you don't like it.

make sure we play the screenshot sound any time a screenshot is taken, but that probably doesn't need to be done in the scope of this branch.

The sound is being played here:
https://github.com/elementary/screenshot/blob/master/src/ScreenshotBackend.vala#L158

Canberra is already a dependency of Gala, so, personally, I'd move that code to org.gnome.Shell.Screenshot as an extra method.
Adding it as an extra param to the screenshot_* methods (bool play_sound) will break the compatibility with GNOME's implementation, so I think it is safer to just add a new method.

The menu order is indeed an awful mess. I think that could be tackled in another branch too. We need some separators in there. It's ugly lol

+1 😆 If you have any ideas I can implement them in this PR, it should be pretty simple.

I agree that we should send a notification after taking screenshots. There's another issue report here: #137

Cool, I think that the same logic applies here. I'd add a method that wraps the notification sending so we don't end up with duplicated logic.

It looks like taking screenshots with the keyboard shortcuts does send sounds, so my guess is that this is implemented in GNOME Settings Daemon, which means we probably need to implement it here too and we may not be able to always send sounds or we'll get double sounds with some clients

I've been testing a little bit and it seems like the shortcuts that are copying images to clipboard are not playing sounds. However, shortcuts that are saving to file are plying it.
Do you know who is handling those shortcuts? GNOME settings daemon?

I see some references to clipboard in io.elementary.screenshot, but I'm not sure about how that works.

What do you think? I don't know what's the scope of io.elementary.screenshot. If it is mean to always run in a Pantheon sessions, it is safe to move some code to Gala. However, if the goal of the project is to also work on a GNOME environment, then we'll need to duplicate some code...

[danirabbit]

@JoseExposito Ah yes sorry, flash window I mean! I think that's working as intended :)

Yeah we're already breaking compatibility with the GNOME interface because of the with cursor and the conceal_text/redact options, so I don't think it would be that big of a deal to add a play_sound param. Realistically, there will be a screenshot portal, so I think this interface will only be consumed by the portal, and it looks like the portal accepts a vardict of params https://docs.flatpak.org/en/latest/portal-api-reference.html#gdbus-org.freedesktop.portal.Screenshot

I think it would probably be best not to let the scope of the PR get out of control. So it's probably fine the way it is already right now and we can come back in future PRs to do things like clean up the menu, add a sounds param, etc.

Yes that's correct, keyboard shortcut handling is done in GNOME Settings Daemon afaik

I think we are carrying maybe some code in Screenshot just to for example hide the conceal text option if it's not available in the server, but I don't think we really expect to cater to running elementary apps outside of elementary OS

[JoseExposito]

I think it would probably be best not to let the scope of the PR get out of control. So it's probably fine the way it is already right now and we can come back in future PRs to do things like clean up the menu, add a sounds param, etc.

Fair enough, we can always clean up a little bit duplicated code in the future. Feel free to merge this as it is then @danrabbit 😄

I'll do some testing with the portal as well as part of the clean up.

[danirabbit]

Does exactly what it says it does! We can follow up with some more enhancements in future branches :)