Last Comment Bug 797268 - The relayout of the month view is being executed twice
: The relayout of the month view is being executed twice
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Calendar Views (show other bugs)
: Lightning 1.7
: All All
: -- normal (vote)
: 2.2
Assigned To: Decathlon
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-02 23:12 PDT by Decathlon
Modified: 2013-02-07 11:09 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (610 bytes, patch)
2012-10-02 23:19 PDT, Decathlon
philipp: review+
Details | Diff | Splinter Review
patch with showDate() (677 bytes, patch)
2012-12-02 02:00 PST, Decathlon
bv1578: review+
Details | Diff | Splinter Review

Description Decathlon 2012-10-02 23:12:31 PDT
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.
Comment 1 Decathlon 2012-10-02 23:19:39 PDT
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.
Comment 2 Philipp Kewisch [:Fallen] 2012-11-28 09:50:55 PST
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
Comment 3 Decathlon 2012-12-02 02:00:15 PST
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?
Comment 4 Decathlon 2013-01-01 16:10:21 PST
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.
Comment 5 Philipp Kewisch [:Fallen] 2013-02-07 07:48:07 PST
Its not that important, lets close this bug as is :-) Thanks for investigating!

Note You need to log in before you can comment on or make changes to this bug.