(optimization) The calendar view should only be refreshed if clicked date is not already displayed

RESOLVED FIXED in 3.4

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: alexandre.f.demers, Assigned: weichen302)

Tracking

Lightning 1.9.1

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.64 Safari/537.31

Steps to reproduce:

While displaying a whole week in the calendar view, I clicked a date in the mini-calendar (on the left) already shown in my weekly view.


Actual results:

The calendar view was refreshed.


Expected results:

When the clicked date is already displayed, the calendar view should not be refreshed.
(Assignee)

Comment 1

5 years ago
Created attachment 8372784 [details] [diff] [review]
fix unnecessary refresh of Month, Multiweek, Week view when click on minimonth

It is strange that the solution is already there. In calendar-multiday-view.xml, there is a section to figure out whether current date range has been changed, but the author disabled it because 
"In general it should be possible to return here, but lightning doesn't like it when first initializing the view" -- Line 2772

I found it is Okay to un-comment that section.

The similar refresh problem also exist in Multiweek and Month view. Here is a proposed fix, for week, multiweek, and month views.
(Assignee)

Updated

5 years ago
Attachment #8372784 - Flags: review?(philipp)
Assignee: nobody → weichen302
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 2

5 years ago
Chen, as soon as I'll be able to compile the latest Thunderbird version, I'll test your patch.
(Assignee)

Comment 3

5 years ago
Alex, forgot to mention, it was tested on lightning 1.9.1 and that patch was generated from comm-esr17. Looks like its "calendar-multiday-view.xml" is 4 commits behind comm-central.
Please do not provide patches against comm-esr17. This branch is obsolete. If you provide patches please build them based on the latest comm-central repository to ensure that they apply to the current Lightning 3.2 test builds.
(Reporter)

Comment 5

5 years ago
Looking at the patch, it should be pretty straight forward to rebase on comm-central. I'm still experiencing some build problem with latest source code, but I shall investigate it later today. Once done, I'll let you know if there is any probem applying the patch on latest comm-central.
(Assignee)

Comment 6

5 years ago
Created attachment 8373820 [details] [diff] [review]
patch against comm-central
Attachment #8372784 - Attachment is obsolete: true
Attachment #8372784 - Flags: review?(philipp)
Attachment #8373820 - Flags: review?(philipp)

Comment 7

5 years ago
Returning prematurely from the function setDateRange() in the calendar-multiday-view.xml has some bad effect because, e.g., some sub-menu item inside the menu View > Calendar > Current View doesn't have an immediate effect.
For example setting the menu item "Workweek days only" doesn't work until you change the view (the next/previous week). The opposite works immediately instead. Similar problems occur for the options "Tasks in view" and "Show completed Tasks".
(Assignee)

Comment 8

5 years ago
Created attachment 8376790 [details] [diff] [review]
fix unnecessary refresh  when click on minimonth

Thanks Decathlon. I can repeat this problem.

It appears multiday-view uses slightly different logic than month-view, specifically, setDateRange is called directly instead of showDate after preference change. This patch attempts fix this problem by adopting the similar logic as month-view does.

I use lightning like a old fashion paper calendar, so the test I did might not complete. Because it changed several places in calendar-multiday-view.xml and calendar-views.xml, it is likely new bugs have been introduced.
Attachment #8373820 - Attachment is obsolete: true
Attachment #8373820 - Flags: review?(philipp)
Attachment #8376790 - Flags: review?(bv1578)

Comment 9

5 years ago
Comment on attachment 8376790 [details] [diff] [review]
fix unnecessary refresh  when click on minimonth

Review of attachment 8376790 [details] [diff] [review]:
-----------------------------------------------------------------

