GNOME Bugzilla – Bug 791340
Update gs-use-system-search.page
Last modified: 2017-12-19 18:47:41 UTC
Created attachment 365194 [details] [review] patch file to be reviewed by gnome-user-docs-maint I have altered gs-use-system-search.page, see attached patch. Could someone from the maint team please review it and give comment? Thanks, Hannie
The changes look all good to me. Thanks :) Could you change the commit summary to describe your actual changes and use present tense / imperative? Something like "Update to 3.26 control center UI - bug 791340" maybe?
+ <item><p>matching contacts, </p></item> + <item><p>matching documents,</p></item> + <item><p>matching calendar,</p></item> + <item><p>matching calculator,</p></item> Hannie, this is not the most important thing, but I notice you used Tab while previous lines had a sequence of whitespaces. Could you please fix the indentation before pushing the patch?
Review of attachment 365194 [details] [review]: The patch itself looks good, but it does not apply to master. The file gnome-help/C/gs-use-system-search.page does not appear in the gnome-user-docs tree. Do you have some other local changes that create (or rename) that file? It would be great if you could rearrange your local changes so that the patches attached to Bugzilla cleanly apply to master - thanks!
(In reply to David King from comment #3) > The file > gnome-help/C/gs-use-system-search.page does not appear in the > gnome-user-docs tree. Based on the title of the bug, is this intended for gnome-getting-started-docs instead? If so, could you reassign it there?
My bad. I thought gnome-getting-started-docs was an integrated part of gnome-user-docs and when I saw the latter in the list I chose that. I will send the patch to Bugzilla: gnome-getting-started-docs.
(In reply to André Klapper from comment #1) > The changes look all good to me. Thanks :) > > Could you change the commit summary to describe your actual changes and use > present tense / imperative? Something like "Update to 3.26 control center UI > - bug 791340" maybe? Good comment! I will see if I can find a good way to describe the changes in the commit summary. But since I add the comment (commit -m "description of the change") BEFORE creating and sending the patch to Bugzilla, I cannot include the bug #.
(In reply to David King from comment #3) > Review of attachment 365194 [details] [review] [review]: > > The patch itself looks good, but it does not apply to master. The file > gnome-help/C/gs-use-system-search.page does not appear in the > gnome-user-docs tree. Do you have some other local changes that create (or > rename) that file? It would be great if you could rearrange your local > changes so that the patches attached to Bugzilla cleanly apply to master - > thanks! Yes, I should have sent this patch to Bugzilla: gnome-getting-started. I tried to change it here under Product, but I get a message: You are moving the bug(s) to the product gnome-getting-started, and the version, component, and/or target milestone fields are no longer correct. Please set the correct version, component, and target milestone now: I have changed the .page file in Master, but the last version I can choose from the list above is 3.26.
(In reply to Rafael Fontenelle from comment #2) > + <item><p>matching contacts, </p></item> > + <item><p>matching documents,</p></item> > + <item><p>matching calendar,</p></item> > + <item><p>matching calculator,</p></item> > > Hannie, this is not the most important thing, but I notice you used Tab > while previous lines had a sequence of whitespaces. Could you please fix the > indentation before pushing the patch? Good catch! I will use whitespaces instead of Tab. Thanks for the review and comment.
(In reply to Hannie from comment #6) > (In reply to André Klapper from comment #1) > > The changes look all good to me. Thanks :) > > > > Could you change the commit summary to describe your actual changes and use > > present tense / imperative? Something like "Update to 3.26 control center UI > > - bug 791340" maybe? > > Good comment! I will see if I can find a good way to describe the changes in > the commit summary. But since I add the comment (commit -m "description of > the change") BEFORE creating and sending the patch to Bugzilla, I cannot > include the bug #. You can: $ git commit --amend and then, git format-patch, and re-submit the bug. When you add a new attachment in the same bug report, you can mark a previous attachment as obsolete.
(In reply to Hannie from comment #5) > My bad. I thought gnome-getting-started-docs was an integrated part of > gnome-user-docs and when I saw the latter in the list I chose that. I will > send the patch to Bugzilla: gnome-getting-started-docs. In the same bug report, on top, you can choose the product and change it from there. Just in case you were thinking to file a new bug in the other product.
(In reply to Hannie from comment #7) > (In reply to David King from comment #3) > > Review of attachment 365194 [details] [review] [review] [review]: > > > > The patch itself looks good, but it does not apply to master. The file > > gnome-help/C/gs-use-system-search.page does not appear in the > > gnome-user-docs tree. Do you have some other local changes that create (or > > rename) that file? It would be great if you could rearrange your local > > changes so that the patches attached to Bugzilla cleanly apply to master - > > thanks! > > Yes, I should have sent this patch to Bugzilla: gnome-getting-started. I > tried to change it here under Product, but I get a message: You are moving > the bug(s) to the product gnome-getting-started, and the version, component, > and/or target milestone fields are no longer correct. Please set the correct > version, component, and target milestone now: > I have changed the .page file in Master, but the last version I can choose > from the list above is 3.26. Then choose that one. Likely there is not major difference between those versions (3.26. is recent enough).
Germán Comment 11: I have chosen Product gnome-getting-started, ignored the message (see my comment #7), and checked the field 'gnome-getting-started developers'; I am not sure if that is right, the report is sent to only a small group of people. Component: General (instead of gnome-help). Version: git master.
Review of attachment 365194 [details] [review]: Thanks for your patch, it looks good. It is a good practice to wrap long lines at 79 characters, but that's just a nit (also, don't use tabs).
(In reply to Petr Kovar from comment #13) > Review of attachment 365194 [details] [review] [review]: > > Thanks for your patch, it looks good. > > It is a good practice to wrap long lines at 79 characters, but that's just a > nit (also, don't use tabs). I am so glad I get all this feedback! I consider this first patch a good start for me to learn how to change .page files and report these changes on Bugzilla. I will follow all the tips above and resubmit gs-use-system-search following Germán's instructions shortly. Thanks.
Created attachment 365624 [details] [review] Improved gs-use-system-search based on tips below. I have experimented with: better?? commit message (more than one line), whitespaces instead of tabs, break long lines using Enter. Please have a look and give comment.
Comment on attachment 365624 [details] [review] Improved gs-use-system-search based on tips below. >+ <item><p>matching contacts, </p></item> Minor: This should probably not have a whitespace at the end of the string. >+ <item><p>matching terminal,</p></item> Should " and" be added to the end of this line, similar to the style before?
(In reply to André Klapper from comment #16) > Comment on attachment 365624 [details] [review] [review] > Improved gs-use-system-search based on tips below. > > >+ <item><p>matching contacts, </p></item> > Minor: This should probably not have a whitespace at the end of the string. > >+ <item><p>matching terminal,</p></item> > Should " and" be added to the end of this line, similar to the style before? 1. Right. I will remove the whitespace. 2. In summing up a number of items, I personally would not add "and" in the last one. But if this is usually done in the docs I will add it. I will also have a look at what is said on this on the web. Thanks for the review.
See https://developer.gnome.org/gdp-style-guide/2.32/infodesign-12.html.en Do not connect list items with conjunctions such as "and" or "or".
Review of attachment 365624 [details] [review]: Thanks for your patch. Let's not block this on trailing whitespaces. Pushing as c4816c4e3a4bd9459f9d5ab175fcf8b8d5b325e9.