I posted a $250 bounty for whomever solves this issue: https://www.bountysource.com/issues/4253282-calendar-doesn-t-properly-handle-multi-day-events

Here's proposed solution that looks like this: https://imgur.com/rbu6ohu

The commit introduces a few new translation strings (no new strings, just put the ellipsis on the right side) to give full bi-directional text flexibility to the translators.

The bounty URL comment has some instructions to try it on stable versions. Tested on 3.12.2 and 3.10.4

Specific Commit:
https://github.com/abrauchli/gnome-shell/commit/03809aef088ae7e7b7377781ee4ecd3c4f86b930

Branch (single commit so far):
https://github.com/abrauchli/gnome-shell/tree/calendar-event-continuation

Feedback is warmly welcome

Thanks for working on this Andreas! Your code seems to work quite well. Why don't you attach it as a patch to this bug: create a patch for the most recent commit on the current branch with the command 'git format-patch @^' (or type HEAD^ instead of @^ on older versions of git).

I have just two concerns:

* When an ellipses occurs on the left side of the time, the times become misaligned.  It would be better if the times were always aligned regardless. I guess one way to fix this would be to use a separate label for the first ellipses character, and giving it a minimum width likely to be greater than the size of the character. Not sure how hard that would be; there might be a better way.

* It's good that you considered right-to-left locales when writing this patch. I wonder if RTL translators will really expect to flip the ellipses manually, though, or if this should be done programatically instead (easy to handle by checking gtk_widget_get_default_direction, though it will make the code longer). Yosef, can you advise us on this?

Created attachment 285874 [details] [review]
gnome-shell calendar: correctly display multi-day events

This is the original patch from https://github.com/abrauchli/gnome-shell/commit/03809aef088ae7e7b7377781ee4ecd3c4f86b930

Thanks for your feedback, Michael.

I did notice the alignment issue but decided to go with this version because of the (likely) increase in horizontal space needed for separate labels.
I'll make a new version then we can compare, maybe it's not as bad... especially as most people nowadays have more horizontal pixels than I do (1440px)

I was first thinking of adding the ellipses programatically but didn't know about gtk_widget_get_default_direction. I also don't know if all languages are fine with ellipses or if some would use a different character (maybe short dashes?)
Programatically setting the ellipses would actually make the code slightly shorter since if we're going with separate ellipsis labels there's no need for all those translation strings along with the comments for translators..

I'll report back when I have the new version.

Review of attachment 285874 [details] [review]:

Thanks for your patch!
I think we would want design input here. Ask on #gnome-design on IRC about this bug.
On the other hand, as Michael said the ellipsis could be added programatically and flipped if Clutter.get_default_text_direction() is RTL.
Also, I saw an issue, now the events are shown with its end date, not its start day when "This week" is shown.

::: js/ui/calendar.js
@@ +406,3 @@
+            let d2 = event2.date < begin && event2.end <= end ? event2.end : event2.date;
+            return d1.getTime() - d2.getTime();
+        });

This is not related to this patch. Can you make another one?

Thanks for your comments Carlos. It's related but you're right: I'll split the patch.

Here's what the column aligned version looks like:
https://imgur.com/n6n9Yu8

Maybe it's fine like this, but I intended to make the spacing between the ellipses and the time label smaller but couldn't figure out how. I tried adapting the css class but it seems that padding and margin are already 0. Guidance is appreciated.

Created attachment 286109 [details] [review]
Column-aligned version of the first patch

Column-aligned version of the first patch. This part does not include displaying the ending time on the ending day (see next patch)

Created attachment 286110 [details] [review]
Calender: Show end time on day of event ending

Split out this part from the first patch.

..found the css property: "spacing-columns: 6pt;" on ".events-table"
It affects the whole EventsList grid but it doesn't look to have adverse effects when removed.
Could someone confirm this please?

New screenshot:
https://imgur.com/CgZoBbY

Created attachment 286144 [details] [review]
Eliminate column spacing

Patch stylesheet to eliminiate the column spacing.
This optimizes the horizontal space usage and places the event continuation ellipses (if any) right next to the time label while still keeping them aligned.

Created attachment 286147 [details] [review]
Column-aligned version of the first event-continuation patch

Column-aligned version of the first patch. This part does not include
displaying the ending time on the ending day (see other patch)

This updated version does not ellipsize events with the allDay flag set.
The previous (column-aligned) patch would display "All Day..." for a single-day event with the allDay flag since the ending time is on midnight of the next day and thus 1ms after _getEndOfDay().

