GNOME Bugzilla – Bug 763251
Big dark week numbers in year view
Last modified: 2017-04-17 18:20:40 UTC
Created attachment 323300 [details] screenshot Testing with GNOME continuous today, week numbers are shown in big dark squares. These wreck the layout of the view. They also look broken, since they overlap the edge of the view on the left side. See attached screenshot.
Georges tells me that the style is following what GNOME Shell does for week numbers. However, there are some significant differences between these week numbers and those in the shell: * The containing boxes are larger in Calendar. * The background of the boxes has more contrast with the background (it should be a lot lighter in Calendar). * Week numbers are an option in the shell, and are off by default.
Created attachment 323462 [details] [review] year-view: Week numbers overlapping view edge In some conditions when window is narrower than taller, week numbers overlaps the view edge. That problem started after we added a background to week numbers and moved it a little farther from the days. The whole drawing function didn't take in account the space used by the week numbers. To fix that we had to move the whole drawing area and start to count the space for the week numbers. We also had to change the logic to find the day and month based on x,y coordinates.
Allan, the above patch is to fix the first issue with week numbers overlapping the edges. I'll work on the second issue.
Comment on attachment 323462 [details] [review] year-view: Week numbers overlapping view edge The layout issue is gone. Thanks for your patch. Attachment 323462 [details] pushed as d7daa8f - year-view: Week numbers overlapping view edge
Created attachment 323658 [details] [review] year-view: Week numbers shown in big dark squares Design team testing with GNOME continuous, complained about week numbers being shown in big dark squares. According to them, these wreck the layout of the view. Design team listed the issues, comparing to gnome-shell calendar as: * The containing boxes are larger in Calendar. * The background of the boxes has more contrast with the background (it should be a lot lighter in Calendar). * Week numbers are an option in the shell, and are off by default. This patches fixes the two first items: week numbers box' size and color. Option to set on/off week numbers will be implemented in a future patch.
Comment on attachment 323658 [details] [review] year-view: Week numbers shown in big dark squares Thanks for the patch. Now there's only one more issue to fix wrt this bug. Attachment 323658 [details] pushed as 8af0e6f - year-view: Week numbers shown in big dark squares
Created attachment 323716 [details] [review] year-view: Read show-weekdate from GNOME Shell Year view always shows week numbers and by design team the default show not display them. This patch adds support to year-view be able to read org.gnome.shell.calendar::show-weekdate from GNOME Shell settings in advance to the re-layout of the view comming next.
Review of attachment 323716 [details] [review]: I'm not in favor of adding gcal_year_view_show_weekdate(..) function. Year view should be smart enough to read the setting by itself, and redraw when the setting changes. Long term, I want to move the 24h code to the views & edit dialog, and remove it from GcalWindow. Besides that, I think this patch doesn't really change the drawing code, right? ::: src/gcal-window.c @@ +1333,3 @@ + helper_settings = g_settings_new ("org.gnome.shell.calendar"); + show_week_numbers = g_settings_get_boolean (helper_settings, "show-weekdate"); + g_object_unref (helper_settings); I think it's better if this code is container in Year View itself. Also, a nice addition would be hooking the "changed::show-weekdate" signal with a redraw. Something like: g_signal_connect_swapped (settings, "changed::show-weekdate", gtk_widget_queue_redraw, drawing_area). ::: src/gcal-year-view.h @@ +40,3 @@ + gboolean show_week_numbers); +void gcal_year_view_set_current_date (GcalYearView *year_view, + icaltimetype *current_date); Avoid mixing style changes and real corrections. Also, see the other comment about the new API added here.
Created attachment 323801 [details] [review] year-view: Read show-weekdate from GNOME Shell Year view always shows week numbers and by design team the default show not display them. This patch adds support to read "show week numbers" setting from GNOME Shell (org.gnome.shell.calendar::show-weekdate) and to change year-view accordingly.
Review of attachment 323801 [details] [review]: Comments below, looking good overall. ::: src/gcal-year-view.c @@ +109,3 @@ }; +static GParamSpec *show_week_numbers_property; This static property holder is not needed. See the comment below. @@ +869,2 @@ g_free (nr_week); + } Fix the indentation of the code inside this condition. @@ +1102,2 @@ g_clear_pointer (&year_view->date, g_free); + g_object_unref (year_view->shell_settings); Use g_clear_object(&year_view->shell_settings). @@ +1119,3 @@ g_value_set_boxed (value, self->date); break; + case PROP_SHOW_WEEK_NUMBERS: Jump a line between the last 'break;' and this 'case'. @@ +1120,3 @@ break; + case PROP_SHOW_WEEK_NUMBERS: + g_value_set_boolean (value, gcal_year_view_get_show_week_numbers (self)); As commented below, use the show_week_numbers field directly. @@ +1121,3 @@ + case PROP_SHOW_WEEK_NUMBERS: + g_value_set_boolean (value, gcal_year_view_get_show_week_numbers (self)); + break; Ditto. @@ +1138,3 @@ update_date (GCAL_YEAR_VIEW (object), g_value_dup_boxed (value)); break; + case PROP_SHOW_WEEK_NUMBERS: Ditto. @@ +1139,3 @@ break; + case PROP_SHOW_WEEK_NUMBERS: + gcal_year_view_set_show_week_numbers (GCAL_YEAR_VIEW (object), g_value_get_boolean (value)); 1. As commented below, directly set the year_view->show_week_numbers property here. 2. Add a check for if (year_view->show_wn != g_value_get_boolean (value)) 3. Add a g_object_notify (year_view, "show-week-numbers"). @@ +1140,3 @@ + case PROP_SHOW_WEEK_NUMBERS: + gcal_year_view_set_show_week_numbers (GCAL_YEAR_VIEW (object), g_value_get_boolean (value)); + break; Ditto. @@ +1448,3 @@ + G_PARAM_READWRITE); + + g_object_class_install_property (object_class, PROP_SHOW_WEEK_NUMBERS, show_week_numbers_property); 1. Remove the show_week_numbers_property static handler. 2. Move the g_param_spec_boolean() to here. 3. Split this code in 3 lines, like this: g_object_class_install_property (object_class, PROP_SHOW_WEEK_NUMBERS, g_param_spec_boolean (...)); @@ +1565,3 @@ + year_view->show_week_numbers = show_week_numbers; +} + No need to add this API. Do this directly in the {get,set}_property() functions. ::: src/gcal-year-view.h @@ +43,3 @@ +void gcal_year_view_set_show_week_numbers (GcalYearView *year_view, + const gboolean show_week_numbers); + Remove this code from the header, since we're not using if from outside the year-view files.
Review of attachment 323801 [details] [review]: Comments below, looking good overall. ::: src/gcal-year-view.c @@ +109,3 @@ }; +static GParamSpec *show_week_numbers_property; This static property holder is not needed. See the comment below. @@ +869,2 @@ g_free (nr_week); + } Fix the indentation of the code inside this condition. @@ +1102,2 @@ g_clear_pointer (&year_view->date, g_free); + g_object_unref (year_view->shell_settings); Use g_clear_object(&year_view->shell_settings). @@ +1119,3 @@ g_value_set_boxed (value, self->date); break; + case PROP_SHOW_WEEK_NUMBERS: Jump a line between the last 'break;' and this 'case'. @@ +1120,3 @@ break; + case PROP_SHOW_WEEK_NUMBERS: + g_value_set_boolean (value, gcal_year_view_get_show_week_numbers (self)); As commented below, use the show_week_numbers field directly. @@ +1121,3 @@ + case PROP_SHOW_WEEK_NUMBERS: + g_value_set_boolean (value, gcal_year_view_get_show_week_numbers (self)); + break; Ditto. @@ +1138,3 @@ update_date (GCAL_YEAR_VIEW (object), g_value_dup_boxed (value)); break; + case PROP_SHOW_WEEK_NUMBERS: Ditto. @@ +1139,3 @@ break; + case PROP_SHOW_WEEK_NUMBERS: + gcal_year_view_set_show_week_numbers (GCAL_YEAR_VIEW (object), g_value_get_boolean (value)); 1. As commented below, directly set the year_view->show_week_numbers property here. 2. Add a check for if (year_view->show_wn != g_value_get_boolean (value)) 3. Add a g_object_notify (year_view, "show-week-numbers"). @@ +1140,3 @@ + case PROP_SHOW_WEEK_NUMBERS: + gcal_year_view_set_show_week_numbers (GCAL_YEAR_VIEW (object), g_value_get_boolean (value)); + break; Ditto. @@ +1448,3 @@ + G_PARAM_READWRITE); + + g_object_class_install_property (object_class, PROP_SHOW_WEEK_NUMBERS, show_week_numbers_property); 1. Remove the show_week_numbers_property static handler. 2. Move the g_param_spec_boolean() to here. 3. Split this code in 3 lines, like this: g_object_class_install_property (object_class, PROP_SHOW_WEEK_NUMBERS, g_param_spec_boolean (...)); @@ +1565,3 @@ + year_view->show_week_numbers = show_week_numbers; +} + No need to add this API. Do this directly in the {get,set}_property() functions. ::: src/gcal-year-view.h @@ +43,3 @@ +void gcal_year_view_set_show_week_numbers (GcalYearView *year_view, + const gboolean show_week_numbers); + Remove this code from the header, since we're not using it from outside the year-view files.
Created attachment 323908 [details] [review] Updated Russian translation
Created attachment 323910 [details] [review] year-view: Read show-weekdate from GNOME Shell Year view always shows week numbers and by design team the default show not display them. This patch adds support to read "show week numbers" setting from GNOME Shell (org.gnome.shell.calendar::show-weekdate) and to change year-view accordingly.
Review of attachment 323910 [details] [review]: Only a small nitpick. ::: src/gcal-year-view.c @@ +1141,3 @@ + case PROP_SHOW_WEEK_NUMBERS: + if (GCAL_YEAR_VIEW (object)->show_week_numbers != g_value_get_boolean (value)) Add a 'self = GCAL_YEAR_VIEW (object)' variable in the beggining of the function, and use that instead. You can update other parts of this function to use the 'self' var as well. @@ +1143,3 @@ + if (GCAL_YEAR_VIEW (object)->show_week_numbers != g_value_get_boolean (value)) + { + GCAL_YEAR_VIEW (object)->show_week_numbers = g_value_get_boolean (value); Ditto.
Created attachment 323912 [details] [review] year-view: Read show-weekdate from GNOME Shell Year view always shows week numbers and by design team the default show not display them. This patch adds support to read "show week numbers" setting from GNOME Shell (org.gnome.shell.calendar::show-weekdate) and to change year-view accordingly.
Created attachment 323915 [details] [review] year-view: Read show-weekdate from GNOME Shell Year view always shows week numbers and by design team the default show not display them. This patch adds support to read "show week numbers" setting from GNOME Shell (org.gnome.shell.calendar::show-weekdate) and to change year-view accordingly.
Thanks for all your work on this bug. We're having a great release with this addition. Attachment 323915 [details] pushed as 32c5c82 - year-view: Read show-weekdate from GNOME Shell