GNOME Bugzilla – Bug 747479
Show weekday instead of date in the create event popover when creating new event within a week
Last modified: 2017-06-23 14:28:33 UTC
It might be nice to instead of show the date like: "New event on April 07" it would show it like "New event today", "New event tomorrow", "New event on Thursday" when the selected date is within one week from today. I'm not sure if this would be feasable for tranlations, i.e. if all languages could be assumed to have a notion of "tomorrow".
Created attachment 333832 [details] change the header displayed on the create event popover Hi, I would like to work on this enhancement. I have implemented the change for current date for now. Please let me know how does this look.
Can someone please tell me the status of this bug?
(In reply to nj4710 from comment #2) > Can someone please tell me the status of this bug? Hi, you need to share the patch for us to check. I'm sorry it took so long to reach this bug.
Hi, if the original proposer for this improvement is not working on it anymore, I'd like to give it a try. The patch I propose addresses the "New event Today/Tomorrow/Yesterday" part of the feature. If this looks good, I'd like to discuss a bit more about the other part related to the usage of the week name for the header.
Created attachment 350756 [details] [review] This patch change popover title if the event is Today/Yesterday/Tomorrow
Created attachment 350757 [details] [review] Patch v2 to change popover title if the event is Today/Yesterday/Tomorrow Sorry, in the previous patch I forgot some old code.
Review of attachment 350757 [details] [review]: Nice patch! I think you can improve it by handling not only today/tomorrow/yesterday, but also handling the next week days. Here's an example: https://git.gnome.org/browse/gnome-todo/tree/plugins/scheduled-panel/gtd-panel-scheduled.c#n80 The commit message is very good =)
Created attachment 350821 [details] [review] Patch v3 to change popover title if the event is Yesterday, Today, Tomorrow or within 7 days Thank you :) and thanks for the suggestion. What do you think about this change?
Review of attachment 350821 [details] [review]: Thanks again for the patch. One last thing: now that you applied those changes, I think that the code looks way too complex. Could you please factor out that code into a separate function?
Yep, maybe replacing the switch with a simple if/else cascade would have been better. I'll submit the new version ASAP. Thanks
Created attachment 350863 [details] [review] Patch v4: factored out the new code into a new function
Review of attachment 350863 [details] [review]: One comment about translations. ::: src/gcal-quick-add-popover.c @@ +326,3 @@ + date_name = get_name_for_date (self->date_start); + title_date = g_strdup_printf (_("New Event %s, %s – %s"), This is not translation friendly. Ideally, there'd be: - One string per special case ("New Event Today", "New Event Tomorrow", "New Event Yesterday") - One translation for week days ("New Event on %s") - Then, one translation for the merged strings ("%1$s, %2$s - %3$s")
I see, one question though, it is just a matter of string formatting or even the string "New Event next <weekday>" is hard to translate? Honestly, I would prefer "New Event next Monday" to "New Event on Monday", because in this patch weekdays are used only for future events and the latter message does not clarify it well in my opinion.
Created attachment 351807 [details] [review] patch v5: code review plus minor fix Hi, besides making the changes discussed in the code review, I also had to make a minor fix. The span for using the string "New event next <weekday name>" was too short by 1 day, so that, for example, if Today were Monday May 15, setting an event for May 22th (the next Monday), the pop up wouldn't have shown "New Event next Monday", but just "New event on May 22"
Review of attachment 351807 [details] [review]: After testing this patch for a while, I concluded that it'd be better if this behavior happened everywhere (not only Week view). Can you please make the code do that regardless of the view? Also, some comments below: ::: src/gcal-quick-add-popover.c @@ +215,3 @@ +static gchar* +get_name_for_new_event (GDateTime *event_date) Rename to "get_date_string_for_date()" @@ +218,3 @@ +{ + g_autoptr (GDateTime) today; + gchar *event_date_name = NULL; Assign to NULL below, together with 'today' @@ +224,3 @@ + if (g_date_time_get_year (today) == g_date_time_get_year (event_date)) + { + gint n_days = g_date_time_get_day_of_year (today) - g_date_time_get_day_of_year (event_date); This won't work on December 31 and January 1st. @@ +240,3 @@ + else if (n_days < -1 && n_days > -8) + { + g_autofree gchar *week_name = g_date_time_format (event_date, "%A"); Declare var in one line, then blank line, then assign it to g_date_time_format() @@ +249,3 @@ + } + + if (event_date_name == NULL) Can be just "!event_date_name" @@ +251,3 @@ + if (event_date_name == NULL) + { + g_autofree gchar *date = g_date_time_format (event_date, "%B %d"); Ditto about one line → blank line → assignment
I agree with you. I'll update the patch and fix the other points
(In reply to Georges Basile Stavracas Neto from comment #13) > This is not translation friendly. Ideally, there'd be: > > - One string per special case ("New Event Today", "New Event Tomorrow", > "New Event Yesterday") > - One translation for week days ("New Event on %s") > - Then, one translation for the merged strings ("%1$s, %2$s - %3$s") It’s closer to being ideal, but it’s not there yet. In the second case, some languages would translate it differently for different days of the week. For example, in Polish: “New Event on Tuesday” is ‘Nowe wydarzenie we wtorek” “New Event on Wednesday” is ‘Nowe wydarzenie w środę” Note not only different prepositions, but also different grammatical cases. The only solution that I see is to have seven separate strings for days of the week. (In reply to Georges Basile Stavracas Neto from comment #16) > + g_autofree gchar *date = g_date_time_format (event_date, "%B %d"); > This needs to be translatable, which is bug #780629.
Created attachment 352534 [details] [review] Patch v6: code review and attempt to fix strings for translation Here is the new version. Changes according to code review and attempt to fix the strings for simplify the translation.
Review of attachment 352534 [details] [review]: I'm sorry this is getting so complex. But thanks to your work, we're finally reaching a point where the translation bits are correct. About the code: looks like your get_date_string_for_date() ignores the end date. It should have 2 arguments: GDateTime *start, GDateTime *end. Some other comments below. ::: src/gcal-quick-add-popover.c @@ +217,3 @@ +get_date_string_for_date (GDateTime *event_date) +{ + GDate *today, *event_day; You can use stack-based GDates here (not pointers). Make sure to clean them before using, with g_date_clear (&date); @@ +234,3 @@ + + g_date_free (today); + g_date_free (event_day); If you use stack GDates, you can remove this. @@ +259,3 @@ + "New Event next Sunday", + NULL + }; You have to mark these strings as translatable string using N_(). See https://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS @@ +285,3 @@ + "New Event on December", + NULL + }; Hm, when adding an event at a different month, I'd rather have the current behavior. You'd have to consider: - New Event on <month name>, day1 - day2 (start and end dates in the same month) - New Event on <month1 name> day1 - <month2 name> day2 (start and end dates in different months) - New Event on <year1> <month1 name> day1 - <year2> <month2 name> day2 (start and end dates in different years) So I propose you to have 3 arrays of translatable strings: 1. This one (but using N_() and with printf-vars, e.g. "New Event on May, %1$s - %2$s") 2. Another for dates in different months (with N_(), in a printf-like format, e.g. "New Event on %1$s %2$d - %3$s %4$d") 3. Another for dates in different years (with N_(), in a printf-like format, e.g. "New Event on %1$d %2$s %3$d - %4$d %5$s %6$d") Piotr, do you think this will cover all the necessary translation needs?
(In reply to Georges Basile Stavracas Neto from comment #20) > Piotr, do you think this will cover all the necessary translation needs? I think so, yes. Also some notes: • Are you sure these should be translatable: _("%s") ? • Strings with variables could use translator comments, especially “%s, %s – %s” and similar, which provide no context by themselves. • _("%s %d") could use the same strftime treatment from bug #780629.
> I'm sorry this is getting so complex. No worries, I'm happy to learn some new stuff (no experience with translation, so far). I'll be back with a new patch version
I've got some doubts about the last part of the review. When the event span is several days, get_date_string_for_date isn't actually used (a more descriptive name could be get_date_string_for_single_date in this sense), and it's the reason why the event's end date isn't considered. So the behavior for "multiday" events is still the one you'd prefer. However, I can change the patch in order to have popups like "New event from Tomorrow to June 29" or "New Event from Today to next Thursday", which is a bigger one, but doable I think. Is this what you meant, or have I misunderstood something?
(In reply to clobrano from comment #23) > I've got some doubts about the last part of the review. > > When the event span is several days, get_date_string_for_date isn't actually > used (a more descriptive name could be get_date_string_for_single_date in > this sense), and it's the reason why the event's end date isn't considered. > So the behavior for "multiday" events is still the one you'd prefer. > > However, I can change the patch in order to have popups like "New event from > Tomorrow to June 29" or "New Event from Today to next Thursday", which is a > bigger one, but doable I think. > > Is this what you meant, or have I misunderstood something? Indeed, that sounds good! Please go ahead and implement this.
In multidays events, considering popups like "New Event from Yesterday to June 21", I was thinking of using two substrings for start and end dates, something like: "New Event %s %s" and then "from Yesterday" as a whole string (or "next Monday" in case of weeknames), and "to %1$s %2$s" for the end date, to be filled with Month name and number of the day. The same would apply for start date substring, that would have the format "from %1$s %2$s". I'm not too familiar with translation, but I expect that the preposition "from" or "to" should give enough context, while the printf-like vars should let use Angloamerican date format "<month-name> <number>" as well as the opposite one (which is used in Italian also). What do you think?
Created attachment 353298 [details] [review] Patch v7: code review and changes to multiday event title Patch v7 - fixed patch v6 code review - extended new Popup Headers to multiday events Note about the translation: For multiday events, I splitted the title event in three parts 1. New Event 2. from <date> 3. to <date> hopefully, the particles from/to should be enough for translators to come up with an appropriate translation. For events in the current week, the strings are constants, like "from Today", "to next Tuesday". For events outside the current week, the strings have the format "from %1$s %2$s" and "to %1$s %2$s", and the code provides in month name and number of day in the month in this order. Translators should be able to tweak the order of this two variables according to the language, but still, I have little experience in translations, so I might be wrong here. Said so, I wonder if the multiday title can become too much. I'm thinking about events like the following: "New Event from next Tuesday to next Friday" in my opinion, this titles aren't very clear, maybe because there isn't a precise reference to any date. An improvement, could be to keep the end event date in the old fashion when start date isn't Yesterday/Today/Tomorrow: "New Event from next Tuesday to June 9" don't know, maybe I'm just overthinking, what's your opinion? By the way, it became a very long patch, I hope to haven't missed anything.
Review of attachment 353298 [details] [review]: This patch is fantastic! A few very minor issues below. Also, I think the commit title should be updated to "quick-add-popover". Thanks!!! ::: src/gcal-quick-add-popover.c @@ +232,3 @@ + + g_date_clear (&today, 1); + g_date_clear (&event_day, 1); You don't need to clear these GDates, they're in the stack and are wiped out once the function returns. @@ +237,3 @@ + +static gchar* +get_date_string_for_multiday (GDateTime *start, GDateTime *end) Nit: One parameter per line please @@ +298,3 @@ + { + start_date_str = g_strdup_printf (_("%s"), + start_date_weekdays_strings [g_date_time_get_day_of_week (start) - 1]); Nit: put start_date_weekday_strings[...] in a local variable, so you can keep the indentation. @@ +333,3 @@ + { + end_date_str = g_strdup_printf (_("%s"), + end_date_weekdays_strings [g_date_time_get_day_of_week (end) - 1]); Ditto about local variable @@ +352,3 @@ + date_string = g_strdup_printf (_("New Event %s %s"), + start_date_str, + end_date_str); I believe this should ise the same position technique of the previous ones (i.e. "New Event %1$s %2$s") @@ +414,3 @@ + string_for_date = g_strdup_printf (_("%s %d"), + event_month_names [g_date_time_get_month (day) - 1], + g_date_time_get_day_of_month (day)); Ditto about local variable (also, don't break parameter alignment) @@ +441,3 @@ { + title_date = get_date_string_for_multiday (self->date_start, + g_date_time_add_days (self->date_end, -1)); Nit: params can be at the same line
"%s" almost certainly shouldn’t be marked for translation.
Created attachment 354007 [details] [review] Patch v8: code review changes > This patch is fantastic! A few very minor issues below. Also, I think the > commit title should be updated to "quick-add-popover". I'm glad you like it :D, agreed with the title > @@ +352,3 @@ > + date_string = g_strdup_printf (_("New Event %s %s"), > + start_date_str, > + end_date_str); > > I believe this should ise the same position technique of the previous ones > (i.e. "New Event %1$s %2$s") I'm not against this, it just adds more flexibility IMHO, but is there really any chance that a translator might need to invert start and end date? Anyway, as I said, I'm totally ok with the changes. > Thanks!!! Thank you for the reviews
(In reply to clobrano from comment #29) > I'm not against this, it just adds more flexibility IMHO, but is there > really any > chance that a translator might need to invert start and end date? > > Anyway, as I said, I'm totally ok with the changes. There's always a language that does whatever you can think of. Think of RTL languages, where the sentence order is the inverse. Also, what Piotr said; avoid _("%s %d") translations.
(In reply to Georges Basile Stavracas Neto from comment #30) > Also, what Piotr said; avoid _("%s %d") translations. No, I specifically mean _("%s") (that is no longer in v8). _("%s %d") is, as I understand it, for the month name and the day number, which order should be translatable. See https://bugzilla.gnome.org/show_bug.cgi?id=780629
> _("%s %d") is, as I understand it, for the month name and the day number, which order should be translatable. The string for %s here isn't only the month name though, but a string like "New Event on January". Is this ok as well?
Is there anything that we as translators can do with a string like "%s" except for copying it as-is?
Comment on attachment 354007 [details] [review] Patch v8: code review changes Looking very good. Can't thank you enough for figuring this quite complicated task out.
Pushed with a minor couple of changes. I guess that, with this patch, the bug is finally fixed, so I'm closing it as such. Thanks! Attachment 354007 [details] pushed as 9033d98 - quick-add-popover: improve event title in header
Awesome! Thank you all :)
Review of attachment 354007 [details] [review]: Just a couple of little things I noticed while browsing git. ::: src/gcal-quick-add-popover.c @@ +242,3 @@ + + gchar *start_date_weekdays_strings [] = { + N_("from next Monday"), Couldn't we get names of day, month, etc from somewhere else where they are already used? @@ +402,3 @@ + gchar *event_month; + gchar *event_month_names [] = { + N_("New Event on January"), This "on" should be changed to "in". Things happen at times, on days, in months. :P
(In reply to Daniel Boles from comment #37) > Couldn't we get names of day, month, etc from somewhere else where they are > already used? Sorry, never mind: I just realised that would be problematic for translations and then read Georges' comments on that topic.
Created attachment 354302 [details] [review] quick-add-popover: Say “in Month”, not “on Month” This is the right way to phrase that an event occurs in a given month.
Review of attachment 354302 [details] [review]: I'm an idiot and just read the rest of the code, to see how this actually works. Sorry for the noise.