GNOME Bugzilla – Bug 641383
Calendar menu changes height with some months
Last modified: 2013-03-04 18:00:07 UTC
Some months need 6 lines to be displayed, while others only need 5 (compare for example April and May 2011); 4 weeks also exists (see February 2010). When viewing a month with a different number of lines than the current one, the menu changes height, which doesn't feel good. Instead, a kind of size group should be used so that padding fills the free space when a month with only 3 or 5 weeks is shown.
I agree that this would be more elegant.
Created attachment 227077 [details] [review] Calendar: force all months at the same height Add extra hidden weeks to avoid having the menu change height when navigating it.
Created attachment 227080 [details] [review] Calendar: freeze updates when the event source is loading Check if the event source is currently doing an async call, and prevent UI updates in that case. This avoids a flash of "No updates" when switching months. Also, some general code cleanups (and possible memory leak fixes) around event sources. Another similar issue, this time with events and for the horizontal direction.
Review of attachment 227077 [details] [review]: ::: js/ui/calendar.js @@ +555,3 @@ let iter = new Date(beginDate); let row = 2; + // We want to show always 6 weeks (to keep the calendar menu at the same 6 weeks? @@ +558,3 @@ + // height if there are no events), so we pad it with hidden days from the + // following month. + let nRows = 8; Unused. @@ +559,3 @@ + // following month. + let nRows = 8; + let finished = false; I'd prefer a better variable name for this. @@ +560,3 @@ + let nRows = 8; + let finished = false; + while (row < 8) { 8 weeks? What's going on?
Review of attachment 227080 [details] [review]: I'd prefer the code cleanups in a separate patch; otherwise looks good.
There are months that are displayed with 6 weeks (Dec 2012 is an example). 8 rows is 6 weeks + the table header + the navigation bar. I should probably explain it in a comment. Btw, I'm not sure I personally like this solution, it adds empty space at the end of the calendar, and without the events pane it just looks odd.
(In reply to comment #6) ... > Btw, I'm not sure I personally like this solution, it adds empty space at the > end of the calendar, and without the events pane it just looks odd. Can you post some screenshots?
Sure
Created attachment 227810 [details] Calendar menu with 5 weeks month
Created attachment 227811 [details] Calendar menu with 6 weeks month
Thanks Giovanni. I see the problem... I was wondering about reducing the padding below the calendar, or removing the separator. Not sure about that now though. Jimmac - what do you think?
I personally prefer the menu changing dimensions to introducing these gaps.
(In reply to comment #12) > I personally prefer the menu changing dimensions to introducing these gaps. I'm inclined to agree. If there's no other solution, we should probably close this as notabug.
What Windows 7 does is always show the Calendar with 6 weeks of context, even if one of those weeks is entirely grayed out. We could make the "gray out" more obvious, and always show 6 weeks.
(In reply to comment #14) > What Windows 7 does is always show the Calendar with 6 weeks of context, even > if one of those weeks is entirely grayed out. We could make the "gray out" more > obvious, and always show 6 weeks. That would work.
I double checked on a 4-week month (like February 2009), and it centers the four weeks inside one week of context on either side.
Created attachment 230565 [details] [review] Calendar: force all months at the same height Add one or two weeks from nearby months to avoid changing the menu height when navigating it.
Created attachment 230566 [details] [review] Calendar: freeze updates when the event source is loading Check if the event source is currently doing an async call, and prevent UI updates in that case. This avoids a flash of "No updates" when switching months. Also, some general code cleanups (and possible memory leak fixes) around event sources.
Screenshot?
Created attachment 231219 [details] 6 weeks
Created attachment 231221 [details] 4 weeks
lgtm
Review of attachment 230565 [details] [review]: This leaves some months like April 2012 looking visually unbalanced because it starts on the beginning of the first row and then ends with row nr. 6 empty and row nr. 5 having only 2 days. I think a better approach is to add the empty row, when it's needed, as the first one if the number of days to the week start is less than the number of days until the week end. E.g. for this month it would be (0 < 5) .
Btw, I just looked at GtkCalendar and gnome-calendar. GtkCalendar has no special handling for 4 weeks and forces everything to 6 weeks by adding padding at the end. gnome-calendar does something very fancy, in that it introduces two half-height rows for 5 weeks, so that the content is always centered. It is probably desirable to have consistency here, so it would be nice if designers could agree on a common pattern to be applied across all GNOME calendars.
(In reply to comment #24) > Btw, I just looked at GtkCalendar and gnome-calendar. > GtkCalendar has no special handling for 4 weeks and forces everything to 6 > weeks by adding padding at the end. > gnome-calendar does something very fancy, in that it introduces two half-height > rows for 5 weeks, so that the content is always centered. > > It is probably desirable to have consistency here, so it would be nice if > designers could agree on a common pattern to be applied across all GNOME > calendars. The approach being used here is what I've advised for gnome-calendar. (I'm not sure that the half-row approach works tbh.) One thing I did notice when testing this is that it can be difficult to identify the month when their are events for visible days in the previous and next months. It would be good if we could add a separate style for these days, so we can dim them.
Created attachment 231777 [details] [review] Calendar: dim days from other months, even if they have events Invert the order of the declarations, so those for calendar-other-month-day take priority over those for calendar-day-with-events. Patches are still missing code review, btw...
(In reply to comment #26) > Created an attachment (id=231777) [details] [review] > Calendar: dim days from other months, even if they have events ... Thanks for working on this, Giovanni. I probably wasn't clear before (sorry): I think that we will need an additional .calendar-other-month-day-with-events style for this.
Make it three separate style classes: .calendar-day.other-month.has-events
source(In reply to comment #27) > (In reply to comment #26) > > Created an attachment (id=231777) [details] [review] [details] [review] > > Calendar: dim days from other months, even if they have events > ... > > Thanks for working on this, Giovanni. I probably wasn't clear before (sorry): I > think that we will need an additional .calendar-other-month-day-with-events > style for this. Style classes can be combined, like .calendar-other-month-day.calendar-day-with-events. I just did the move because I didn't think we needed another color for those days, but maybe it's better if you handle this, as you understand visual design more than me.
(In reply to comment #26) > Patches are still missing code review, btw... Rui reviewed the first patch, and I'm not sure which patches we should use, here. Please reattach. (In reply to comment #28) > Make it three separate style classes: .calendar-day.other-month.has-events And please update for this feedback as well.
(In reply to comment #30) > (In reply to comment #26) > > Patches are still missing code review, btw... > > Rui reviewed the first patch, and I'm not sure which patches we should use, > here. Please reattach. All of them are to be used. > (In reply to comment #28) > > Make it three separate style classes: .calendar-day.other-month.has-events > > And please update for this feedback as well. There is nothing to there: style classes are already separated, and adding a .calendar-day-other-month.calendar-day-with-events selector would be redundant with .calendar-other-month-day - unless I changed the color, but I honestly don't know what color to use. And no, I won't change the code to use 'other-month' or 'has-events' as style classes, they don't make sense on their own, they are less readable than calendar-day-* and they require combining selectors, which makes the style priorities more complex for no practical gain.
Review of attachment 230565 [details] [review]: ::: js/ui/calendar.js @@ +561,3 @@ + if (this._weekStart == beginDate.getDay() && + beginDate.getMonth() == 1 && + ((year % 4) != 0 || ((year % 100) == 0 && (year % 400) != 0))) { Can we not use some of the other logic, like that in _getCalendarWeekForDate, to determine if a month has four weeks?
Created attachment 233889 [details] [review] Calendar: force all months at the same height Add one or two weeks from nearby months to avoid changing the menu height when navigating it.
Review of attachment 230566 [details] [review]: Can you split out some of the cleanups and memory leak fixes here? It's sort of difficult to review with everything squashed together. ::: js/ui/calendar.js @@ +581,3 @@ let rtl = button.get_text_direction() == Clutter.TextDirection.RTL; + if (this._eventSource.isDummy) This seems silly to me. I don't see why we should prevent clicking on the day if we don't have an event source. I know it's a sort of pre-existing thing, but...
Review of attachment 231777 [details] [review]: OK.
Review of attachment 233889 [details] [review]: OK.
(In reply to comment #34) > Review of attachment 230566 [details] [review]: > > ::: js/ui/calendar.js > @@ +581,3 @@ > let rtl = button.get_text_direction() == > Clutter.TextDirection.RTL; > > + if (this._eventSource.isDummy) > > This seems silly to me. I don't see why we should prevent clicking on the day > if we don't have an event source. I know it's a sort of pre-existing thing, > but... Because the events pane is invisible if isDummy, so clicking has no effect.
Created attachment 238003 [details] [review] Calendar: freeze updates when the event source is loading Check if the event source is currently doing an async call, and prevent UI updates in that case. This avoids a flash of "No updates" when switching months. Also, some general code cleanups (and possible memory leak fixes) around event sources.
Created attachment 238004 [details] [review] Calendar: clean up code by always having an event source Instead of sometimes having an event source and sometimes not, use the empty event source when the session mode says the calendar is disabled. This way, the code can assume an event source object and avoid checks.
Review of attachment 238004 [details] [review]: OK.
Review of attachment 238003 [details] [review]: Commit message needs update, but OK.
I'll assume no freeze break is needed, given that it's a small bug fix, it has no documentation impact, and you marked it acn rather than acf.
Attachment 231777 [details] pushed as ec39aa3 - Calendar: dim days from other months, even if they have events Attachment 233889 [details] pushed as 69cdc5a - Calendar: force all months at the same height Attachment 238003 [details] pushed as 443fe81 - Calendar: freeze updates when the event source is loading Attachment 238004 [details] pushed as beb0fdf - Calendar: clean up code by always having an event source