GNOME Bugzilla – Bug 681844
Take screenshots directly instead of spawning gnome-screenshot
Last modified: 2014-05-05 23:43:26 UTC
If my understanding is correct, it is currently necessary to have GNOME Screenshot installed if you want to take screenshots. This doesn't seem quite right to me. Screenshots seem like functionality that should be provided by the system rather than an application. All that should be required is a press of the PrintScreen key: a separate application shouldn't be necessary. It would be really cool if GNOME 3 could provide screenshots without the need for a separate application. The GNOME Screenshot application could then become an optional addition.
All that *is* already required is a press of the PrintScreen key. :) What's the advantage for users of merging it into anything else? Maybe I miss your point...
I agree with the main idea here; if you install e.g. an ostree snapshot (which includes gnome-shell and gnome-settings-daemon, but not apps like gnome-screenshot), you should still be able to take screenshots. The approach I would like to take for this is moving the code that calls into the Shell screenshot methods (and the code that goes through X11 as a fallback path in case Shell is not available) into gnome-settings-daemon. g-s-d then would call into that code instead of spawning the gnome-screenshot binary in order to save the picture. This way, the gnome-screenshot app could as well continue to live unmodified, for people that want to use it from the command line or using the desktop file launcher. Bastien, does this sound good to you?
Don't want to add fallback code, sorry.
Created attachment 228428 [details] [review] media-keys: save screenshots directly Now that we decided to deprecate fallback mode completely, we can depend on the shell being around to take screenshots, without any X11 fallback paths. This allows g-s-d not to depend on gnome-screenshot anymore for the core feature, which will let gnome-screenshot free to evolve into a proper application.
Review of attachment 228428 [details] [review]: Looks good, apart from some problems with the existing screenshot code. ::: plugins/media-keys/gsd-screenshot-utils.c @@ +475,3 @@ + +static void +screenshot_select_area_async (SelectAreaCallback callback, All the select area code looks like it should leave in gnome-shell instead. It would avoid drawing artifacts or other bad interactions with other applications. @@ +677,3 @@ + } + + g_dbus_connection_call_sync (connection, I'd rather this was async. @@ +752,3 @@ + ctx->save_path = screenshot_build_tmp_path (); + + if (ctx->save_path) How can that happen? @@ +755,3 @@ + screenshot_take (ctx); + else + screenshot_build_filename_async (filename_ready_cb, ctx); It would be useful if one could pass a pattern to gnome-shell and have it save the file itself, to avoid races as we have now.
Created attachment 228520 [details] [review] media-keys: save screenshots directly -- Patch updated to address the review comments and use the proposed shell method to select an area. Still missing: moving support for file existance in the shell. I don't think parsing formats in the shell is worth it, but we could make it so that if a basename is passed to the shell method it automatically saves it into the default locations we want to use (it could be done separately in another round of patches though).
> Still missing: moving support for file existance in the shell. I don't think > parsing formats in the shell is worth it, but we could make it so that if a > basename is passed to the shell method it automatically saves it into the > default locations we want to use (it could be done separately in another round > of patches though). The only important thing is to remove the race condition that exists.
Review of attachment 228520 [details] [review]: Rest of the patch looks good (minus the file existence code). ::: plugins/media-keys/gsd-screenshot-utils.c @@ +317,3 @@ + goto done; + + ca_context_play_full (c, 0, p, NULL, NULL); Can you collapse all this in a single ca_context_play() call?
Created attachment 228600 [details] [review] media-keys: save screenshots directly -- Use ca_context_play()
Created attachment 228601 [details] [review] screenshot: don't check for filename existence ourselves Pass a basename to the Shell, it will automatically try to save it in the default locations, with duplicate checking. -- Depends on the shell patch from bug 688004.
Created attachment 231161 [details] [review] media-keys: save screenshots directly -- Update for latest shell patches
Created attachment 231162 [details] [review] screenshot: don't check for filename existence ourselves -- Update for latest shell patches
Review of attachment 231161 [details] [review]: Looks good otherwise. ::: plugins/media-keys/gsd-screenshot-utils.c @@ +291,3 @@ + g_mkdir_with_parents (path, 0700); + + tmpname = g_strdup_printf ("scr-%d.png", g_random_int ()); I don't think this is safe, I'd rather you used: g_file_open_tmp() for example, which will make sure that the filename is unique.
Created attachment 232642 [details] [review] media-keys: save screenshots directly -- Updated for review.
Review of attachment 232642 [details] [review]: Looks good otherwise. ::: plugins/media-keys/gsd-screenshot-utils.c @@ +267,3 @@ + gint fd; + + fd = g_file_open_tmp (NULL, &path, NULL); I'd rather you used a template here ("gnome-settings-daemon-screenshot-XXXXXX" for example).
Attachment 232642 [details] pushed as f33cd19 - media-keys: save screenshots directly Pushed with the suggested change.
f33cd19 caused a regression in that there's no way to choose where the screenshots are saved now: https://bugzilla.gnome.org/show_bug.cgi?id=729610