392418 - Sluggish response on rapid clicking of several checkboxes in the calendar list.

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Kramer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.4pre) Gecko/20070614 Sunbird/0.5

Sunbird immediately takes action on the selection/deselection of a calendar, and such action is sluggish when loading from an HTTP URL. As a result, user must wait for Sunbird to complete its HTTP transaction(s) before clicking the next checkbox in the calendars list.

Reproducible: Always

Steps to Reproduce:
1. Set up 5-10 ICS web-hosted calendar files
2. Try to quickly display them all and/or hide them all using the calendars list

Actual Results:  
After the first click, Sunbird tries to communicate with the web server, and I must wait for it to finish before clicking the next checkbox.

Expected Results:  
Sunbird should instead wait a short time (500-1000ms?) before taking action on such clicks. This gives the user time to quickly strafe the column of checkboxes as desired and then do something better with his time while Sunbird adjusts according to his selection.

Comment 1

[Kramer]

An alternative (and perhaps superior) fix would be to make the calendar file fetch calls asynchronous so that the UI remains responsive. Just a thought.

Comment 2

[Kramer]

It's almost just as laggy trying to open a local ICS file as it is to open one over HTTP. Is this a parsing issue? Is our ICS parser extremely slow?

Regardless, the slow action can probably be threaded into the background, and this would be preferable from the UI perspective.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Fetching ics files is already done in an async way, so that is not what causes the UI freeze. Parsing the file most likely is the problem here.

Comment 4

[seshu.parvataneni]

Yes, the refresh is really slow. Now with the introduction of the mouse scroll wheel to change the months, the UI refresh is really slow. I have about 40 events/month (which also contain description for almost every event) and 5 local calendars. It takes about 4-5 seconds before the month switches to another month when I use the scroll wheel to go to the previous or the next month. Hopefully the refresh rate will be improved in the near releases. Thanks in advance for providing us the extension. It is very usefule.

Comment 5

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

I could see a 500 ms delay improving the situation if you check and then uncheck a certain calendar, but the slowness problems will remain regardless of if the refresh is done right away or after 500 ms, if different calendars are being selected/unselected.

This could possibly be solved by setting _selectDelay="500" on the tree, but its possible that that only works for selecting rows, not for contained checkboxes. If that doesn't work, a timer logic needs to be implemented. I think this is a good first bug. Its a matter of making the actual code in cycleCell() for the checkbox column be called on a setTimeout(), saving the timer for that specific row. If cycleCell is called again, then check if the timer still exists and if so then clearTimeout() and set up a new timer, otherwise just set up the new timer.

Not sure we want to delay this, but I'd say its worth a shot. Daniel, Berend, whats your opinion?

Comment 6

[Anant Kumar Singh]

Hi,

I am new to this community and like to get started with some work. Kindly guide me how can I start work on this bug.

Comment 7

[Anant Kumar Singh]

Attachment: calendar-list-tree.patch

Modified the cycleCell to make add/remove calendar using setTimeout.

Comment 8

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

Comment on attachment 791630 [details] [diff] [review]
calendar-list-tree.patch

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

So first of all, there are some whitespace issues. Be sure to set your editor to use spaces instead of tabs. Then make sure the lines align correctly.

Also, could you make sure there is only one timer that is restarted on each cycle? So in short, here is what you need:

* cycleCell:
  - if the timer is running, cancel it
  - set a flag that the calendar has a pending toggle, or if already set remove the flag
  - set the timer that calls cycleCellCommit when it fires

* cycleCellCommit:
  - start batch operation
  - for each calendar that has a pending toggle:
    - add/remove from composite calendar
  - end batch operation
  - clear all pending toggles


Sorry if I was unclear, but I think this will give the best user experience: If multiple calendars are selected/deselected, then it will be done at once.

::: calendar/base/content/widgets/calendar-list-tree.xml
@@ +225,4 @@
>        <field name="treebox">null</field>
>        <field name="ruleCache">new Object()</field>
>        <field name="mCachedSheet">null</field>
> +      <field name="calFlag">new Object()</field>

Maybe you can use a more descriptive name instead and prefix it with "m" and use camelCase.

@@ +940,4 @@
>          <parameter name="aCol"/>
>          <body><![CDATA[
>            let calendar = this.getCalendar(aRow);
> +          

Trailing whitespaces in this (empty) line.

@@ +944,4 @@
>            switch (aCol.element.getAttribute("anonid")) {
>                case "checkbox-treecol":
> +              	if(!this.calFlag[calendar.id]) {
> +                	setTimeout(this.toggleCal(aRow),100);

This will execute the function toggleCal and call setTimeout on the result. You will want:

setTimeout(this.toggleCal.bind(this, aRow), 100);

which returns a function with the parameters and this-object bound.

Comment 9

[Anant Kumar Singh]

Attachment: calendar-list-tree.patch

Tried to incorporate changes suggested in review comments.

Comment 10

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

Comment on attachment 793460 [details] [diff] [review]
calendar-list-tree.patch

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

Sorry, not quite there yet. Please consider the following:

::: calendar/base/content/widgets/calendar-list-tree.xml
@@ +947,5 @@
> +              if (composite.getCalendarById(calendar.id)) {
> +                  this.mCycleCalendarFlag[calendar.id] = 'R';
> +              } else {
> +                  this.mCycleCalendarFlag[calendar.id] = 'A';
> +              }

If you check the composite calendar in cycleCell each time, then the following situation will fail:

1) Prerequisite: calendar has been previously checked
2) User cycles calendar => R flag is set
3) User cycles again to re-enable => its still in the composite, so R is set again (not ok)
4) 200 ms have passed
5) Since R flag is still set, calendar is removed.

Instead of concentrating on the composite calendar, consider the flag first. Psuedocode:

cycleCell:
  if boolean flag for calendar is set:
    remove boolean flag for calendar
  else:
    set boolean flag
  restart timer

cycleCellCommit:
  go through all true flags (not calendars):
    if in composite calendar:
      remove from composite
    else:
      add to composite
    remove true flag for calendar


Of course there is some possibility for optimization of the above pseudocode.

@@ +964,5 @@
> +        <body><![CDATA[
> +          let composite = this.compositeCalendar; 
> +              try {
> +                  composite.startBatch();
> +                  for (let i = 0; i < this.mCalendarList.length; i++) {

As mentioned above: You have all the flags in the mCycleCalendarFlag, you don't need to go through the whole calendar list, only those where the flag is set.

Comment 11

[Anant Kumar Singh]

Attachment: calendar-list-tree.patch

Hoping this time it is much closer.

Comment 12

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

Comment on attachment 793881 [details] [diff] [review]
calendar-list-tree.patch

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

Yes, very good. If its ok with you I will review this when I am available again after Sept. 5th? The patch should be fine aside from a few whitespace issues, I'd just like to take a moment to test it too.

Comment 13

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

Comment on attachment 793881 [details] [diff] [review]
calendar-list-tree.patch

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

I will fix the whitespace issues before pushing, r=philipp

Comment 14

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

Attachment: Fix - v4

Here is the patch, ready for checkin.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/9de1aac7e031