GNOME Bugzilla – Bug 735375
Does not come up when selected from Activities menu
Last modified: 2014-09-06 07:36:14 UTC
Gnome-screenshot does not come up when selected from the Activities menu. Works just fine if I run gnome-screenshot --interactive from the terminal though. Not sure how to debug this. It works OK in gdb too. Even selecting "new window" on the right click doesn't bring up the window. gnome-screenshot-3.13.90-1.fc21.x86_64 gnome-shell-3.13.90-1.fc21.x86_64
Hey Matthias, I think you broke this.... When an app is activated by DBus, then the Exec line in the desktop file is ignored. The --gapplication-service argument is incompatible with other arguments, so it's not a simple fix in the service file either.
Created attachment 285052 [details] [review] Fix launching from gnome-shell Instead of starting in headless mode, start in the interactive mode and bring up the UI when we're being DBus activated. To make this possible, this patch separates the GSettings loading and command line parsing, so we would only have to parse the command line when started on the command line, and skip this in the DBus activation case.
Created attachment 285053 [details] [review] Present the existing window when activated in UI mode
Created attachment 285054 [details] [review] Start command line instances in non-unique mode This makes it possible to run non-interactive command line instances in parallel to an existing UI instance. Activating the app from the .desktop file launcher or starting with --interactive still brings up the same, unique window.
Review of attachment 285054 [details] [review]: I don't think this works. Or even if it does, it is not the right way to do it.
Given the constraints of the current commandline API of gnome-screenshot, there is really only one way to do this properly: - create a TakeScreenshot action with a parameter field of some type - handle the commandline as so: - no args: invoke the TakeScreenshot action - --delay, etc.: invoke the action with the appropriate args - --interactive: Activate the application doing this would mean that further invocations (for example, while an interactive window was up or while we were in a delay) would forward the request to the primary instance for handling. Because of the global-state nature of gnome-screenshot, this would probably require some amount of rewriting. For that reason, the non-unique approach is maybe a good pragmatic solution for now. Another consideration: perhaps there exist scripts that rely on gnome-screenshot exiting in a predictable way, particularly if used with --delay. This is more argument towards either doing a full service/launcher split or using non-uniqueness for these cases. Another solution could be to do a bit of a different kind of split: keep the gnome-screenshot commandline tool as the "take a screenshot now" app and split the interactive bit into a separate process which would be structured more like a typical Gtk application. Modify the desktop file to use this other (normal) app as the Exec= line. Invoking the (existing) commandline tool with --interactive would then just be an alias for starting that app.
Review of attachment 285052 [details] [review]: Looks mostly good, some comments below. ::: src/screenshot-application.c @@ +690,2 @@ { + g_application_activate (app); I think the previous behavior was a little different here, in that the delay, include_border and border_effect command line parameters were reflected as new defaults in the interactive dialog too. Shouldn't be too hard to put this behavior back here. @@ +698,1 @@ + if (window_arg && area_arg) Can you split this parsing into a function like screenshot_config_parse_command_line()? @@ +728,3 @@ + screenshot_config->file = g_file_new_for_commandline_arg (file_arg); + + screenshot_config->include_border = !disable_border_arg; Not sure you need to repeat this code block here, as you already do it in screenshot_load_config()
Review of attachment 285053 [details] [review]: ::: src/screenshot-application.c @@ +749,3 @@ + window = screenshot_interactive_dialog_new ((CaptureClickedCallback) screenshot_start, self); + gtk_application_add_window (GTK_APPLICATION (g_application_get_default ()), + GTK_WINDOW (window)); Is this really needed? screenshot_interactive_dialog_new() calls gtk_application_window_new() passing g_application_get_default() already, and I expect that to eventually call into gtk_application_add_window() already...
Review of attachment 285054 [details] [review]: This looks OK until we get a more robust implementation like the one outlined by Ryan.
Created attachment 285440 [details] [review] Fix launching from gnome-shell Instead of starting in headless mode, start in the interactive mode and bring up the UI when we're being DBus activated. To make this possible, this patch separates the GSettings loading and command line parsing, so we would only have to parse the command line when started on the command line, and skip this in the DBus activation case.
Created attachment 285441 [details] [review] Present the existing window when activated in UI mode
Review of attachment 285440 [details] [review]: Looks good with one minor nitpick ::: src/screenshot-config.h @@ +61,3 @@ + const gchar *border_effect_arg, + guint delay_arg, + gboolean area_arg, Weird indentation :-)
Review of attachment 285441 [details] [review]: OK
Thanks for the help and reviews everyone! Attachment 285054 [details] pushed as cbdd3f5 - Start command line instances in non-unique mode Attachment 285440 [details] pushed as dbc2099 - Fix launching from gnome-shell Attachment 285441 [details] pushed as 71f3835 - Present the existing window when activated in UI mode
Thanks for the quick fix Kalev. gnome-screenshot-3.13.90-2.gitcbdd3f5.fc21.x86_64 seems to have fixed it.