After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747479 - Show weekday instead of date in the create event popover when creating new event within a week
Show weekday instead of date in the create event popover when creating new ev...
Status: RESOLVED FIXED
Product: gnome-calendar
Classification: Applications
Component: Quick Add Popover
unspecified
Other Linux
: Normal normal
: 3.26
Assigned To: GNOME Calendar maintainers
GNOME Calendar maintainers
Depends on:
Blocks:
 
 
Reported: 2015-04-07 20:21 UTC by Marcus Lundblad
Modified: 2017-06-23 14:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
change the header displayed on the create event popover (13.84 KB, image/png)
2016-08-21 18:12 UTC, nj4710
  Details
This patch change popover title if the event is Today/Yesterday/Tomorrow (3.46 KB, patch)
2017-04-30 11:25 UTC, clobrano
none Details | Review
Patch v2 to change popover title if the event is Today/Yesterday/Tomorrow (3.26 KB, patch)
2017-04-30 11:32 UTC, clobrano
none Details | Review
Patch v3 to change popover title if the event is Yesterday, Today, Tomorrow or within 7 days (3.87 KB, patch)
2017-05-01 19:58 UTC, clobrano
none Details | Review
Patch v4: factored out the new code into a new function (3.06 KB, patch)
2017-05-02 11:57 UTC, clobrano
none Details | Review
patch v5: code review plus minor fix (3.16 KB, patch)
2017-05-14 08:32 UTC, clobrano
none Details | Review
Patch v6: code review and attempt to fix strings for translation (4.71 KB, patch)
2017-05-24 21:00 UTC, clobrano
none Details | Review
Patch v7: code review and changes to multiday event title (10.98 KB, patch)
2017-06-07 08:51 UTC, clobrano
none Details | Review
Patch v8: code review changes (11.19 KB, patch)
2017-06-18 20:37 UTC, clobrano
committed Details | Review
quick-add-popover: Say “in Month”, not “on Month” (1.82 KB, patch)
2017-06-23 09:39 UTC, Daniel Boles
rejected Details | Review

Description Marcus Lundblad 2015-04-07 20:21:18 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".
Comment 1 nj4710 2016-08-21 18:12:27 UTC
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.
Comment 2 nj4710 2016-08-24 16:12:13 UTC
Can someone please tell me the status of this bug?
Comment 3 Georges Basile Stavracas Neto 2017-04-17 19:54:25 UTC
(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.
Comment 4 clobrano 2017-04-30 11:24:04 UTC
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.
Comment 5 clobrano 2017-04-30 11:25:52 UTC
Created attachment 350756 [details] [review]
This patch change popover title if the event is Today/Yesterday/Tomorrow
Comment 6 clobrano 2017-04-30 11:32:23 UTC
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.
Comment 7 Georges Basile Stavracas Neto 2017-05-01 14:49:24 UTC
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 =)
Comment 8 clobrano 2017-05-01 19:58:13 UTC
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?
Comment 9 Georges Basile Stavracas Neto 2017-05-01 20:57:10 UTC
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?
Comment 10 clobrano 2017-05-02 06:47:31 UTC
Yep, maybe replacing the switch with a simple if/else cascade would have been better. I'll submit the new version ASAP.

Thanks
Comment 11 clobrano 2017-05-02 11:57:21 UTC
Created attachment 350863 [details] [review]
Patch v4: factored out the new code into a new function
Comment 12 Georges Basile Stavracas Neto 2017-05-04 15:28:33 UTC
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")
Comment 13 Georges Basile Stavracas Neto 2017-05-04 15:28:41 UTC
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")
Comment 14 clobrano 2017-05-05 09:42:37 UTC
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.
Comment 15 clobrano 2017-05-14 08:32:23 UTC
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"
Comment 16 Georges Basile Stavracas Neto 2017-05-16 01:50:56 UTC
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
Comment 17 clobrano 2017-05-16 20:00:41 UTC
I agree with you. I'll update the patch and fix the other points
Comment 18 Piotr Drąg 2017-05-16 20:20:37 UTC
(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.
Comment 19 clobrano 2017-05-24 21:00:10 UTC
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.
Comment 20 Georges Basile Stavracas Neto 2017-05-27 14:34:11 UTC
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?
Comment 21 Piotr Drąg 2017-05-27 14:49:13 UTC
(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.
Comment 22 clobrano 2017-05-27 16:59:52 UTC
> 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
Comment 23 clobrano 2017-05-28 09:35:19 UTC
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?
Comment 24 Georges Basile Stavracas Neto 2017-06-01 19:29:36 UTC
(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.
Comment 25 clobrano 2017-06-05 12:38:08 UTC
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?
Comment 26 clobrano 2017-06-07 08:51:10 UTC
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.
Comment 27 Georges Basile Stavracas Neto 2017-06-18 18:12:23 UTC
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
Comment 28 Piotr Drąg 2017-06-18 18:21:27 UTC
"%s" almost certainly shouldn’t be marked for translation.
Comment 29 clobrano 2017-06-18 20:37:49 UTC
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​
Comment 30 Georges Basile Stavracas Neto 2017-06-18 21:02:40 UTC
(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.
Comment 31 Piotr Drąg 2017-06-18 21:22:13 UTC
(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
Comment 32 clobrano 2017-06-19 07:06:31 UTC
> _("%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?
Comment 33 Piotr Drąg 2017-06-19 14:39:49 UTC
Is there anything that we as translators can do with a string like "%s" except for copying it as-is?
Comment 34 Georges Basile Stavracas Neto 2017-06-23 06:59:58 UTC
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.
Comment 35 Georges Basile Stavracas Neto 2017-06-23 07:02:09 UTC
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
Comment 36 clobrano 2017-06-23 07:11:28 UTC
Awesome! Thank you all :)
Comment 37 Daniel Boles 2017-06-23 09:23:50 UTC
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
Comment 38 Daniel Boles 2017-06-23 09:29:02 UTC
(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.
Comment 39 Daniel Boles 2017-06-23 09:39:04 UTC
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.
Comment 40 Daniel Boles 2017-06-23 14:28:21 UTC
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.