Just testing until now without look as the code works, but there are a few things that doesn't work properly, in particular the zoom-in/zoom-out features (menu View->Zoom or keys CTRL + +/-) and the options "day starting hour" and "day ending hour" (menu Tool->Option->Calendar->View).
These options need to change the view to get working, the same as comment 7 (they probably don't get the view refreshed).

Minor issues related to the code style:

::: calendar/base/content/calendar-month-view.xml
@@ +644,5 @@
> +
> +          if (this.mStartDate && this.mEndDate
> +                && this.mEndDate.compare(viewEnd) == 0
> +                && this.mStartDate.compare(viewStart) == 0)
> +                return;

Fix indentation and insert the brackets before and after the "return" statement.

::: calendar/base/content/calendar-multiday-view.xml
@@ +3253,5 @@
>        <method name="relayout">
>          <body><![CDATA[
> +
> +          if (!this.mStartDate || !this.mEndDate) {
> +            return;

Four spaces indentation.

@@ +3281,1 @@
>            }

Four spaces indentation in all the "for" block.

@@ +3286,5 @@
> +          this.setSelectedItems(selectedItems.length, selectedItems, true);
> +
> +
> +
> +

Maybe less blank lines here ;-)

::: calendar/base/content/calendar-views.xml
@@ +87,5 @@
>                  <body><![CDATA[
>                      this.displayDaysOff = !this.mWorkdaysOnly;
>  
> +                    if (aDate) {
> +                      aDate = aDate.getInTimezone(this.timezone);

Four spaces indentation.


Setting r- for now.
Attachment #8376790 - Flags: review?(bv1578) → review-

Comment 10

5 years ago
... "four spaces indentation in all the "for" block"...  was about this one:

>+          for (let d=this.mStartDate.clone(); d.compare(this.mEndDate)<=0;) {
>+            var workday = d.clone();
>+            workday.makeImmutable();
>+            if (this.mDisplayDaysOff) {
>+              computedDateList.push(workday);
>+            } else if (this.mDaysOffArray.indexOf(d.weekday) == -1) {
>+              computedDateList.push(workday);
>+            }
>+            d.day += 1;
>           }
(Assignee)

Comment 11

5 years ago
Created attachment 8377431 [details] [diff] [review]
fix minimonth refresh

Thanks for the review, Decathlon.

It turns out that refresh problem was the desired behavior of goToDay because refreshView() depends on goToDay refresh the view whenever it is called. Add a this.refresh() to goToDay solved this problem, and, hopefully, nothing else is broken in the process. BTW, how do you carry out test? Is it automatically or manually?
Attachment #8376790 - Attachment is obsolete: true
Attachment #8377431 - Flags: review?(bv1578)
(Reporter)

Comment 12

5 years ago
Sorry for not replying last week, but after trying to build Thunderbird and Lightning a few times, I just put it on the side because it was still failing. I'll try to test it again this week. Meanwhile, good to know somebody else has been testing it on his side too.

Comment 13

5 years ago
Comment on attachment 8377431 [details] [diff] [review]
fix minimonth refresh

Review of attachment 8377431 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Chen Wei from comment #11)
> BTW, how do you carry out test? Is it automatically or manually?

Manually. A lot of patience and the awareness that anyway there is always something missing ;-)


The patch doesn't work fine yet.
When you change settings such as "Day starts at", "Day ends at", "Number of week to show"... (menu Tools>Options>Calendar>Views), the changes take place in the view where you are (when the changes are related to that particular view), but don't appear in the other views when you change view. Eventually they appear only the first time you change the view.

E.g. after TB has started, from week or day view, try to change the settings "Day starts at" or "Day ends at" then go to the other view (day or week), here you can see that the new settings are correct, but if you change again the settings and return back to the initial view, now the off-time zone is wrong, apart from the time-bar where it is correct (excluded the effect due to bug 975157). To sort out the off-time zone you have to do an action that calls the refresh, e.g. navigate to the previous/next day/week.
The same happens if you change those settings from the month/multiweek views and then go to day/week views.

Another case: if you are in week view and you change the setting "Number of week to show", when you switch to multiweek view you can see that the number of week has not changed, it changes only the first time if TB has just started.

All that happens because the first time you change the view, the refresh() takes place from setDateRange() because mStartDate and mEndDate are both null. The second time and the next times the "if" condition inside setDateRange() is always satisfied so the refresh is never executed.

::: calendar/base/content/calendar-month-view.xml
@@ +638,4 @@
>          <body><![CDATA[
>            this.rangeStartDate = aStartDate;
>            this.rangeEndDate = aEndDate;
> +          var viewStart = getWeekInfoService().getStartOfWeek(

I forgot to say in the previous review: please use the keyword "let" instead of "var" when declaring/defining new variables, here and in all the patch.

@@ +644,5 @@
> +                                      aEndDate.getInTimezone(this.mTimezone));
> +          if (this.mStartDate && this.mEndDate &&
> +                  this.mEndDate.compare(viewEnd) == 0 &&
> +                  this.mStartDate.compare(viewStart) == 0) {
> +              return;

Code indentation.

::: calendar/base/content/calendar-multiday-view.xml
@@ -2869,5 @@
> -            // For a true multiday view (e.g, 3 days advanced by one day
> -            // at a time), a smarter refresh could reuse boxes, comparing
> -            // the current date range and add/remove, instead of just
> -            // replacing.
> -            //

I would leave this comment somewhere, it could be useful in the future if someone will change multiday view's implementation.


When the patch is going to work fine we have to sort out other comments too.

Setting r- for now.
Attachment #8377431 - Flags: review?(bv1578) → review-

Comment 14

5 years ago
>+<!--
>+cw: since setDateList has been removed from setDateRange and all its logic has
>+been moved to relayout, try comment out this block, so far so good.
>+
>       <method name="setDateList">
>         <parameter name="aCount"/>
>         <parameter name="aDates"/>
>@@ -2927,6 +2895,7 @@
>           this.setSelectedItems(selectedItems.length, selectedItems, true);
>         ]]></body>
>       </method>
>+-->
> 

Since the method setDateList() belongs to a general scheme that allow to treat the problem of showing disjoint dates in a view
http://mxr.mozilla.org/comm-central/source/calendar/base/public/calICalendarView.idl#154
I'm not sure that deleting it is a good approach even though the patch was working fine and the related code has been moved elsewhere, but here I prefer to ask Philipp's opinion.
Flags: needinfo?(philipp)
(Assignee)

Comment 15

5 years ago
Created attachment 8383609 [details] [diff] [review]
fix unnecessary refresh

Thanks for the review, Decathlon.

Changes in this patch: 
1) add 3 private properties, this.mViewStart, this.mViewEnd, and this.mToggleStatus to determine whether a view need to be refreshed.

   Lightning already have two private propterties, mRangeStartDate and mRangeEndDate, they seem have the same role as the newly added mViewStart and mViewEnd, but I am not 100 percent sure.

2) add a forcereRresh() method, refreshView() call it to refresh all views, visible or invisible.

I have tested it on problems mentioned by Alex and by you in comments 7, 9, 13. So far so good.

This problem has wider impact than my first impression. I wonder if it can be fixed other way. On the one hand, since we are trying to fix the minimonth refresh problem, would it better to begin with minimonth? I tried that approach and found minimonth dispatch the "select" event to something else, and eventually trigger the refresh, in a way I am not fully understand; and on the other hand, there are other operations can benefit from the modified setDateRange(), e.g. go to today no longer cause view refreshing.
Attachment #8377431 - Attachment is obsolete: true
Attachment #8383609 - Flags: review?(bv1578)

Comment 16

5 years ago
(In reply to Chen Wei from comment #15)

> This problem has wider impact than my first impression. I wonder if it can
> be fixed other way. On the one hand, since we are trying to fix the
> minimonth refresh problem, would it better to begin with minimonth? I tried
> that approach and found minimonth dispatch the "select" event to something
> else, and eventually trigger the refresh, in a way I am not fully
> understand; and on the other hand, there are other operations can benefit
> from the modified setDateRange(), e.g. go to today no longer cause view
> refreshing.

Every action of that kind should end with a call to the goToDay() function.
The minimonth does the same. The 'select' event is captured by onchange:
http://mxr.mozilla.org/comm-central/source/calendar/lightning/content/messenger-overlay-sidebar.xul#148
which calls the method minimonthPick:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-common-sets.js#889
and here there is a call to the goToDay() function.


The basic idea is to separate goToDay() from refresh() when the start date and the end date of the view doesn't change but the main problem is that there are many situations that need the refresh even in those cases, like the issues described in comment 13.
As a general approach we could call a refresh() after the goToDay() in these cases, e.g. *at first glance* to fix the issues in your previous patch should be sufficient to add a call to refresh() from the function that switches the views after the call to goToDay() here:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-views.js#343

in this way you are sure that the new view always get the refresh, but it needs to be verified deeper.

In another way we could add a second parameter to goToDay(), a boolean, which, when present and true, forces anyway the refresh (e.g. inside the function setDateRange). Obviously it should be identified all the cases that need to force the refresh and this could be done by analyzing all the calls to goToDay and where they came from:

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

Your last patch IMHO seems a bit twisted for the scope.

Since Philipp will have the last word on this, would be better to ask him a correct way to approach the problem in order to avoid taking wrong directions.
(Assignee)

Comment 17

5 years ago
Thanks for the explanation. So try fix it from minimonth will also go to goToDay() and eventually setDateRange().

Currently, goToDay has its own way to trigger an unconditional refresh by call the showDate() with a null aDate. The problem is for those invisible views, for example, in the month view, change the "Day starts at", the weekly view should update its view, but instead, because the refresh() will only push jobs to the refresh queue when the view is visible, that refresh request is ignored. In current Lightning, next view switching will probably ends up with setDateRange(), which will refresh anyway, therefor ignoring refresh request on invisible view is not a problem.

My point is both the forced refresh by goToDay() with a second parameter, or call refresh() after goToDay() will still be ignored on some views, unless refresh() works on invisible views as well, that is why there is a forceRefresh() in the patch, it is a clone of refresh(), except the isVisible conditional switch has been removed. Of course it won't be a problem if we do not change setDateRange() and let it refresh anyway when called, but again it refreshes too often.

Although my test seems Okay, but since there are changes in many places, I also feel slightly “unsecure”. At this moment, we need more eyeballs!
(In reply to Decathlon from comment #14)
> Since the method setDateList() belongs to a general scheme that allow to
> treat the problem of showing disjoint dates in a view
> http://mxr.mozilla.org/comm-central/source/calendar/base/public/
> calICalendarView.idl#154
> I'm not sure that deleting it is a good approach even though the patch was
> working fine and the related code has been moved elsewhere, but here I
> prefer to ask Philipp's opinion.

I haven't actually tested this patch, but given setDateList isn't implemented in any other view, I think it would be safe to just remove this method from the idl file too.
Flags: needinfo?(philipp)

Comment 19

5 years ago
Comment on attachment 8383609 [details] [diff] [review]
fix unnecessary refresh

There are still a few problems with the navigation bar and the selected events.

The navigation bar *sometimes* doesn't change when switching from a view to another. The same was in the previous patches too but I didn't notice that, sorry. It's caused by the fact that now switching the views doesn't always need a refresh, but the navigation bar need always to change.
At first glance it should be easy to fix by moving the line of code that updates the navigation bar before the "if" that allows to "return" from setDateRange() (i.e. by updating anyway the navigation bar) but please verify carefully.
While I was testing this last thing I found bug 992669 that doesn't depend on your patch, despite is strictly related, so I think it's fine to treat it elsewhere, only be aware of that when testing. 

The other issue about selected events depends on this code:

>+          //cw: not sure if need save selectedItems and restore later
>+          let selectedItems = this.getSelectedItems({});
>+          this.setSelectedItems(0, [], true); // suppress event, will restore
> 
...
>+          //cw: not sure if need save selectedItems and restore later
>+          this.setSelectedItems(selectedItems.length, selectedItems, true);

it has been introduced with the fix for bug 405196 (in particular read bug 405196 comment 6). 
The way you implemented it seems useless because now there isn't anymore a refresh between suppress and restore.
Because of that, the patch arises the issues reported in bug 405196, in particular: in week-view select an event then change the week and select another event, when you turn back to the previous week the first event is still selected but it shouldn't. Now that event stays selected forever even if you select another one in the same week.


>+      <method name="forceRefresh">
>+        <body><![CDATA[
>+          var refreshJob = {};

let instead of var.

>+          // Store the start and end of current view. Next time when
>+          // setDateRange is called, it will use mViewStart and mViewEnd to
>+          // check if view range has been changed.
>+          this.mViewStart = this.mStartDate;
>+          this.mViewEnd = this.mEndDate;
>+
...
>+          this.mToggleStatus = toggleStatus;

About the new fields
  mToggleStatus
  mViewStart
  mViewEnd
you have to declare them inside the implementation in the calendar-base-view binding:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-base-view.xml#17
Since mToggleStatus stores three booleans fields, you could use an array of booleans, but the way you did works fine too and it's simpler doing a check.

> 
>+<!--
>+cw: since setDateList has been removed from setDateRange and all its logic has
>+been moved to relayout, try comment out this block, so far so good.
>+
>       <method name="setDateList">
>         <parameter name="aCount"/>
>         <parameter name="aDates"/>
>@@ -2927,6 +2924,7 @@
>           this.setSelectedItems(selectedItems.length, selectedItems, true);
>         ]]></body>
>       </method>
>+-->

Since Philipp agrees with deleting the method setDateList(), you can remove the code commented out and all the references to setDateList() in comm-central included the idl file:
http://mxr.mozilla.org/comm-central/search?string=setDateList&find=%2Fcalendar%2F 


Apart from things that need to be fixed, it's nice to see the calendar views that don't "flash" (because of the deleted refresh) when one switches many times between views, in particular with many events in the views.
Attachment #8383609 - Flags: review?(bv1578) → review-
(Assignee)

Comment 20

5 years ago
Created attachment 8403999 [details] [diff] [review]
fix unnecessary refresh

Thanks for the thorough test.

Changes in this patch:

1) The event selecting behavior in week view has been changed. The selected events will be de-selected upon moving forward/backward in time. It is not exactly the way it was in the un-patched lightning, which only de-select events after click event on another week, but it matches the behavior of multi-week & month view in un-patched lightning.

2) always updating navigationbar

3) declare mToggleStatus, mViewStart, mViewEnd in implementation section

4) mToggleStatus is computed by bit OR

5) remove all setDateList
Attachment #8383609 - Attachment is obsolete: true
Attachment #8403999 - Flags: review?(bv1578)

