501244 - Activating or deactivating any calendar clears the marker of the selected day

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Wolfgang Beyer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090612 Calendar/1.0pre

Changing the status of any calendar in the "calendar" window from hidden to displayed or vice versa clears the marker of the marked day. This happens only in the "Multiweek View" and "Month View" but not in the "Week View". The marker also reappears if the "View" is changed. So the actual behaviour is not consistent.

This behaviour also is troublesome if the user has defined several calendars, which he sometime hides or displays again.

Reproducible: Always

Steps to Reproduce:
1. Select "Multiweek View" or "Month View" 
2. Make sure that some day is selected at all by clicking on it or by pressing "Today" 
3. Change the visibility status of an arbitrary calendar by clicking in its checkbox in the "calendar" window on the left.

Actual Results:  
The marker of the selected day disappears.

Expected Results:  
The selected day should be displayed as selected/marked.

Comment 1

[Martin Schröder [:mschroeder]]

Confirmed using recent Lightning 1.0pre nightly build.

Comment 2

[Decathlon]

Attachment: patch

This works but is it too simple?

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 386518 [details] [diff] [review]
patch

Yes, I think we should rather take care somewhere inside the refresh call. You could try tracing where the marker goes away and then either changing that code to keep the day selected, or if thats cludgy at least restore the selected day more close to where it is unselected.

Comment 4

[Decathlon]

Attachment: patch - v2

The code that deletes the selected day is inside relayout() method in calendar-month-base-view binding [1].
The comment there explicitly says that the selectedDay it's going to be deleted because "it won't be valid after relayout", but actually it's still valid if the relayout has been caused by the activation/deactivation of a calendar and in many others cases as well.

I think it should be possible to delete those code lines (and fixing the bug) without damages because it seems that when the selected day has to change, it is  explicitly set to a new one, and in the other cases there isn't any need to delete the marker.

The relayout() in *month view* is called explicitly here:
http://mxr.mozilla.org/comm-central/search?string=.relayout&find=calendar-month-view.xml&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
that are cases where the selected day doesn't change (methods addItem and setAttribute).

relayout() is also called from calendar-base-view.xml file by the refresh() method (via popRefreshQueue [5]) and this happens in the following cases:

- activating/deactivating a calendar (*our bug*)
onCalendarAdded/onCalendarRemoved [7] -> onEndBatch() -> refresh() -> ... relayout()

- changing month (navigation buttons or scroll the view)
moveView() -> goToDay() -> setDateRange()/showDate() -> refresh() -> ... relayout()

- going to a specific date (minimonth or selected element in unifinder)
goToDay() -> showDate -> setDateRange() -> refresh() -> ... relayout()

but in the last two cases, the selected day is explicitly set to a new/old value after the relayout inside goToDay [2] and inside moveView [3], so it doesn't matter if we don't delete the selected day in the relayout().

The same happens for others refresh, like those related to preferences [6] (start the week on..., workweek days, etc.) or categories. All call the refreshView() method [4] and again goToDay() where the argument is exactly the selected day.


What's your opinion Philipp? Is something missing in the previous analysis?


[1]  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-month-view.xml#806
[2]  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-views.xml#240
[3]  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-views.xml#282
[4]  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-base-view.xml#702
[5]  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-base-view.xml#458
[6]  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-month-view.xml#596
[7]  http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-base-view.xml#279

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 504232 [details] [diff] [review]
patch - v2

I've doublechecked and fully agree to your analysis. Good work! r=philipp

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/c4e01e8c0649>
-> FIXED

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/c4ae263ae514>
a=philipp