I guess the version with columns worked well for the start ellipses, but not for the end ellipses (you can see there's too much space after 2:00 and 8:00 in the screenshot).

I can try centering the time labels, then the spaces before and after are equal. Would this be acceptable?
In the column version there will, naturally, still be less space between "All Day" and "..." than between "08:00" and "..." because of the different string lengths.

I don't think that's right.

How about using the extra column for the starting ellispsis (left in LTR, right in RTL) but not for the ending ellispses, just appending (LTR) or prepending the ellipses like you did in the first patch? I think that would always create the right amount of space, right?

Indeed, good thinking: https://imgur.com/gF1I5TY

Best result so far IMO.

Created attachment 286160 [details] [review]
New column-aligned version with now only the leading ellipsis in its own column.

New column-aligned version with now only the leading ellipsis in its own column.
This version now includes the previously separate stylesheet patch needed to get rid of the column spacing.

Apart from this, the curent description still applies:
Column-aligned version of the first patch. This part does not include
displaying the ending time on the ending day (see other patch)

This updated version does not ellipsize events with the allDay flag set.
The previous (column-aligned) patch would display "All Day..." for a single-day
event with the allDay flag since the ending time is on midnight of the next day
and thus 1ms after _getEndOfDay().

Created attachment 286161 [details] [review]
Calender: Show end time on day of event ending

Same patch, rebased

Previous description still applies:
The patch changes the weekday to the ending day and
sorts the list according to to ending time on multi-day events.

This should only be accepted after the multi-day ellipsis patch
has been merged as the ellipsis in front suggests that the event
runs until "... 10:00"

OK, that looks good to me. Now we need Carlos or another gnome-shell developer to review it.

Review of attachment 286160 [details] [review]:

Overall is looking good, but most of the second patch has to be merged in here. Only the sorting change is for the second patch.
Also some style nitties.

::: js/ui/calendar.js
@@ +14,3 @@
 const MSECS_IN_DAY = 24 * 60 * 60 * 1000;
 const SHOW_WEEKDATE_KEY = 'show-weekdate';
+const EventEllipses = { NONE: 0, BEFORE: 1, AFTER: 2, BOTH: 3 };

boxpointer.js already uses unary and uses it like this:
const EventEllipses = {
   NONE: 0,
   BEFORE: 1 << 0,
   AFTER: 1 << 1,
   BOTH: ~0
}

I would use that for consistency and to make clear we are using it as unary.

@@ +60,3 @@
 }
 
+function _ellipsizeEventTime(event, begin, end) {

It's a little confusing having a "begin" and "end" if we already have event.date and event.end.
How about periodBegin or periodStart or something like that? Do you have a better name in mind?
The same for all the "begins" and "ends" in the new code =)
I would be happy if you change the _addPeriod(begin, end, ...) to _addPeriod(periodBegin, periodEnd, ...)

@@ +63,3 @@
+    let ret = EventEllipses.NONE;
+    if (event.allDay)
+        return EventEllipses.NONE;

this "if" statement could look better above the let ret = ...

@@ +754,3 @@
+        layout.attach(dayLabel, rtl ? 3 : 0, index, 1, 1);
+
+        const ELLIPSIS_CHAR = '\u2026';

Better putting it at the const list at the start of the file

