GNOME Bugzilla – Bug 694985
Mould date-time settings into new designs
Last modified: 2013-08-20 13:52:31 UTC
Allan Day's designs as shown in https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/date-and-time/date-and-time-hi-res.png need to be implemented. This bug tracks the task.
Created attachment 237780 [details] shot for the primary window
Created attachment 237781 [details] shot for the primary window
Created attachment 237949 [details] timezone.ui
Created attachment 237950 [details] date-and-time dialog
We really don't need screenshots of the builder file in Glade. If you want to iterate the GtkBuilder work with someone, Bugzilla isn't the right tool to do this.
Created attachment 250851 [details] [review] datetime: Resave the .ui file with recent Glade No functional changes; this is only to minimize changes in subsequent commits that touch the .ui file.
Created attachment 250853 [details] [review] datetime: Implement listbox based overview ... and move existing date/time and timezone settings to separate subdialogs.
Created attachment 250854 [details] [review] datetime: Show the timezone map in the original, unscaled size
Created attachment 250855 [details] [review] datetime: Move the 24h/12h selection to the main overview ... and use a combobox for choosing between the options.
Created attachment 250856 [details] [review] datetime: Implement new design for the time subdialog
Created attachment 250857 [details] [review] datetime: Show the UTC offset in the overview Replacing the previously shown timezone codename, so it will now show e.g. GMT+2 instead of CEST.
Created attachment 250858 [details] [review] datetime: Use g_clear_object()
Created attachment 250859 [details] [review] datetime: Mark the time format string as translatable ... and add a translator comment that explains what the string does. The RTL languages are likely going to want to show this in a different order. XXX: Anyone who can comment on this from translator's perspective? Not sure what is the best way here. Instead of exposing the format string to translators, could also reorder it in code based on date endianess.
Review of attachment 250851 [details] [review]: Let's hope this doesn't break anything.
Comment on attachment 250851 [details] [review] datetime: Resave the .ui file with recent Glade Attachment 250851 [details] pushed as c020116 - datetime: Resave the .ui file with recent Glade
Review of attachment 250853 [details] [review]: This is reviewed, will need to get thorough testing and ui-reviews. ::: panels/datetime/cc-datetime-panel.c @@ +303,3 @@ + /* Update the time on the listbow row */ + if (use_ampm) + label = g_date_time_format (priv->date, "%e %B %Y, %l:%M %p"); This needs to use a stock date time modifier. @@ +547,3 @@ + + /* Update the timezone on the listbow row */ + tz_info = tz_info_from_location (self->priv->current_location); This isn't translated. See "get_regions()" in the current code. @@ +1076,3 @@ + provider = gtk_css_provider_new (); + gtk_css_provider_load_from_data (GTK_CSS_PROVIDER (provider), + ".gnome-control-center-datetime-row-label:insensitive {\n" I'd rather this wasn't needed (what is it used for?). If specific CSS is required, it should go in Adwaita/gnome-themes-standard. ::: panels/datetime/datetime.ui @@ +197,3 @@ + <child internal-child="accessible"> + <object class="AtkObject" id="hour_up_accessible"> + <property name="accessible-description" translatable="yes">Set the time one hour ahead.</property> Those accessible descriptions need work. @@ +369,3 @@ + <child internal-child="accessible"> + <object class="AtkObject" id="month-accessible"> + <property name="accessible-description" translatable="yes">Month</property> Those short translatable descriptions need to use translation contexts.
Review of attachment 250854 [details] [review]: I think we probably want to be a bit more clever than that wrt to screen size for example. This is good enough for now though, pending testing/ui-review.
Review of attachment 250855 [details] [review]: Rest looks good. ::: panels/datetime/cc-datetime-panel.c @@ +1238,3 @@ else { + priv->ampm_available = TRUE; What changes on that line?
Review of attachment 250856 [details] [review]: ::: panels/datetime/cc-datetime-panel.c @@ +1140,3 @@ + adjustment = gtk_spin_button_get_adjustment (spin); + hour = (int)gtk_adjustment_get_value (adjustment); + if (use_ampm) I really don't understand that, why the %d vs. %02d? @@ +1176,3 @@ + char *text; + + date = g_date_time_new_utc (1, 1, 1, 0, 0, 0); What are those magic numbers for? @@ +1182,3 @@ + g_date_time_unref (date); + + date = g_date_time_new_utc (1, 1, 1, 12, 0, 0); What about this? @@ +1221,3 @@ + /* Big time buttons */ + provider = gtk_css_provider_new (); + gtk_css_provider_load_from_data (GTK_CSS_PROVIDER (provider), As mentioned before, I don't like the CSS here, especially with a hard-coded font size! ::: panels/datetime/datetime.ui @@ +153,3 @@ + <child internal-child="accessible"> + <object class="AtkObject" id="hour-accessible"> + <property name="accessible-description" translatable="yes">Hour</property> Needs context.
Review of attachment 250857 [details] [review]: Why is that bad? That's one of the things Jon mentioned during the hackfest where we would have wanted the "actual" name of the timezone, possibly in addition to use an offset as you did. Please check with the designers.
Review of attachment 250858 [details] [review]: Rest looks good. ::: panels/datetime/cc-datetime-panel.c @@ -156,3 @@ - if (priv->cancellable) - { - g_cancellable_cancel (priv->cancellable); You still need that call :)
Review of attachment 250859 [details] [review]: As mentioned during the meeting, please use the "stock" formats in GDateTime, we can add new ones there if needed to match mockups, but we really don't want translators to handle that.
(In reply to comment #19) > Review of attachment 250856 [details] [review]: > > ::: panels/datetime/cc-datetime-panel.c > @@ +1140,3 @@ > + adjustment = gtk_spin_button_get_adjustment (spin); > + hour = (int)gtk_adjustment_get_value (adjustment); > + if (use_ampm) > > I really don't understand that, why the %d vs. %02d? I tried to keep it consistent with the default formatting. As an example, the time that's shown in the GNOME Shell's top bar is either "9:50 AM" or "09:50", depending on if the 12h or 24h formatting is used. (At least with LC_TIME=en_US.utf8.) > + date = g_date_time_new_utc (1, 1, 1, 0, 0, 0); [snip] > + date = g_date_time_new_utc (1, 1, 1, 12, 0, 0); These are used for getting the labels for AM and PM. I'm constructing two dates, one at midnight and the other one at midday and then reading back the translated am/pm labels to show on the am/pm button. But yeah, these appear really magical here. I'll see if I can find a nicer way to construct this, and if not, I'll make sure it's obvious from the code what this is doing.
Comment on attachment 250858 [details] [review] datetime: Use g_clear_object() Attachment 250858 [details] pushed as e665689 - datetime: Use g_clear_object()
Thanks for the reviews! I've pushed two of the patches and will respin the rest and re-upload when I get home. I am sure that I'll be able to get rid of the custom CSS that's attached to labels on listbox rows; it was mostly a hack to work around what I think is a CSS state passing issue in listbox code. I'll work on this in GTK+ to fix it up properly.
(In reply to comment #23) > (In reply to comment #19) > > Review of attachment 250856 [details] [review] [details]: > > > > ::: panels/datetime/cc-datetime-panel.c > > @@ +1140,3 @@ > > + adjustment = gtk_spin_button_get_adjustment (spin); > > + hour = (int)gtk_adjustment_get_value (adjustment); > > + if (use_ampm) > > > > I really don't understand that, why the %d vs. %02d? > > I tried to keep it consistent with the default formatting. As an example, the > time that's shown in the GNOME Shell's top bar is either "9:50 AM" or "09:50", > depending on if the 12h or 24h formatting is used. (At least with > LC_TIME=en_US.utf8.) Can't we use gnome-wall-clock directly for that then? > > + date = g_date_time_new_utc (1, 1, 1, 0, 0, 0); > [snip] > > + date = g_date_time_new_utc (1, 1, 1, 12, 0, 0); > > These are used for getting the labels for AM and PM. I'm constructing two > dates, one at midnight and the other one at midday and then reading back the > translated am/pm labels to show on the am/pm button. Fair enough, but could you move that to a separate function to keep the hacks together :) > But yeah, these appear really magical here. I'll see if I can find a nicer way > to construct this, and if not, I'll make sure it's obvious from the code what > this is doing. (In reply to comment #25) > Thanks for the reviews! I've pushed two of the patches and will respin the rest > and re-upload when I get home. > > I am sure that I'll be able to get rid of the custom CSS that's attached to > labels on listbox rows; it was mostly a hack to work around what I think is a > CSS state passing issue in listbox code. I'll work on this in GTK+ to fix it up > properly. That would be great, thanks.
putting this on the 3.10 watchlist
Created attachment 252240 [details] [review] datetime: Implement listbox based overview ... and move existing date/time and timezone settings to separate subdialogs.
Created attachment 252241 [details] [review] datetime: Show the timezone map in the original, unscaled size
Created attachment 252242 [details] [review] datetime: Move the 24h/12h selection to the main overview ... and use a combobox for choosing between the options.
Created attachment 252243 [details] [review] datetime: Implement new design for the time subdialog
Created attachment 252244 [details] [review] datetime: Implement timezone search with autocompletion This is first cut at implementing the new timezone dialog design. Instead of having drop down menus with the city and the continent, we're now switching to a search entry that has autocompletion.
Created attachment 252245 [details] [review] datetime: Add API to draw a text bubble on the timezone map It's currently drawn either west or east of the position marker, depending where we have more free space.
Created attachment 252246 [details] [review] datetime: Show information about the selected timezone ... using the text bubble API from the previous commit. We're currently displaying the time zone abbreviation, UTC offset, city and country name, and the current time.
Created attachment 252247 [details] [review] datetime: Mark the time format strings as translatable ... and add translator comments explainings what they do. The RTL languages are likely going to want to show them in a different order. We should eventually move to just using GnomeWallClock and/or g_date_time_format with %c that already have translated the time format strings. However, in case this doesn't make it in 3.10, mark the current strings in g-c-c as translatable to get some translation coverage.
Created attachment 252248 [details] [review] datetime: Toggle the NTP switch when clicking on the row Pointed out by Allan Day on IRC.
Review of attachment 252247 [details] [review]: Is there a bug filed for the translated time format strings? ::: panels/datetime/cc-datetime-panel.c @@ +536,3 @@ else + { + /* Translators: This is the time format used for AM/PM. */ The comments should be swapped here. You should probably also be consistent in using either "12-hour mode" or "AM/PM".
Review of attachment 252240 [details] [review]: ::: panels/datetime/cc-datetime-panel.c @@ +1018,3 @@ + goto out; + + memcpy (found, "dialog", 6); :-) @@ +1104,3 @@ + ret = gtk_builder_add_from_resource (priv->builder, + "/org/gnome/control-center/datetime/datetime.ui", + &error); Not your bug but, we have two error variables here and they are being used as if they were a single one. There's an 'err' and an 'error'. We should have only one and when (priv->dtm == NULL) we should use g_clear_error() instead. @@ +1196,3 @@ + gtk_widget_unparent (widget); + gtk_container_add (GTK_CONTAINER (self), widget); + g_object_unref (widget); gtk_widget_reparent() saves you 3 lines here.
Review of attachment 252241 [details] [review]: ok
Review of attachment 252242 [details] [review]: ok
Review of attachment 252243 [details] [review]: ::: panels/datetime/cc-datetime-panel.c @@ -931,2 @@ static void -reorder_date_widget (DateEndianess endianess, Seems like we don't need the date-endian.* files anymore. @@ +1125,3 @@ + char *text; + + date = g_date_time_new_utc (1, 1, 1, 0, 0, 0); A comment here wouldn't hurt. @@ +1154,3 @@ + gtk_style_context_add_provider (context, + GTK_STYLE_PROVIDER (provider), + GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); _add_provider() adds its own reference to the provider so we should g_object_unref(provider) here. @@ +1177,3 @@ + gtk_style_context_add_provider (context, + GTK_STYLE_PROVIDER (provider), + GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); idem @@ +1190,1 @@ cc_date_time_panel_init (CcDateTimePanel *self) This function is long enough as is. I think the changes below could be moved, along with the surrounding code, to the respective setup_*_dialog() functions. ::: panels/datetime/datetime.ui @@ +147,3 @@ + <property name="width_chars">2</property> + <property name="xalign">0.5</property> + <property name="input_purpose">number</property> s/number/digits/ @@ +184,3 @@ + <property name="width_chars">2</property> + <property name="xalign">0.5</property> + <property name="input_purpose">number</property> s/number/digits/
Review of attachment 252243 [details] [review]: Also, the day-liststore and year-liststore objects in the .ui file can be removed.
Created attachment 252308 [details] [review] datetime: Implement listbox based overview ... and move existing date/time and timezone settings to separate subdialogs.
Created attachment 252309 [details] [review] datetime: Clean up error handling Avoid using two GError variables in a single function to reduce programmer confusion. Instead, only use one and clear it with g_clear_error() when we need to reuse it. Pointed out by Rui Matos.
(In reply to comment #38) > Review of attachment 252240 [details] [review]: > @@ +1104,3 @@ > + ret = gtk_builder_add_from_resource (priv->builder, > + > "/org/gnome/control-center/datetime/datetime.ui", > + &error); > > Not your bug but, we have two error variables here and they are being used as > if they were a single one. There's an 'err' and an 'error'. Ah indeed. Edited this patch to use err here. > > We should have only one and when (priv->dtm == NULL) we should use > g_clear_error() instead. Done, attached a new patch with this. > @@ +1196,3 @@ > + gtk_widget_unparent (widget); > + gtk_container_add (GTK_CONTAINER (self), widget); > + g_object_unref (widget); > > gtk_widget_reparent() saves you 3 lines here. Fixed.
Created attachment 252318 [details] [review] datetime: Implement new design for the time subdialog
Created attachment 252319 [details] [review] datetime: Shuffle initialization code around Move subdialog setup code out of the main init function and to respective setup_*_dialog() functions.
(In reply to comment #41) > Review of attachment 252243 [details] [review]: > > ::: panels/datetime/cc-datetime-panel.c > @@ -931,2 @@ > static void > -reorder_date_widget (DateEndianess endianess, > > Seems like we don't need the date-endian.* files anymore. We discussed this on IRC and Bastien and Allan said we should still reorder the widgets, no matter if they are positioned vertically. Let's keep the date-endian.* files around for now. > @@ +1125,3 @@ > + char *text; > + > + date = g_date_time_new_utc (1, 1, 1, 0, 0, 0); > > A comment here wouldn't hurt. I've moved these out to separate functions and added comments explaining what the magical numbers do. > @@ +1154,3 @@ > + gtk_style_context_add_provider (context, > + GTK_STYLE_PROVIDER (provider), > + GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); > > _add_provider() adds its own reference to the provider so we should > g_object_unref(provider) here. Fixed. > @@ +1177,3 @@ > + gtk_style_context_add_provider (context, > + GTK_STYLE_PROVIDER (provider), > + GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); > > idem Fixed. > @@ +1190,1 @@ > cc_date_time_panel_init (CcDateTimePanel *self) > > This function is long enough as is. I think the changes below could be moved, > along with the surrounding code, to the respective setup_*_dialog() functions. Agreed. I've amended the patch to put the new code directly in the setup_*_dialog() functions, and attached another patch that moves the old surrounding setup code to the respective setup_*_dialog() functions as well. > ::: panels/datetime/datetime.ui > @@ +147,3 @@ > + <property name="width_chars">2</property> > + <property name="xalign">0.5</property> > + <property name="input_purpose">number</property> > > s/number/digits/ Fixed. > @@ +184,3 @@ > + <property name="width_chars">2</property> > + <property name="xalign">0.5</property> > + <property name="input_purpose">number</property> > > s/number/digits/ Fixed. > Also, the day-liststore and year-liststore objects in the .ui file can be > removed. Fixed.
Created attachment 252321 [details] [review] datetime: Mark the time format strings as translatable ... and add translator comments explainings what they do. The RTL languages are likely going to want to show them in a different order. We should eventually move to just using GnomeWallClock and/or g_date_time_format with %c that already have translated the time format strings. However, in case this doesn't make it in time for 3.10, mark the current strings in g-c-c as translatable to get some translation coverage.
Review of attachment 252244 [details] [review]: ::: panels/datetime/cc-datetime-panel.c @@ +515,3 @@ g_free (string); } while (gtk_tree_model_iter_next (model, &iter)); Instead of doing this linear search we could factor out the load_cities() code and call it here to get the human readable string. @@ -635,2 @@ /* Slash look-alikes that might be used in translations */ #define TRANSLATION_SPLIT \ This define isn't used anymore, might as well remove it. @@ +977,3 @@ + "city-modelsort")); + gtk_entry_completion_set_model (completion, completion_model); + g_object_unref (completion_model); Drop the unref, we don't own completion_model here.
Review of attachment 252245 [details] [review]: ++
Review of attachment 252246 [details] [review]: ++
Review of attachment 252248 [details] [review]: yes!
Review of attachment 252308 [details] [review]: ++
Review of attachment 252309 [details] [review]: ++
Review of attachment 252318 [details] [review]: Cool. The date widgets re-ordering is a UI freeze break though.
Review of attachment 252319 [details] [review]: ++
Review of attachment 252321 [details] [review]: /me nods
Attachment 252248 [details] pushed as 6fddeda - datetime: Toggle the NTP switch when clicking on the row Attachment 252318 [details] pushed as f5cb98c - datetime: Implement new design for the time subdialog Attachment 252319 [details] pushed as 20670b5 - datetime: Shuffle initialization code around Attachment 252242 [details] pushed as 60e448b - datetime: Move the 24h/12h selection to the main overview Attachment 252241 [details] pushed as 9a3265d - datetime: Show the timezone map in the original, unscaled size Attachment 252309 [details] pushed as f90edf4 - datetime: Clean up error handling Attachment 252308 [details] pushed as a0d156d - datetime: Implement listbox based overview
Created attachment 252408 [details] [review] datetime: Implement timezone search with autocompletion This is first cut at implementing the new timezone dialog design. Instead of having drop down menus with the city and the continent, we're now switching to a search entry that has autocompletion.
Review of attachment 252408 [details] [review]: Looks good with that fix. ::: panels/datetime/cc-datetime-panel.c @@ +486,3 @@ + + /* Load the translation for it */ + zone_translated = dgettext (GETTEXT_PACKAGE_TIMEZONES, loc->zone); You're skipping one step here: g_strdelimit (zone_translated, "_", ' '); Otherwise we get "Buenos_Aires" and so on.
Review of attachment 252408 [details] [review]: ::: panels/datetime/cc-datetime-panel.c @@ +486,3 @@ + + /* Load the translation for it */ + zone_translated = dgettext (GETTEXT_PACKAGE_TIMEZONES, loc->zone); Forgot to mention that that has to be done on a copy of the string though.
> You're skipping one step here: g_strdelimit (zone_translated, "_", ' '); > > Otherwise we get "Buenos_Aires" and so on. Fixed.
Thanks for the reviews, both Bastien and Rui! Attachment 252245 [details] pushed as 05dc928 - datetime: Add API to draw a text bubble on the timezone map Attachment 252246 [details] pushed as da9d4a7 - datetime: Show information about the selected timezone Attachment 252321 [details] pushed as c5812c3 - datetime: Mark the time format strings as translatable Attachment 252408 [details] pushed as 7181189 - datetime: Implement timezone search with autocompletion