GNOME Bugzilla – Bug 746927
(Year view) Mark on gray the proper day on year-navigator. It should not be always Sunday.
Last modified: 2017-08-24 09:06:01 UTC
Right now, in the year view, sunday is gray (while other days are black). The software shouldn't assume sunday is a special day.
(In reply to Abderrahim Kitouni from comment #0) > Right now, in the year view, sunday is gray (while other days are black). > The software shouldn't assume sunday is a special day. How would you like us to let the user know which are the weeks days? This is by design. We have to choose a particular day, we chose Sunday. We'll stay like this.
I think I wasn't clear enough. What I wanted to say is that Sunday is a work day where I live. The translator should be able to choose the "special day" according to the country (for me it would be Friday)
(In reply to Abderrahim Kitouni from comment #2) > I think I wasn't clear enough. What I wanted to say is that Sunday is a work > day where I live. The translator should be able to choose the "special day" > according to the country (for me it would be Friday) Now, that's a different stuff, so, the problem is which day to mark? Do you have a work week with more than one day off? What does your locale data say?
Yes, it's about which day to mark. I thought it would be clear from the title. Anyway, depending on where you work, you could have either Friday or Friday and Saturday. I don't know what the locale data say.
This issue was solved in https://bugzilla.gnome.org/show_bug.cgi?id=762428
The issue still isn't solved. gnome-calendar still marks sunday in gray, even though gnome-shell is correctly marking friday and saturday.
Created attachment 336373 [details] Screenshot of the bug
Created attachment 351839 [details] [review] Use translatable string for non working day selection This patch uses the solution the gnome-shell calendar plugin uses. This is a translatable string containing the non working days (e.g. "0" sunday, "6" saturday, "06" saturday+sunday) combined with a helper function. For this to work i had to rework the calendar-view logic for choosing "sunday" columns as this would only allow one day to be a non working day.
Review of attachment 351839 [details] [review]: Nice patch! However, I'm opposed to the usage of translated strings for this end in is_workday(). The biggest problem is that the date format might be different than the language. Some users, for example, might have a German desktop but use UK date format. I think the right approach is checking LC_ALL, then LC_TIME, then falling back to the translation. What do you think?
Hey, I'd like to complete this patch, but I've no intention to steal anybody's work. If you think it's OK, I'll provide my attempt Best regards, Carlo
Sure go ahead. Sadly I am currently stuck on a problem with gnome-builder so feel free to take this over.
Created attachment 356728 [details] [review] year-view: Mark working days according to Locale Thank you, here is the patch
Review of attachment 356728 [details] [review]: I think this approach is not right. Workdays around the world are well known and documented [1], so we can just hardcode it here. Resorting to translators would be a bad approach. [1] https://en.wikipedia.org/wiki/Workweek_and_weekend
> Workdays around the world are well known and documented [1], so we can just hardcode it here. Ok, I'll update the mapping > Resorting to translators would be a bad approach. I don't understand this, I'm not resorting on translators here
By the way, looking again at the patch I also saw some memory leaks, I'll fix those as well
Created attachment 357992 [details] [review] Patch v2 - hard coded mapping between territory and non-working days (except territories with Sunday+Saturday as non working days, considered as "default") - fixed some memory leaks
Review of attachment 357992 [details] [review]: Some comments below. I think that, with the idea that I proposed, the implementation will be much more readable and better. What do you think? :) ::: src/gcal-utils.c @@ +1237,3 @@ +{ + gchar *no_work_days; /* sequence of non-working days as numbers. Sunday = 0 */ + gchar *territory; /* territory string as in Locale */ Instead of using strings, you should add a new "GcalWeekDay" bitfield in the .h file with the week days. Something like: typedef enum { GCAL_WEEK_DAY_INVALID = 0, GCAL_WEEK_DAY_SUNDAY = 1 << 0, GCAL_WEEK_DAY_MONDAY = 1 << 1, ... GCAL_WEEK_DAY_SATURDAY = 1 << 6, } GcalWeekDay; @@ +1242,3 @@ +/* Mapping of the Territories with non-working days other + * than the most common Saturday-Sunday == "06" */ +static GcalNonWorkingDays no_work_days_map [] = { - The comments are nor following Calendar style. It should be: /* * <multiline comment> */ - No need to typedef a struct here. You can use an anonymous one. Like: struct { const gchar *territory; GcalWeekDay workdays; } workday_per_locale[] = { ... }; @@ +1270,3 @@ + {"0", "UG"} /* Uganda */, + {"56", "YE"} /* Yemen */, +}; If you add the GcalWeekDay bitfield, you'll be able to write this as: { "YE", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } Much more semantic and clear, don't you agree? :) @@ +1278,3 @@ + * Checks whether @day is workday or not based on the Territory part of Locale. + * + * Returns: %TRUE if @source is a workday, %FALSE otherwise. [..] if %day is a workday [...] @@ +1290,3 @@ + + /* Saturday+Sunday is the most common non-working days list, + * so be the no_work_days string "06" by default */ Wrong style. See coment above. @@ +1311,3 @@ + territory = NULL; + + locale_regex = g_regex_new ("^(?P<lang>[a-z]+)_(?P<territory>[A-Z]+).*", G_REGEX_RAW, 0, NULL); I think using a regex here is an overkill. It's safe to assume that all locales will be formatted as <lang>_<TERRITORY>.<charset>. Thus, you can just retrieve the 4th and 5th characters. Of course, you should put some sanity checks like g_return_val_if_fail (len >= 5). @@ +1331,3 @@ + if (g_strcmp0 (territory, no_work_days_map [i].territory) == 0) + { + no_work_days = no_work_days_map [i].no_work_days; Wong style (no space before '[') @@ +1344,3 @@ + + while (*no_work_days) + if (*no_work_days++ - '0' == day) return FALSE; When you add the GcalWeekDay enum, this can be written as: return !(not_workday & 1 << day);
I agree with the comments, thanks! Working on the fixes
Created attachment 358007 [details] [review] Patch v3 - used hidden struct in place of typedef - new enum GcalWeekDay - removed regex and added check for locale's expected format - style fixes
Review of attachment 358007 [details] [review]: The patch is missing the GcalWeekDay enum. Per the comments, please double check the code style. Calendar code style is basically the GTK code style [1][2]. [1] https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en [2] https://git.gnome.org/browse/gtk+/tree/docs/CODING-STYLE ::: src/gcal-utils.c @@ +1246,3 @@ + GcalWeekDay no_work_days; + gchar *locale; + gchar territory[3]; Initialize to 0, as in 'gchar territory[3] = { 0, };' @@ +1252,3 @@ + gchar *territory; + GcalWeekDay no_work_days; + } no_work_day_per_locale[] = { This should be outside the function (to not allocate it on the stack every time we call). @@ +1279,3 @@ + { "SY", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } /* Syria */, + { "UG", GCAL_WEEK_DAY_SUNDAY } /* Uganda */, + { "YE", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } /* Yemen */, Nitpick: for the sake of legibility, please align the final } @@ +1288,3 @@ + + locale = getenv ("LC_ALL"); + if (locale == NULL || strlen(locale) <= 5) - if (!locale || ...) - Wrong style: there should be a space before '(' - Use g_utf8_strlen() - I think it should be '< 5', shouldn't it? @@ +1290,3 @@ + if (locale == NULL || strlen(locale) <= 5) + locale = getenv ("LC_TIME"); + if (locale == NULL) { - if (!locale) - Wrong style: curly braces are on the next line @@ +1292,3 @@ + if (locale == NULL) { + g_warning ("Locale is NULL"); + goto out; Instead of a goto, you can put another return here. @@ +1297,3 @@ + g_return_val_if_fail (strnlen (locale, 6) <= 5, TRUE); + + memcpy(territory, &locale[3], 2); Nitpick: I prefer territory[0] = locale[3]; territory[1] = locale[4]; @@ +1298,3 @@ + + memcpy(territory, &locale[3], 2); + territory[2] = '\0'; If you initialize to 0, this line won't be needed.
Created attachment 358023 [details] [review] Patch v4 Sorry for the style errors
Review of attachment 358023 [details] [review]: Now this is looking much better! And, more importantly, works too :) Left some nitpicks and a small comment. I think it'll be good to go after that. Thanks! ::: src/gcal-utils.c @@ +1242,3 @@ + { "BD", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } /* Bangladesh */, + { "BH", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } /* Bahrain */, + { "BN", GCAL_WEEK_DAY_SUNDAY | GCAL_WEEK_DAY_FRIDAY } /* Brunei Darussalam */, Nitpick: FRIDAY is misaligned @@ +1265,3 @@ + { "UG", GCAL_WEEK_DAY_SUNDAY } /* Uganda */, + { "YE", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } /* Yemen */, + }; Nitpick: this entire block is misaligned (it has 2 spaces of indentation, should have none) @@ +1295,3 @@ + if (!locale) + { + g_warning ("Locale is NULL"); Nitpick: "Locale is NULL, assuming Saturday and Sunday as non workdays" @@ +1296,3 @@ + { + g_warning ("Locale is NULL"); + return FALSE; This should return !(no_work_days & 1 << day) too ::: src/views/gcal-year-view.c @@ +971,3 @@ { gtk_style_context_save (context); gtk_style_context_add_class (context, "sunday"); Now that we're using the term "workday", it makes sense to change the "sunday" CSS class to "non-workday".
Comment on attachment 351839 [details] [review] Use translatable string for non working day selection Marking the previous patch as obsolete.
Created attachment 358085 [details] [review] Patch v5 Everything should be fixed now :) - [X] Nitpick: FRIDAY is misaligned - [X] Nitpick: this entire block is misaligned (it has 2 spaces of indentation, should have none) - [X] Nitpick: "Locale is NULL, assuming Saturday and Sunday as non workdays" - [X] This should return !(no_work_days & 1 << day) too - [X] Now that we're using the term "workday", it makes sense to change the "sunday" CSS class to "non-workday".
Review of attachment 358085 [details] [review]: Only nits ::: src/gcal-utils.c @@ +1235,3 @@ +struct + { + gchar *territory; - Should be const gchar * - Align 'territory' with 'no_work_days' @@ +1264,3 @@ + { "SY", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } /* Syria */, + { "UG", GCAL_WEEK_DAY_SUNDAY } /* Uganda */, + { "YE", GCAL_WEEK_DAY_FRIDAY | GCAL_WEEK_DAY_SATURDAY } /* Yemen */, Still 2 spaces of indentation off.
Review of attachment 358085 [details] [review]: You also need to update the CSS in data/style/gtk-styles.css
Created attachment 358187 [details] [review] year-view: Mark working days according to locale Fix wrong assertion, code style and CSS.
Thank you so much for the patches! This new function (is_workday) will come in handy in many other places too Attachment 358187 [details] pushed as 66fa49b - year-view: Mark working days according to locale
Thank you for the last adjustments :)