Comment 21

5 years ago
Comment on attachment 8403999 [details] [diff] [review]
fix unnecessary refresh

(In reply to Chen Wei from comment #20)
> 1) The event selecting behavior in week view has been changed. The selected
> events will be de-selected upon moving forward/backward in time. It is not
> exactly the way it was in the un-patched lightning, which only de-select
> events after click event on another week, but it matches the behavior of
> multi-week & month view in un-patched lightning.

Humm, here I'm not sure if this is the right behavior, it sounds like the bugged behavior is that one of the multiweek-view and month-view. We will ask to Philipp when he will do the review. In case we can open a specific bug for both the kind of views.

> 4) mToggleStatus is computed by bit OR

>+      <field name="mToggleStatus">0</field>
>+      <field name="mToggleStatusFlag"><![CDATA[
>+      ({ WorkdaysOnly:  1,
>+         TasksInView:   2,
>+         ShowCompleted: 4,
>+      })
>+      ]]></field>
>+

>+          let toggleStatus = 0;
>+
>+          if (this.mTasksInView) {
>+              toggleStatus |= this.mToggleStatusFlag.TasksInView;
>+          }
>+          if (this.mWorkdaysOnly) {
>+              toggleStatus |= this.mToggleStatusFlag.WorkdaysOnly;
>+          }
>+          if (this.mShowCompleted) {
>+              toggleStatus |= this.mToggleStatusFlag.ShowCompleted;
>+          }
>+
>+          this.mToggleStatus = toggleStatus;

