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

RESOLVED FIXED in 3.3

Status

defect
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: bugzilla.mozilla.org, Assigned: anantkumarsingh)

Tracking

unspecified

Details

(Whiteboard: [good first bug][mentor=Fallen][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

12 years ago
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.
Reporter

Comment 1

12 years ago
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.
Reporter

Comment 2

12 years ago
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.
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

12 years ago
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.
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?
Component: Provider: ICS/Webdav → Calendar Views
OS: Windows 2000 → All
QA Contact: ics-provider → views
Hardware: PC → All
Whiteboard: [good first bug]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → Marcus
Status: NEW → ASSIGNED
Assignee: Marcus → nobody
Whiteboard: [good first bug] → [good first bug][mentor=Fallen][lang=js]
Status: ASSIGNED → NEW
Assignee

Comment 6

6 years ago
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.
Assignee: nobody → anantkumarsingh
Assignee

Comment 7

6 years ago
Posted patch calendar-list-tree.patch (obsolete) β€” β€” Splinter Review
Modified the cycleCell to make add/remove calendar using setTimeout.
Attachment #791630 - Flags: review?(philipp)
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.
Attachment #791630 - Flags: review?(philipp) → review-
Assignee

Comment 9

6 years ago
Posted patch calendar-list-tree.patch (obsolete) β€” β€” Splinter Review
Tried to incorporate changes suggested in review comments.
Attachment #791630 - Attachment is obsolete: true
Attachment #793460 - Flags: review?(philipp)
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.
Attachment #793460 - Flags: review?(philipp) → review-
Assignee

Comment 11

6 years ago
Posted patch calendar-list-tree.patch (obsolete) β€” β€” Splinter Review
Hoping this time it is much closer.
Attachment #793460 - Attachment is obsolete: true
Attachment #793881 - Flags: review?(philipp)
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.
Status: NEW → ASSIGNED
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
Attachment #793881 - Flags: review?(philipp) → review+
Posted patch Fix - v4 β€” β€” Splinter Review
Here is the patch, ready for checkin.
Attachment #793881 - Attachment is obsolete: true
Attachment #8392455 - Flags: review+
https://hg.mozilla.org/comm-central/rev/9de1aac7e031
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.1
Target Milestone: 3.1 → 3.3
You need to log in before you can comment on or make changes to this bug.