@@ +757,3 @@
+        let ellipses = _ellipsizeEventTime(event, begin, end);
+        if (ellipses & EventEllipses.BEFORE) {
+            let elLabel = new St.Label({ style_class: 'events-day-time',

How about ellipsisLabel? Sounds more clear maybe?

@@ +803,2 @@
             /* Translators: Text to show if there are no events */
+            let nothingEvent = new CalendarEvent(begin, begin, _("Nothing Scheduled"), true);

since you do the change, seems that it makes more sense new CalendarEvent(begin, end, ...
Note that with the current code doesn't matter at all.

Review of attachment 286161 [details] [review]:

In overall I would like to have in this patch only the sorting change, since is the only change that I don't see related to handling right multiple day events.
The others parts I think belongs to the first patch for handling correctly multiple day events.

::: js/ui/calendar.js
@@ +381,3 @@
+            let d2 = event2.date < begin && event2.end <= end ? event2.end : event2.date;
+            return d1.getTime() - d2.getTime();
+        });

This is the only part I would put in the second patch.

@@ +748,3 @@
+                dayString = _getEventDayAbbreviation(event.date.getDay());
+            else /* show event end day if it began earlier */
+                dayString = _getEventDayAbbreviation(event.end.getDay());

This is wrong for the case of multiple day events when showing the "This week" heading, since it will show the last day instead of the start day.
Any reason why you want to show the end day for events instead of the start day? (Maybe I'm missing something)
If not, I would change nothing here.

@@ +749,3 @@
+            else /* show event end day if it began earlier */
+                dayString = _getEventDayAbbreviation(event.end.getDay());
+        } else

if one if block add a bracket, all blocks add brackets.
so or:
if
   statement
else
   statement

or:
if {
   statament
   ....
} else {
   statement
}

Also, if I didn't make it clear before, we want design input here. Make sure ask on gnome-design to aday or jimmac before. (Maybe the code needs slight changes based on the design input)

Here's some info about IRC where you'll find #gnome-design: https://wiki.gnome.org/Community/GettingInTouch/IRC

I'm not really sure this needs designer input, since it's a small change, but it never hurts to ask and they don't bite. :)

(In reply to comment #23)
> Here's some info about IRC where you'll find #gnome-design:
> https://wiki.gnome.org/Community/GettingInTouch/IRC
> 
> I'm not really sure this needs designer input, since it's a small change, but
> it never hurts to ask and they don't bite. :)

Hi Michael

At Gnome shell we are used to ask for even the smallest changes in UI like opacity, paddings, round corners etc. Most of the time is an "ok go ahead" or "um...a little more padding there and here" and that's it, so it doesn't make the process slower.

Actually, yesterday Florian Müllner was the first thing that told me: Be careful to review that bug before asking for design input, it may change and the code will need little changes.

That's why I put a warning here again =)

Great feedback, thanks for the review Carlos. I'll integrate the changes in the next iteration.

> @@ +748,3 @@
> +                dayString = _getEventDayAbbreviation(event.date.getDay());
> +            else /* show event end day if it began earlier */
> +                dayString = _getEventDayAbbreviation(event.end.getDay());
> 
> This is wrong for the case of multiple day events when showing the "This week"
> heading, since it will show the last day instead of the start day.
> Any reason why you want to show the end day for events instead of the start
> day? (Maybe I'm missing something)
I intended it that way.. Did you consider this case:
Event Foo Mo 08:00 - Fr 16:00, Today is Monday
Today "08:00... Foo"
Tomorrow "...All Day... Foo"
Rest of the week "Mo ...16:00 Foo" vs "Fr ...16:00 Foo"
The first is wrong since the event goes past Mo 16:00.

> @@ +803,2 @@
>              /* Translators: Text to show if there are no events */
> +            let nothingEvent = new CalendarEvent(begin, begin, _("Nothing
> Scheduled"), true);
> 
> since you do the change, seems that it makes more sense new
> CalendarEvent(begin, end, ...
> Note that with the current code doesn't matter at all.
I first had that and then changed back to "begin, begin". If we go with "begin, end" then the event length is dependent on the current query and may be more than one day, which causes the wrong weekday to be displayed if the ending weekday is displayed and the query is for the rest of the week. It would also cause an ellipsis for any query period long than a day.


I've had some trouble reaching people on IRC. Mostly the channel is near- or fully empty. Probably my bad, I'm a bit off with my GMT-10 time zone.) I did, however, noticed a gnome-design community on G+, are the relevant persons active there?

(In reply to comment #25)
> I've had some trouble reaching people on IRC. Mostly the channel is near- or
> fully empty. Probably my bad, I'm a bit off with my GMT-10 time zone.) I did,
> however, noticed a gnome-design community on G+, are the relevant persons
> active there?

Yeah, that time zone doesn't help; I guess peak activity would probably be in the late evening in your timezone.

Anyway, hi Allan, could you give us feedback on this design for multi-day events? There is a screenshot is comment #16.

Created attachment 286434 [details] [review]
Column-aligned version of the first event-continuation patch

with latest review changes from comment #20, #21

Created attachment 286435 [details] [review]
Sort multi-day events by ending day/time

Includes changes from review in comment #21

(In reply to comment #25)
> Great feedback, thanks for the review Carlos. I'll integrate the changes in the
> next iteration.
> 
> > @@ +748,3 @@
> > +                dayString = _getEventDayAbbreviation(event.date.getDay());
> > +            else /* show event end day if it began earlier */
> > +                dayString = _getEventDayAbbreviation(event.end.getDay());
> > 
> > This is wrong for the case of multiple day events when showing the "This week"
> > heading, since it will show the last day instead of the start day.
> > Any reason why you want to show the end day for events instead of the start
> > day? (Maybe I'm missing something)
> I intended it that way.. Did you consider this case:
> Event Foo Mo 08:00 - Fr 16:00, Today is Monday
> Today "08:00... Foo"
> Tomorrow "...All Day... Foo"
> Rest of the week "Mo ...16:00 Foo" vs "Fr ...16:00 Foo"
> The first is wrong since the event goes past Mo 16:00.
> 
I see your point now, but still I'm not sure what's the best thing to show. Let's designers decide =)
> > @@ +803,2 @@
> >              /* Translators: Text to show if there are no events */
> > +            let nothingEvent = new CalendarEvent(begin, begin, _("Nothing
> > Scheduled"), true);
> > 
> > since you do the change, seems that it makes more sense new
> > CalendarEvent(begin, end, ...
> > Note that with the current code doesn't matter at all.
> I first had that and then changed back to "begin, begin". If we go with "begin,
> end" then the event length is dependent on the current query and may be more
> than one day, which causes the wrong weekday to be displayed if the ending
> weekday is displayed and the query is for the rest of the week. It would also
> cause an ellipsis for any query period long than a day.
> 
good catch
> 
> I've had some trouble reaching people on IRC. Mostly the channel is near- or
> fully empty. Probably my bad, I'm a bit off with my GMT-10 time zone.) I did,
> however, noticed a gnome-design community on G+, are the relevant persons
> active there?
ups, sorry for that. I will ask directly to Allan, a gnome designer.

Review of attachment 286434 [details] [review]:

Code looks good now.

But we need design input to make sure designers think it's ok to have the ellipsis thing.

On the other hand,the patch have one problem thought, that was catch by Florian Mullner.
Given that days without multiple day events doesn't need ellipsis, and days with them needs ellipsis, the space between the time and the event changes between days.
I don't know a "nice" way to solve it. And I think is not that important. Let's designers or Florian decide on that one as well.

Review of attachment 286435 [details] [review]:

OK, wait for designers acceptance.

Review of attachment 286434 [details] [review]:

Oh I forgot, the commit message needs to be redone. Take a look here to do a good commit message: https://wiki.gnome.org/GnomeLove/CodeContributionWorkflow#Commit_guidelines

Review of attachment 286434 [details] [review]:

reviewed, not accept commit.

Review of attachment 286435 [details] [review]:

reviewed, not accept commit.

Created attachment 286760 [details] [review]
Calendar: Show multi-day event continuation

Rebased on master, fixed commit message

Created attachment 286761 [details] [review]
Calendar: sort multi-day events by ending day/time

Rebased on master, fixed commit message

I caught Allan on IRC yesterday and pointed him at this bug. Didn't get any verdict, but he's aware of this.

Thanks for the update, Michael. Are we still moving forward on this now that 2.14 is out of the way?

sorry for the typo, 3.14 - not trying to go back

Sorry this stalled for an entire month. :( On my to-do list for tomorrow morning is "bug every designer I can find on IRC."  Maybe Carlos can do so too.

We have approval, plus a request:

jimmac:  would be nice if it was a littl emore subtle
jimmac:  maybe same grey as the descriptions
mcatanzaro:  OK
jimmac:  but it's ok

Sorry, that refers to just the ellipses.

Great, thanks for the quick reply!

I'm not quite sure what a good way to tackle the more subtle ellipses would be.

Currently, the leading ellipsis is in it's own column (easy, css fix), while the trailing is directly attached to the time string. I looked into applying the :first-letter css selector but that doesn't seem to be recognized. Even if it were, it would still cause problems when there is no ellipsis - adding a space might be a workaround. My general feeling is that it'd be the wrong approach and is too hacky anyway.

(In reply to comment #43)
> Great, thanks for the quick reply!
> 
> I'm not quite sure what a good way to tackle the more subtle ellipses would be.
> 
> Currently, the leading ellipsis is in it's own column (easy, css fix), while
> the trailing is directly attached to the time string. I looked into applying
> the :first-letter css selector but that doesn't seem to be recognized. Even if
> it were, it would still cause problems when there is no ellipsis - adding a
> space might be a workaround. My general feeling is that it'd be the wrong
> approach and is too hacky anyway.

I think the best solution here is having a St.BoxLayout with three labels, the start ellipses, the day label, and the end label, all inside one column.
Then you can apply a css selector to the ellipses label with some gray color, and just hide the ellipses labels that are not necessary. But don't hide them with actor.hide(), since it will not reversing the space for that label and then you will have misalignments between rows; so use actor.opacity property instead to hide/show them.

Florian, what do you think?

(In reply to comment #44)
> (In reply to comment #43)
> > Great, thanks for the quick reply!
> > 
> > I'm not quite sure what a good way to tackle the more subtle ellipses would be.
> > 
> > Currently, the leading ellipsis is in it's own column (easy, css fix), while
> > the trailing is directly attached to the time string. I looked into applying
> > the :first-letter css selector but that doesn't seem to be recognized. Even if
> > it were, it would still cause problems when there is no ellipsis - adding a
> > space might be a workaround. My general feeling is that it'd be the wrong
> > approach and is too hacky anyway.
> 
> I think the best solution here is having a St.BoxLayout with three labels, the
> start ellipses, the day label, and the end label, all inside one column.
> Then you can apply a css selector to the ellipses label with some gray color,
> and just hide the ellipses labels that are not necessary. But don't hide them
> with actor.hide(), since it will not reversing the space for that label and
> then you will have misalignments between rows; so use actor.opacity property
> instead to hide/show them.

typo, reversing/reserving.

Hey Carlos,

great suggestion with the BoxLayout, I should have done that in the first place: The code is cleaner and allows for more independent styling. The new patch iteration (up next) does that and, using the stylesheet, changes the ellipses colors to the gray used for the event text (See jimmac's quote in comment #41).

Created attachment 289235 [details] [review]
Calendar: Show multi-day event continuation

This iteration uses a BoxLayout wrapping the ellipse labels and the time label.
To keep the alignment, the ellipse labels are hidden (not removed) from the box when not displayed.

Rebased against master

Created attachment 289236 [details] [review]
Calendar: sort multi-day events by ending day/time

Rebased against master, otherwise unchanged.

Do post a screenshot, please. :)

Here you go:
https://imgur.com/ISdu8d3

Review of attachment 289235 [details] [review]:

Nitpick on my part: please rewrap the commit message to ~72 characters[0].

[0] http://stackoverflow.com/questions/2119942/how-to-wrap-git-commit-comments#2120040

::: js/ui/calendar.js
@@ +14,3 @@
 const MSECS_IN_DAY = 24 * 60 * 60 * 1000;
 const SHOW_WEEKDATE_KEY = 'show-weekdate';
+const ELLIPSIS_CHAR = '\u2026';

gjs should be able to deal just fine with UTF-8 literals nowadays, so I'd expect
  let label = new St.Label({ text: '…' });
to work.

@@ +782,3 @@
+                                               y_align: Clutter.ActorAlign.START });
+        if (!(ellipses & EventEllipses.BEFORE))
+            preEllipsisLabel.opacity = 0;

I'm wondering whether using something like

  if (event.allDay || event.date >= periodBegin)
      preEllipsisLabel.opacity = 0;

isn't just as clear as the EventEllipses flags.

Review of attachment 289236 [details] [review]:

LGTM (insert comment on line length in commit message ...)

Created attachment 289323 [details] [review]
Calendar: Show multi-day event continuation

Only change: Wrap commit message at col 72

Created attachment 289324 [details] [review]
Calendar: sort multi-day events by ending day/time

Only change: Wrap commit message at col 72

Thanks for the review Florian,

I mainly opted for the escaped unicode char variant because there's another, unrelated to this patch, unicode char in the same source file that is also escaped.

Review of attachment 289324 [details] [review]:

LGTM

Review of attachment 289323 [details] [review]:

(In reply to comment #56)
> I mainly opted for the escaped unicode char variant because there's another,
> unrelated to this patch, unicode char in the same source file that is also
> escaped.

OK, makes sense. I still think the EventEllipses redirection is unnecessary, but *shrug*

Yay!

Andreas, please leave another comment here when you've submitted your claim on Bountysource.

Attachment 289323 [details] pushed as 65c136f - Calendar: Show multi-day event continuation
Attachment 289324 [details] pushed as 62b6419 - Calendar: sort multi-day events by ending day/time

Created attachment 289488 [details] [review]
Calender: Inline _ellipsizeEventTime into caller

As suggested in Comment #58, inline the _ellipsizeEventTime into caller to simplify the code.

Applies on top the previous patches which have already been merged to master.

(In reply to comment #58)
> Review of attachment 289323 [details] [review]:
> 
> (In reply to comment #56)
> > I mainly opted for the escaped unicode char variant because there's another,
> > unrelated to this patch, unicode char in the same source file that is also
> > escaped.
> 
> OK, makes sense. I still think the EventEllipses redirection is unnecessary,
> but *shrug*

Thanks for reviewing and merging, Florian. I have thus made a separate patch to address that.

(In reply to comment #59)
Thanks Michael, I submitted the claim.

Review of attachment 289488 [details] [review]:

Less code, yay!

Michael, the patch set is also suitable for the next 3.14.x release, so feel free to push to gnome-3-14 as well.

Comment on attachment 289488 [details] [review]
Calender: Inline _ellipsizeEventTime into caller

OK, approved the claim, and backported to gnome-3-14. Thanks!

*** Bug 641186 has been marked as a duplicate of this bug. ***