GNOME Bugzilla – Bug 760796
Doesn't have a --version flag
Last modified: 2016-06-20 19:52:11 UTC
It would be nice to see the currently installed version of gnome-photos from the command line with a --version flag.
working on it
Created attachment 323397 [details] [review] Gnome-Photos: adding --version flag to application Here, commandline arguments version and v are added to the application There was no commandline argument handling function in this gnome-photos application. To know the version of the current application was difficult. But with addition of this functionality, version of application can be found out by passing this argument when running through commandline.
Review of attachment 323397 [details] [review]: Why there is a new file at all in a programming/ subdirectory ? I think you by mistake has done something wrong. Please check and make sure that it compiles and works successfully before submitting.
Created attachment 323482 [details] [review] adding version flag to gnome-photo
Can you please attach it as "git format-patch" ? Have you read https://wiki.gnome.org/Newcomers/CodeContributionWorkflow ?
Created attachment 323681 [details] [review] Version flag is added to gnome-photos
Comment on attachment 323681 [details] [review] Version flag is added to gnome-photos >+ g_free (argv[i]); >+ for (j = i; argv[j]; j++) >+ argv[j] = argv[j + 1]; Is it really needed to shift all the remaining arguments? >+ } Shouldn't you break here? Why keep looking in the rest of the arguments? >+ return 0; >+} Hmm. What if I have a running instance of gnome-photos and then I try to get the version from the command line? I think this is the kind of option that can be handled locally.
(In reply to Rafael Fonseca from comment #7) > Comment on attachment 323681 [details] [review] [review] > Version flag is added to gnome-photos > > >+ g_free (argv[i]); > >+ for (j = i; argv[j]; j++) > >+ argv[j] = argv[j + 1]; > > Is it really needed to shift all the remaining arguments? > > >+ } > > Shouldn't you break here? Why keep looking in the rest of the arguments? > > >+ return 0; > >+} > > Hmm. What if I have a running instance of gnome-photos and then I try to get > the version from the command line? I think this is the kind of option that > can be handled locally. Shifting the argument is actually to dealt with any other argument is specified after or before the given argument that can be handled by given function. It is useful if other argument is added to the argument-list and then it can be handled. Actually, the handling of command line can be split between primary and launcher instances. Normally, the commandline is completely handled in the “command-line” handler. The launching instance exits once the signal handler in the primary instance has returned, and the return value of the signal handler becomes the exit status of the launching instance.
Review of attachment 323681 [details] [review]: Thanks for the patch, Abhinav. Have you read https://wiki.gnome.org/HowDoI/GtkApplication/CommandLine ? If not, you might find it helpful. The URL of this bug should be added to the commit message (see some of the other commits), and would be nice to properly capitalize your name. I mention the name because it shows up in the UI (in the about dialog). ::: src/photos-main.c @@ +47,3 @@ + for (i = 0; i < argc; i++) +{ +if (g_str_has_prefix (argv[i], "--version") || g_str_has_prefix(argv[i],"-v")) Use g_application_add_main_option_entries in photos_application_init to set the list of options. @@ +83,2 @@ app = photos_application_new (); + g_signal_connect (app, "command-line", G_CALLBACK (command_line), NULL); We should use handle-local-options, not command-line. Among other things, it will address the issue Rafael pointed out. Namely, gnome-photos --version will work even if the application is already running. Secondly, since we subclass GApplication (see photos-application.c), it will be nicer to override the handle_local_options vfunc instead of connecting to the handle-local-options signal.
Created attachment 324744 [details] [review] Here version flag is added through GOptionEntry
Review of attachment 324744 [details] [review]: Things to be addressed from the previous review: * Bug URL is missing from commit message. * Write your name like the other entries in git log because it shows up in the UI - specifically, the about dialog. See https://en.wikipedia.org/wiki/Capitalization for the meaning of the word. ::: src/photos-application.c @@ +1660,3 @@ +if (g_variant_dict_contains (options, "version")) + { + g_print ("%s - Version %s\n", "Gnome-Photos", VERSION); We should enclose the name of the application in _("..."), and either use PACAKGE_NAME or PACKAGE_TARNAME as the name. See photos-main-window.c for an example. s/VERSION/PACKAGE_VERSION/ @@ +1663,3 @@ + return 0; + } +} We are not returning anything when the option is not found, which will lead to bad things. We should return -1. I would suggest using a 'gint ret_val' variable that is initialized to -1 to track the return value. @@ +1671,3 @@ + { "version", + 'v', + G_OPTION_FLAG_IN_MAIN, I am curious. Did you see these values used somewhere else? I am asking because usually there is no short variant for 'version'. Also, why G_OPTION_FLAG_IN_MAIN instead of G_OPTION_FLAG_NONE? @@ +1674,3 @@ + G_OPTION_ARG_NONE, + NULL, + N_("Show the application's version") }, All the fields of each struct element can go in one line. We usually go till 120 characters per line in the code. @@ +1691,3 @@ self); + Spurious newline. @@ +1706,3 @@ object_class->finalize = photos_application_finalize; application_class->activate = photos_application_activate; + application_class->handle_local_options = photos_application_handle_local_options; Nitpick: move it below dbus_unregister so that the order is maintained.
Review of attachment 324744 [details] [review]: I have addressed all other issues which you have pointed you out, thanks for that. About bug url, when I try to attach the patch through git-bz attach command it shows 'git: 'bz' is not a git command. See 'git --help'. ' I have looked for it still can't find any solution. And I follow the installation procedure as stated in https://wiki.gnome.org/Newcomers/CodeContributionWorkflow. I think this might be causing that problem. ::: src/photos-application.c @@ +1671,3 @@ + { "version", + 'v', + G_OPTION_FLAG_IN_MAIN, I used G_OPTION_FLAG_IN_MAIN so that version flag can remain in main list help menus but I have changed that to G_OPTION_FLAG_NONE now. I have removed the short version 'v' from the patch.
Created attachment 326372 [details] [review] Issued specified is solved
Created attachment 326427 [details] [review] Issues on the previous patch are addressed
I have solved the issue of bug URL missing from commit message. I think it will come up now.
Review of attachment 326427 [details] [review]: A few quick comments before going any further. When you incorporate the feedback from a patch review, you should usually re-attach one single patch instead of a patch that modifies the older one. See https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Follow_Up_on_the_Feedback for a possible workflow. Secondly, this patch won't apply on top of the earlier one because ... ::: src/photos-application.c @@ +1657,1 @@ gb_application_handle_local_options (GApplication *app, ... the prefix changed from "photos" to "gb".
Review of attachment 326427 [details] [review]: ::: src/photos-application.c @@ +1661,1 @@ if (g_variant_dict_contains (options, "version")) The 'if' is misaligned and there should be a newline separating it from the variable definition. @@ +1662,3 @@ { + g_print ("%s - Version %s\n", PACKAGE_NAME, VERSION); + return ret_val; This isn't what I meant. We should return 0 when "version" is found, -1 otherwise. This is returning -1 when "version" is found, nothing (some garbage) otherwise.
Created attachment 329961 [details] [review] application: Fix the check to detect if we were the primary instance
Created attachment 329962 [details] [review] application: Handle --version from the command line
Pushed to master.