Created attachment 243808 [details] [review]
media-keys: Implement screencast shortcut

GNOME Shell gained a DBus interface for screencasting, use it to
take over handling of the screencast shortcut from the shell.

Review of attachment 243808 [details] [review]:

The file name was translatable in the shell implementation. Also the file extension was configurable in a gsettings key.

Are we deprecating the whole org.gnome.shell.recorder gsettings schema? I don't really have an opinion on if we should or should not. But if we are keeping it we should honor it.

To keep it simple we could special case a 0 length file_template parameter in the DBus API implementation and use the same translated file name template and settings that the old implementation used.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +838,3 @@
+
+static void
+toggle_screencast (GsdMediaKeysManager *manager) {

'{' on its own line here. I know, JS <-> C switching :-)

(In reply to comment #2)
> Review of attachment 243808 [details] [review]:
> 
> The file name was translatable in the shell implementation. Also the file
> extension was configurable in a gsettings key.
> 
> Are we deprecating the whole org.gnome.shell.recorder gsettings schema? I don't
> really have an opinion on if we should or should not. But if we are keeping it
> we should honor it.

We can't have gnome-settings-daemon depend on gnome-shell schemas, so the keys would need to be migrated if that were the case.

> To keep it simple we could special case a 0 length file_template parameter in
> the DBus API implementation and use the same translated file name template and
> settings that the old implementation used.

That sounds like unnecessary code duplication to me, and a nightmare to debug.

Review of attachment 243808 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +844,3 @@
+        g_return_if_fail (manager->priv->screencast_proxy != NULL);
+
+        if (manager->priv->recording) {

This won't work if g-s-d restarted while recording.

We also don't seem to try to monitor for the screencasting if it was stopped some other way (through the shell's tray for example?).

@@ +850,3 @@
+                method_name = "Screencast";
+                method_params = g_variant_new ("(sa{sv})",
+                                               "Screencast from %d %t.webm",

This needs to be translatable.

Also, why can't we pass a filename here? The code to find a free filename could be shared with the screenshot utilities.

(In reply to comment #2)
> The file name was translatable in the shell implementation.

Good point, we need to keep doing that.


> Also the file extension was configurable in a gsettings key.

It was configurable because the gstreamer pipeline was - it's not really useful to allow setting a file extension to e.g. "mp4" while still recording vp8. So if we want to migrate the setting to g-s-d, we should migrate both keys. Personally I think it's overkill here - now that the shortcuts are not the only way to invoke the recorder, a small gnome-screencast CLI tool looks like a better way to expose recorder options - but the decision is really up to Bastien.


> Are we deprecating the whole org.gnome.shell.recorder gsettings schema? I don't
> really have an opinion on if we should or should not. But if we are keeping it
> we should honor it.

Good point, I need to remove the schema - if we want recorder settings to be configurable, it should be done on the consumer side. Having settings to change the behavior of an API (unless callers explicitly overwrite it again!) is just confusing for both developers and users ...

(In reply to comment #5)
> (In reply to comment #2)
> > The file name was translatable in the shell implementation.
> 
> Good point, we need to keep doing that.
> 
> 
> > Also the file extension was configurable in a gsettings key.
> 
> It was configurable because the gstreamer pipeline was - it's not really useful
> to allow setting a file extension to e.g. "mp4" while still recording vp8. So
> if we want to migrate the setting to g-s-d, we should migrate both keys.
> Personally I think it's overkill here - now that the shortcuts are not the only
> way to invoke the recorder, a small gnome-screencast CLI tool looks like a
> better way to expose recorder options - but the decision is really up to
> Bastien.

We don't have configuration like this for the screenshots, so I'd rather not have it for screencasts.

(In reply to comment #4)
> +        g_return_if_fail (manager->priv->screencast_proxy != NULL);
> +
> +        if (manager->priv->recording) {
> 
> This won't work if g-s-d restarted while recording.

Recordings are tied to a DBus connection, e.g. if g-s-d goes away, an ongoing recording will be stopped automatically.


> We also don't seem to try to monitor for the screencasting if it was stopped
> some other way (through the shell's tray for example?).

At least for now, there's no other way to stop a recording - I can add a signal or something in case some alternative way is added in the future, but I think it's fine to only do that when/if it becomes necessary.


> @@ +850,3 @@
> +                method_name = "Screencast";
> +                method_params = g_variant_new ("(sa{sv})",
> +                                               "Screencast from %d %t.webm",
> 
> This needs to be translatable.

Yes, fixed locally.


> Also, why can't we pass a filename here? The code to find a free filename could
> be shared with the screenshot utilities.

There's no reason we can't pass a filename, the template used in the patch is simply the one we currently use ...

Thanks for the bug report. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find.

*** This bug has been marked as a duplicate of bug 704435 ***