GNOME Bugzilla – Bug 783810
units: use plurals where appropriate
Last modified: 2017-06-20 03:01:43 UTC
We should try to match how you would normally write this: 1 kilogram 200 grams 2 1/2 ounces 20 g 7 l So: use plural forms of the unit name if the amount is > 1 and it is spelled out. Currently, we use the unabbreviated unit names only in the completion popup on the edit page, I believe.
Created attachment 353810 [details] [review] patch by paxana
Review of attachment 353810 [details] [review]: This looks like it is an incremental patch on top of the earlier one you sent by mail. Can you produce a unified patch against git master, please ? Actually, I think this should be two patches: one that adds the plurals support to gr-unit.h and gr-unit.c, and another one that uses it in gr-ingredients-viewer-row.c
Review of attachment 353810 [details] [review]: ::: src/gr-ingredients-viewer-row.c @@ +712,1 @@ const char *plural; There is another gtk_list_store_insert_with_values call a bit further up (not visible here), that will need the extra columns added as well. @@ +735,3 @@ 2, tmp, 3, plural, + 4, tmp2, This is actually a bit wasteful - I don't think we ever use the name or plural columns (1 and 3) for anything. We should perhaps remove those columns (in a separate patch). @@ +818,3 @@ + static void + text_changed (GObject *object, GParamSpec *pspec, gpointer data) + { Some indentation trouble here - the {} for the function scope should be in column 0, and then we have an indent of 8 @@ +826,3 @@ + gr_number_parse (&number, &text, NULL); + if (gr_number_parse (&number, &text, NULL)) { g_print ("number: %s\n", gr_number_format (&number)); } + The debug g_print's here should go, obviously. @@ +833,3 @@ + else { + gtk_cell_layout_set_attributes (GTK_CELL_LAYOUT (row->unit_completion), row->unit_cell, "text", 2, NULL); + } This (checking the number.value) needs to go inside the body of the previous if, I think - if gr_number_parse failed, we don't really know that number.value has any meaningful value, and thus we shouldn't be using it, and should probably just fall back to using the singular in that case. @@ +850,3 @@ + gchar *unit = NULL; + + completion = gtk_entry_completion_new (); Was there a reason to move the completion creation up here ? @@ +872,3 @@ cell = gtk_cell_renderer_text_new (); gtk_cell_layout_set_cell_data_func (GTK_CELL_LAYOUT (completion), cell, get_amount, self, NULL); + parse_unit (gtk_entry_get_text (GTK_ENTRY (self->unit_entry)), &amount, &unit); Doesn't look like this is used - just remove it
Created attachment 353926 [details] [review] plurals api causing crash, working on converting between metric and imperial
Hmm, looks like you attached the first patch here. Which is ok, but not quite what I had hoped for: I'd like you to merge the two patches together, and then (after cleaning up the things I commented on) split it up in to two patches: 1. A patch for gr-unit.h and gr-unit.c, with a subject like: unit: Add api to obtain plural of unit names 2. A patch for gr-ingredients-viewer-row.c that uses the api A rough outline of how to do this: patch -p1 < patch1 patch -p1 < patch2 <do cleanups> git add src/gr-unit.[hc] git commit git add src/gr-ingredients-viewer-row.c git commit
Created attachment 354033 [details] [review] merged git patches hoping i merged the right commits
Something went wrong here. What you attached is a commit that adds 3 patches as files to git, which is obviously not what we want. Lets walk through this together when we meet in a bit.
Created attachment 354065 [details] [review] added plural values to gr-unit.c added a getter in gr-units.h
Created attachment 354066 [details] [review] added plural support in new recipe ingredients lists
Review of attachment 354065 [details] [review]: looks ok. the commit message could use some work, along these lines: Add plural api for unit names Add translatable plurals of the unit names, and add a new getter to get them: gr_unit_get_plural().
Review of attachment 354066 [details] [review]: Same comment as in the other patch: commit message could use some work. I'll clean it up when I commit it.