GNOME Bugzilla – Bug 129768
specify file name for automatic save screenshot to file
Last modified: 2014-05-02 17:26:35 UTC
I need an option NOT to ask file name from dialog, but specify it in command line arguments. Desired option: [stas@localhost]$ gnome-panel-screenshot [-window] filename.png
Created attachment 25892 [details] [review] added the functioanlity with --file option
Some style comments (Mark will say about the use of exit() ) [...] } + /* main */ int ... - } - + } [...] take_screen_shot (); } - + Please try to avoid these empty changes in your patches - start_temporary (); + if (!file_from_command_line) + start_temporary (); at this point, if file_from_command_line is not NULL, the program should have reach the exit, so there is no need for the condition (same below)
Created attachment 25947 [details] [review] made the required changes
Can this patch be applied ?
Patch no longer applies. A new patch can be written to do this, but I'd like to know what you intend to do when the file exists, or it isn't able to write the file. Right now, the code pops up a warning dialog -- you'll be hard pressed to not do the same here. Additionally, it has a (partially working) progress dialog when saving to a slow location.
We want easy-fix to be used when either the person who marks it as such is willing to help beginners fix the code, or else the person who marks it as such is sufficiently familiar with the code to know that beginners ought to be able to handle it by themselves. (Note that I just recently updated the description of the keyword to make this more clear.) From reading this bug, I am guessing that it wasn't marked following these rules so I'm removing it; please let me know if I am in error.
*** Bug 534451 has been marked as a duplicate of this bug. ***
Created attachment 144030 [details] [review] Attempt from scratch This implements the behaviour using a commandline switch "--file". If the file already exists and is writable, it is overwritten (as is generally the case with programs executed from the commandline). If the file can't be created, or can't be written to, an error is printed and the program exits with non-zero status.
*** Bug 608035 has been marked as a duplicate of this bug. ***
*** Bug 611139 has been marked as a duplicate of this bug. ***
Until this is fixed, gnome-screenshot is unsuitable for scripting. I'm writing a script now to automate screenshots for documentation so I guess I'm stuck using shutter for now.
Comment on attachment 144030 [details] [review] Attempt from scratch The patch needs updating, as it does not apply anymore to gnome-screenshot master, but I agree this would be a nice feature to have.
Created attachment 214677 [details] [review] Patch: screenshot: Add command options --file and --include_pointer Here's yet another patch that implements the new --file option. I also added --include_pointer, but when I test it on my Ubuntu 12.04 system, no pointer is included in the saved .png file (not in the old interactive mode either, not even without my changes).
Review of attachment 214677 [details] [review]: Hi, thanks for this patch - as a general comment, and to ease the review process, you shouldn't try to mix up different features in a single commit. Specifically, the part relative to the include-pointer option should be removed from this patch and attached as a separate one to bug 594141. For this reason, I'm going to ignore the parts relative to that in the review. ::: src/screenshot-application.c @@ +181,2 @@ screenshot_play_sound_effect ("dialog-error", _("Unable to capture a screenshot")); + self->priv->exit_status = EXIT_FAILURE; Not a big fan of saving this as internal state - I think you could just call exit (EXIT_FAILURE); in case we're being scripted (i.e. config->file != NULL) @@ +427,3 @@ + { + self->priv->save_uri = screenshot_build_filename_to_uri (screenshot_config->file); + screenshot_save_to_file (self); Use g_file_get_uri(), Also, you should set self->priv->should_overwrite to TRUE here instead of below. ::: src/screenshot-config.c @@ +77,3 @@ + g_printerr (_("Option --clipboard is ignored in interactive mode.\n")); + if (file_arg) + g_printerr (_("Option --file is ignored in interactive mode.\n")); These shouldn't be errors, but just warnings. Also, the clipboard one should be added separately, since it's not related to this feature. @@ +97,3 @@ if (border_effect_arg != NULL) + config->border_effect = g_strdup (border_effect_arg); + else This looks like a logic error you caught! I pushed this as a separate patch right away to git master (http://git.gnome.org/browse/gnome-screenshot/commit/?id=25203a16e532b8474c92dfd931ce07d59c21764c), so you can remove that part from this patch. @@ +123,3 @@ + config->include_pointer = include_pointer_arg; + if (file_arg != NULL) + config->file = g_strdup (file_arg); Use g_file_new_for_commandline_arg (file_arg) and create a GFile - see the other comments about this. ::: src/screenshot-config.h @@ +31,2 @@ gchar *save_dir; + gchar *file; This should be GFile *file; ::: src/screenshot-filename-builder.c @@ +322,3 @@ + +gchar * +screenshot_build_filename_to_uri (const gchar *filename) This function can be avoided by using a GFile.
Created attachment 214784 [details] [review] Patch: screenshot: Add command option --file > + self->priv->exit_status = EXIT_FAILURE; > Not a big fan of saving this as internal state - I think you could just call > exit (EXIT_FAILURE); in case we're being scripted (i.e. config->file != NULL) As a general rule I don't think it's very nice to bypass main() by calling exit(), but I've changed as requested. The real problem is that there is no g_application_set_exit_status() to set the exit status from g_application_run(). The glib bug 633267 mentions this issue. > Use g_file_new_for_commandline_arg (file_arg) and create a GFile Great! g_file_new_for_commandline_arg() simplifies a lot. I hadn't noticed that function. > the include-pointer option should be removed from this patch and attached as > a separate one to bug 594141. I've removed it. I may attach a patch to bug 594141 later.
Created attachment 214785 [details] [review] Patch: screenshot: Warn if --interactive is combined with --clipboard > Also, the clipboard one should be added separately, since it's not related > to this feature. Here's the --clipboard warning. It doesn't really fit in this bug report, but I don't know where else to put it. It's too unimportant to deserve a bug report of its own.
Review of attachment 214784 [details] [review]: Thanks, this looks good! Do you have a GNOME git account? If so, please push to git master after fixing the following comment (or I will push it for you if you don't). ::: src/screenshot-config.c @@ +74,3 @@ { + if (file_arg) + g_warning (_("Option --file is ignored in interactive mode.")); Don't translate terminal warnings.
Review of attachment 214785 [details] [review]: Looks good, but the warning shouldn't be translated.
Yes, I have a git account. Usually I work with C++ bindings (glibmm, gtkmm, etc.). I think the --file feature will be useful in the gtkmm-documentation module. I've pushed both patches with the small changes requested in the two previous comments. http://git.gnome.org/browse/gnome-screenshot/commit/?id=78bbf6ef75aa43973799d258e0a426119885cc07 http://git.gnome.org/browse/gnome-screenshot/commit/?id=cc3e215770298adcbf11fdd4405e671142f7d585
(In reply to comment #19) > Yes, I have a git account. Usually I work with C++ bindings (glibmm, gtkmm, > etc.). I think the --file feature will be useful in the gtkmm-documentation > module. > > I've pushed both patches with the small changes requested in the two previous > comments. > http://git.gnome.org/browse/gnome-screenshot/commit/?id=78bbf6ef75aa43973799d258e0a426119885cc07 > http://git.gnome.org/browse/gnome-screenshot/commit/?id=cc3e215770298adcbf11fdd4405e671142f7d585 Perfect, thanks!
*** Bug 563553 has been marked as a duplicate of this bug. ***