GNOME Bugzilla – Bug 726119
[UI calendar] days aren't highlighted anymore
Last modified: 2014-03-21 00:20:35 UTC
Version of component: gnome-shell-3.11.91-1.fc21.x86_64 Steps to reproduce: 1. Open mini-calendar from GS Actual results: Seems events, but days not highlighted (on 3.10.x it was highlighted) Expected results: Seems events and days highlighted
The code to highlight events is there, maybe it's a theming issue? For example, does it work in classic?
Created attachment 271636 [details] [review] Calendar: force-rebuild the calendar when the events change Don't overoptimize and skip the rebuild when the month is the same, as the calendar depends on the events too. Turns out it was not a theming bug...
Just tried the patch: works like a charm, thanks!
Review of attachment 271636 [details] [review]: Reported-and-Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com> Yeah. This patch fixes this bug. Also.. Would this patch fix another bug with no events in cal ? Thank you for this patch
(In reply to comment #4) > Review of attachment 271636 [details] [review]: > > Reported-and-Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com> > > Yeah. This patch fixes this bug. Also.. Would this patch fix another bug with > no events in cal ? I don't know, It would require testing with the conditions of the other bugs...
Review of attachment 271636 [details] [review]: ::: js/ui/calendar.js @@ +424,2 @@ })); + this._update(true); Not a big fan of non-obvious bool parameters - do we actually need anything but _rebuildCalendar() from _update() here? Can't we just call _rebuildCalendar() directly wherever you would pass true?
(In reply to comment #6) > Review of attachment 271636 [details] [review]: > > ::: js/ui/calendar.js > @@ +424,2 @@ > })); > + this._update(true); > > Not a big fan of non-obvious bool parameters - do we actually need anything but > _rebuildCalendar() from _update() here? Can't we just call _rebuildCalendar() > directly wherever you would pass true? Yeah, but then sometimes we would call _rebuildCalendar() twice, I wanted to avoid that by putting all the logic in one place.
(In reply to comment #7) > Yeah, but then sometimes we would call _rebuildCalendar() twice How does forcing _rebuildCalendar() in _update() change that? There is no logic to guard against this, so if update(true) is called multiple times, the calendar is rebuild each time (just as with calling _rebuildCalendar() directly multiple times).
(In reply to comment #8) > (In reply to comment #7) > > Yeah, but then sometimes we would call _rebuildCalendar() twice > > How does forcing _rebuildCalendar() in _update() change that? There is no logic > to guard against this, so if update(true) is called multiple times, the > calendar is rebuild each time (just as with calling _rebuildCalendar() directly > multiple times). Yes, but it would be once. The twice I'm referring to would be calling _update(), and that calling _rebuildCalendar() for unrelated reasons, and then calling _rebuildCalendar() right after that. If the logic is inside _update(), this does not happen, the shouldRebuild variable is merely "more" true.
(In reply to comment #9) > Yes, but it would be once. > The twice I'm referring to would be calling _update(), and that calling > _rebuildCalendar() for unrelated reasons, and then calling _rebuildCalendar() > right after that. Ah. But what I'm saying is that I don't see how changes to events or the show-weekdate setting affects anything but the calendar, so my suggestion was to call _rebuildCalendar() directly instead (rather than in addition to _update()). Of course I may be overlooking something ...
(In reply to comment #10) > (In reply to comment #9) > > Yes, but it would be once. > > The twice I'm referring to would be calling _update(), and that calling > > _rebuildCalendar() for unrelated reasons, and then calling _rebuildCalendar() > > right after that. > > Ah. But what I'm saying is that I don't see how changes to events or the > show-weekdate setting affects anything but the calendar, so my suggestion was > to call _rebuildCalendar() directly instead (rather than in addition to > _update()). Of course I may be overlooking something ... _rebuildCalendar() will destroy the button list, but will not highlight the new one or re-grab focus, _update() does that.
(In reply to comment #9) > Yes, but it would be once. > The twice I'm referring to would be calling _update(), and that calling > _rebuildCalendar() for unrelated reasons, and then calling _rebuildCalendar() > right after that. > If the logic is inside _update(), this does not happen, the shouldRebuild > variable is merely "more" true. If you call _rebuildCalendar() and then _update(), the month will be the same so we'll fizzle the second call out.
Created attachment 271705 [details] [review] Calendar: force-rebuild the calendar when the events change Don't overoptimize and skip the rebuild when the month is the same, as the calendar depends on the events too.
Review of attachment 271705 [details] [review]: OK
(In reply to comment #13) > Created an attachment (id=271705) [details] [review] > Calendar: force-rebuild the calendar when the events change > > Don't overoptimize and skip the rebuild when the month is the same, > as the calendar depends on the events too. yeah. this patch also works for me.
Attachment 271705 [details] pushed as e117aa5 - Calendar: force-rebuild the calendar when the events change
*** Bug 726804 has been marked as a duplicate of this bug. ***