when I talked about an array of booleans I meant a way to store directly the three fields. I don't want to force you to use a different solution but doing in another way would allow to make the code shorter and easier to treat, in particular when you store and read the current status, moreover it would be easy to expand with other fields. I thought something like:

declare the field in this way (calendar-base-view binding):

  <field name="mToggleStatus">[]</field>

store the status in the constructor of the calendar-base-view binding and everywhere you need by simply adding the fields in the array:

  this.mToggleStatus = [this.mTasksInView, this.mWorkdaysOnly, this.mShowCompleted];

read the current status before compare with the stored status by simply creating a new array with the current values:

  let toggleStatus = [this.mTasksInView, this.mWorkdaysOnly, this.mShowCompleted];

compare the current status with the stored status (it could be done in different ways, e.g. as follows):

  let toggleStatusNotChanged = this.mToggleStatus.every(function (element, index, array) {
                                                            return (element == toggleStatus[index]);
                                                        });
and use toggleStatusNotChanged in the "if" condition.


>-/* This Source Code Form is subject to the terms of the Mozilla Public
>+* This Source Code Form is subject to the terms of the Mozilla Public

Here a change that must be restored, probably a typo.


Hopefully this is the last r-, after that I will ask Philipp to take a look here.
Attachment #8403999 - Flags: review?(bv1578) → review-
(Assignee)

Comment 22

5 years ago
Created attachment 8408679 [details] [diff] [review]
fix unnecessary refresh, fix typo

Thanks for the elegant tip on array comparison, which I am unaware of until read comment 20. But for now, can we keep the mToggleStatus bitwise flagging? It consist with the way status flagged in other part of lightning. 

This patch only fixed (3). I blame vim for that typo, :)
Attachment #8408679 - Flags: review?(bv1578)

Comment 23

5 years ago
Comment on attachment 8408679 [details] [diff] [review]
fix unnecessary refresh, fix typo

As far as I can see this one seems working fine, but overall it changes a lot of things and it touches important elements such as refresh and relayout and don't have neither the "power" nor the competence to say the last word.

Philipp, please could you take a look here?

Please keep in count observations about selected events in comment 19 and comment 21.
Moreover consider that something is not always working in the navigation bar but it doesn't depend on this patch (see bug 992669) and that under some circumstances the patch causes a misalignment of the weekday headers in week view. This last issue depends only partially from this patch because bug 998260 is the main cause (see second example there in the description). In any case I have a fix for another bug that would fix this one as well.

Here you can find a build for Windows if you use that platform:
http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/bv1578@gmail.com-737357d8fbae/try-comm-central-win32/
(it is based on the previous patch but with the same correction that the last patch has)
Attachment #8408679 - Flags: review?(philipp)
Attachment #8408679 - Flags: review?(bv1578)
Attachment #8408679 - Flags: review+
Comment on attachment 8408679 [details] [diff] [review]
fix unnecessary refresh, fix typo

Review of attachment 8408679 [details] [diff] [review]:
-----------------------------------------------------------------

> Moreover consider that something is not always working in the navigation bar
> but it doesn't depend on this patch (see bug 992669) and that under some
> circumstances the patch causes a misalignment of the weekday headers in week
> view. This last issue depends only partially from this patch because bug
> 998260 is the main cause (see second example there in the description). In
> any case I have a fix for another bug that would fix this one as well.
As long as you have a patch to fix this case thats fine with me. We should definitely find the root cause for this if not though.

This is landing after the merge anyway, so we still have a lot of time to find errors. Although it would be nice to avoid refreshes for TB31, I'm a bit reluctant to push this to aurora without proper testing. On the other hand, our beta community is not that strong anyway so most testing happens in the release builds.

r=philipp with the following minor changes, please upload a new patch for checkin.

::: calendar/base/content/calendar-base-view.xml
@@ +535,4 @@
>          ]]></body>
>        </method>
>  
> +      <method name="forcerefresh">

forceRefresh

::: calendar/base/content/calendar-month-view.xml
@@ +663,5 @@
> +              toggleStatus |= this.mToggleStatusFlag.WorkdaysOnly;
> +          }
> +          if (this.mShowCompleted) {
> +              toggleStatus |= this.mToggleStatusFlag.ShowCompleted;
> +          }

I don't mind using bitmasks here, ideally we should then convert the views to use a single "features" field to replace mTasksInView/mWorkdaysOnly/mShowCompleted/etc, which uses bitmasks. Define constants directly on the view binding. This way you could just check if the features field has changed and don't need special casing for the toggle check.

This is fine for a followup bug, I'd appreciate if you could take care of that one some time soon though.

@@ +680,2 @@
>            // Update the navigation bar.
>            cal.navigationBar.setDateRange(aStartDate, aEndDate);

I'd suggest to restructure the if statement here by flipping it around and conditionally refreshing to avoid the double call to cal.navigationBar.setDateRange().

::: calendar/base/content/calendar-multiday-view.xml
@@ +2879,5 @@
> +              this.mViewStart.compare(viewStart) == 0 &&
> +              this.mToggleStatus == toggleStatus) {
> +              // Update the navigation bar without refresh entire view
> +              cal.navigationBar.setDateRange(viewStart, viewEnd);
> +              return;

Same comment about the double setDateRange call.
Attachment #8408679 - Flags: review?(philipp) → review+
(Assignee)

Comment 25

5 years ago
> ::: calendar/base/content/calendar-month-view.xml
> > +          if (this.mShowCompleted) {
> > +              toggleStatus |= this.mToggleStatusFlag.ShowCompleted;
> > +          }
> I don't mind using bitmasks here, ideally we should then convert the views
> to use a single "features" field to replace
> mTasksInView/mWorkdaysOnly/mShowCompleted/etc, which uses bitmasks. Define
> constants directly on the view binding. This way you could just check if the
> features field has changed and don't need special casing for the toggle
> check.
> 

Can describe that single "features" field in detail? How to compare the "features" field before and after change? Perhaps we can do it in this bug.
With "features" I mean a field that contains all the boolean decisions we currently have separately on the view. This is kind of what I am thinking of, not necessarily all on one binding.

<field name="CALVIEW_SHOW_COMPLETED">1</field>
<field name="CALVIEW_TASKS_IN_VIEW">2</field>
<field name="CALVIEW_WORKDAYS_ONLY">4</field>
...

...
    if (this.features & this.CALVIEW_SHOW_COMPLETED) {
      // show completed is on, act accordingly
    }
...

You can also define a mask for the flags that cause a status toggle, then you just have to check the mask with this.features. Hope this explains what I am thinking of and that I read the code right.
(Assignee)

Comment 27

5 years ago
Thanks for the explain. I also found codes such as 

switch (itemFilter & calICalendar.ITEM_FILTER_COMPLETED_ALL)
    case xxxxx
...

in lightning. Now I see.

But there still need the this.mToggleStatus field to store the toggle statuses from which current view was generated. Besides, the "if (this.features & this.CALVIEW_SHOW_COMPLETED)" is longer and harder to read, compare to "if (this.mShowCompleted)", and I don't think there is any performance gain.
(Assignee)

Comment 28

5 years ago
Created attachment 8415713 [details] [diff] [review]
fix unnecessary refresh

Changes:

1) forcerefresh => forceRefresh

2) move cal.navigationBar.setDateRange(aStartDate, aEndDate) before if statement.
Attachment #8403999 - Attachment is obsolete: true
Attachment #8408679 - Attachment is obsolete: true
Attachment #8415713 - Flags: review?(philipp)
(In reply to Chen Wei from comment #27)
> But there still need the this.mToggleStatus field to store the toggle
> statuses from which current view was generated. Besides, the "if
> (this.features & this.CALVIEW_SHOW_COMPLETED)" is longer and harder to read,
> compare to "if (this.mShowCompleted)", and I don't think there is any
> performance gain.
No, its not really about a performance gain, I was hoping that features could replace mToggleStatus? Maybe I don't quite understand why mToggleStatus is needed. Could you explain?
(Assignee)

Comment 30

5 years ago
mToggleStatus is basically a saved copy of the proposed "features field" when the view was last generated. It will be compared to the latest "features field" to decide whether to refresh the entire view. The equivalent of "features field" in this patch is called toggleStatus, unlike the "features field", it is only computed right before compare.

Let's say we are storing all features in the "features field", therefore fields such as mShowCompleted is removed, however, mShowCompleted is used in many if statements, so each of them will be replaced by "(this.features & this.CALVIEW_SHOW_COMPLETED)". Here a "CALVIEW_SHOW_COMPLETED" is needed only because mShowCompleted is removed. In short, we will remove mShowCompleted and introduce CALVIEW_SHOW_COMPLETED. We end up with same number of fields and longer if statements.
Comment on attachment 8415713 [details] [diff] [review]
fix unnecessary refresh

Review of attachment 8415713 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I've taken a closer look at the code and I am fine with keeping it this way. I was thinking that any time that mTasksInView/mShowCompleted/etc is changed, there would be a refresh anyway, which would lead to a relayout, which would allow us to compare the previous feature field with the new one. As the flow is a little different and the setters don't trigger the relayout directly, your toggleStatus is needed anyway. Therefore my "features" idea doesn't really make sense as is.

It looks like this will be the last set of comments for this bug, thank you so much for sticking around and working through all the comments with us! I'm looking forward to your next patches.

I'd appreciate if you could upload a new patch with the following minor nits and question answered. r=philipp

::: calendar/base/content/calendar-month-view.xml
@@ +677,5 @@
> +              this.mToggleStatus == toggleStatus) {
> +              return;
> +          }
> +
> +          this.refresh();

Could you invert the if condition and put the refresh inside the if block? This way you don't need the extra return and the control flow if this function is more obvious even if further code is added in the future.

::: calendar/base/content/calendar-multiday-view.xml
@@ +2885,3 @@
>            }
>  
>            this.refresh();

Same here

@@ -2925,5 @@
>  
>            this.refresh();
>  
> -          // restore selected item occurrences in view with new date columns.
> -          this.setSelectedItems(selectedItems.length, selectedItems, true);

BTW, what ever happened to this? Does the selection still get restored correctly?
Attachment #8415713 - Flags: review?(philipp) → review+
(Assignee)

Comment 32

5 years ago
Created attachment 8416405 [details] [diff] [review]
0002-fix-unnecessary-refresh-bug-872063.patch

> @@ -2925,5 @@
> >  
> >            this.refresh();
> >  
> > -          // restore selected item occurrences in view with new date columns.
> > -          this.setSelectedItems(selectedItems.length, selectedItems, true);
> 
> BTW, what ever happened to this? Does the selection still get restored
> correctly?

Its usage was inconsistent: mutiday-view uses it but not by month-view. I guess it was there because lightning used to always refresh the view on actions such as click on minimonth. That behavior has been changed by this patch. If the view is not refreshed, then nothing need to be restored.
Attachment #8415713 - Attachment is obsolete: true
Attachment #8416405 - Flags: review?(philipp)
Comment on attachment 8416405 [details] [diff] [review]
0002-fix-unnecessary-refresh-bug-872063.patch

Review of attachment 8416405 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, done! I've made a few minor style adjustments I don't want to burden you with, this patch is ready for checkin.
Attachment #8416405 - Flags: review?(philipp) → review+
Created attachment 8416682 [details] [diff] [review]
Patch for checkin
Attachment #8416405 - Attachment is obsolete: true
Attachment #8416682 - Flags: review+
Keywords: checkin-needed
Target Milestone: --- → 3.4
Comment on attachment 8416682 [details] [diff] [review]
Patch for checkin

Review of attachment 8416682 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/calendar-month-view.xml
@@ +671,4 @@
>  
> +          // Check whether view range has been changed since last call to
> +          // relayout()
> +          if (!this.mViewStart !this.mViewEnd ||

It seems a logical operator is missing between !this.mViewStart and !this.mViewEnd.
Created attachment 8416705 [details] [diff] [review]
Patch for checkin

Sorry, missing qref. This is the right one.
Attachment #8416682 - Attachment is obsolete: true
Attachment #8416705 - Flags: review+
https://hg.mozilla.org/comm-central/rev/17f71d9c0fc6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 38

5 years ago
> > under some
> > circumstances the patch causes a misalignment of the weekday headers in week
> > view. This last issue depends only partially from this patch because bug
> > 998260 is the main cause (see second example there in the description). In
> > any case I have a fix for another bug that would fix this one as well.
> As long as you have a patch to fix this case thats fine with me. We should
> definitely find the root cause for this if not though.
> 
I attached a fix for Bug 344561 that should fix this issue too.
You need to log in before you can comment on or make changes to this bug.