The relayout of the month view is being executed twice

RESOLVED FIXED in 2.2

Status

Calendar
Calendar Views
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Decathlon, Assigned: Decathlon)

Tracking

Lightning 1.7

Details

Attachments

(1 attachment, 1 obsolete attachment)

677 bytes, patch
Decathlon
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Steps to reproduce:
put a cal.ERROR() or a LOG() just inside the month view relayout function:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-month-view.xml#694

switch to the month view; change month or scroll the month view.

In the error console you can see that the relayout is being called twice.
The same doesn't happen nor for the multiweek view that uses the same relayout, neither for the others views that use the relayout in calendar-multiday-view.xml file.
(Assignee)

Comment 1

5 years ago
Created attachment 667337 [details] [diff] [review]
patch

Inside the function goToDay ("calendar-month-view" binding) there is a call to the function showDate()
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-month-view.xml#609
that is useless because repeats two actions already executed in the function (included the second call to the relayout).
In another way we could delete the lines above and below the call to showDate() but in this way the code is similar to the others "goToDay" functions in the file.
Attachment #667337 - Flags: review?(philipp)
Comment on attachment 667337 [details] [diff] [review]
patch

(In reply to Decathlon from comment #1)

> In another way we could delete the lines above and below the call to
> showDate() but in this way the code is similar to the others "goToDay"
> functions in the file.
I'd actually prefer removing the other two lines, since its exactly what showDate does. If the other functions do the same thing, then those could be changed too.

r=philipp
Attachment #667337 - Flags: review?(philipp) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 687498 [details] [diff] [review]
patch with showDate()

This patch uses showDate().

(In reply to Philipp Kewisch [:Fallen] from comment #2)
> I'd actually prefer removing the other two lines, since its exactly what
> showDate does. If the other functions do the same thing, then those could be
> changed too.

Using showDate() in the other cases is not directly possible without changing his implementation in the calICalendarView interface.

Actually the showDate() methods seem never used, apart from the month-view case considered in this patch (don't know about adds-on):

http://mxr.mozilla.org/comm-central/search?string=showDate&case=on&find=%2Fcalendar%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

where the line 226 is the one involved in this patch. The only other line which uses showDate() is
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-base-view.xml#403
but actually it appears inside the function goToDay() that is overlayed by the the respective goToDay() functions inside the extended bindings for each views (file calendar-views.xml) and no one uses showDate():

http://mxr.mozilla.org/comm-central/search?string=%22goToDay%22&find=%2Fcalendar%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

even the showDate() inside calendar-multiday-view.xml (that is a bit more elaborated) is never used:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-multiday-view.xml#2722
if I've tracked it correctly, it has been added with bug 293549 but in bug 319701 the calls to showDate() have been changed with calls to goToDay() functions.

All the functions "goToDay()", for all the view types, finish with the couple of instructions:

 this.setDateRange(date1, date2);
 this.selectedDay = date3;
 
that should be suitable to build a showDate() function like that one used in month-view, but the implementation in the calICalendarView interface should be changed in order to accept three parameters instead of only one. (Maybe should the calICalendarView interfce have a goToDay() element instead of a showDate() one ?)

Philipp, what's your opinion? It's worth trying to sort out showDate() functions or do I have simply to check-in the patch?
Attachment #667337 - Attachment is obsolete: true
Attachment #687498 - Flags: review+
(Assignee)

Comment 4

5 years ago
I think the patch can be pushed anyway in the repository.
I leave the bug open if we want to dig into the showDate() functions (see comment #3), otherwise we can close it as it is.


Pushed to comm-central changeset 769d60c09681


Philipp, when you have time take a look to comment #3 and let me know your decision.
Its not that important, lets close this bug as is :-) Thanks for investigating!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1

Updated

4 years ago
Target Milestone: 2.1 → 2.2
You need to log in before you can comment on or make changes to this bug.