232202 - Checking "done" checkbox in Tasks sidebar scrolls to top of listbox

Status: VERIFIED WORKSFORME

(Calendar :: Tasks, defect)

Description

[Laurent Duperval]

User-Agent:       
Build Identifier: Calendar 2004010913-cal in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20040105 Thunderbird/0.4

This issue is very silimal, I think, to bug 157269. However, that bug seems to
have been fixed so long a go that I decided to create a new one.

Reproducible: Always
Steps to Reproduce:
1. Enter more tasks thatn space will allow on the sidebar.
2. Scroll to the bottom of the list
3. Select the bottom task.

Actual Results:  
 The scrollbar brings you back to the top

Expected Results:  
I expected the scrollbar to remain where it was.

Comment 1

[Jussi Kukkonen]

I'm not seeing this on select, but checking 'done' or changing 'progress' or
'priority' does result in scrolling to top of list.

I agree it should not do this. Confirming, changing summary.

Comment 2

[Joey Minta]

*** Bug 276773 has been marked as a duplicate of this bug. ***

Comment 3

[Joey Minta]

Attachment: use better tree methods

I've been meaning to write something like this up for a long time (and do something similar for the unifinder).  This patch makes the task-list modify individual rows when tasks change, rather than rebuilding the whole list.  Should be a major perf win.  Also fixes bug 204187.

Comment 4

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

Comment on attachment 216370 [details] [diff] [review]
use better tree methods

>+    addItemToTree: function(aItem) {
>+        for (var i in gTaskArray) {
>+            if (gTaskArray[i].id == aItem.id) {
>+                index = i;

This loop seems to do the same as getRowOfCalendarTask does. Why not just use that function?
But the loop (and the one in getRowOfCalendarTask) made me wonder if just checking the ID is enough. In theory, two calendars can contain tasks with the same id. They should be copies, but you never know. Should we also check if the calendars match? And maybe add a .equals() method on calIItemBase?

Comment 5

[Joey Minta]

*** Bug 333630 has been marked as a duplicate of this bug. ***

Comment 6

[Joey Minta]

Attachment: use better tree methods v2

This version removes the duplicate copies of the code from getRowOfCalendarTask.  The previous patch also failed to consider the case where the task's row might need to change on modification, due to it having a new position in the sort.

This patch also moves to using .hasSameIds, which is the preferred way of checking if two items are the same.  Ids should be unique across calendars, so comparing the calendars shouldn't be necessary.

Comment 7

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

Comment on attachment 218427 [details] [diff] [review]
use better tree methods v2

r2=mvl

Comment 8

[cmtalbert]

Comment on attachment 218427 [details] [diff] [review]
use better tree methods v2

r+ I do have one question, though. In onDeleteItem, do we need to removeItemFromTree if it is completed as well as if it is not completed? The code as written looks like it will only removeItemFromTree if the item is not completed. But, I may not fully understand how the deletion works.

Comment 9

[Joey Minta]

Patch checked in.

(In reply to comment #8)
If the task that was deleted was completed, but we're not showing completed tasks, then it's not in the tree.  Therefore, we don't need to remove it.  (Note that this could only happen if the task was deleted from the views.)

Comment 10

[Martin Schröder [:mschroeder]]

I tried to verify that this bug is fixed, but sometimes (I think when the scrollbar is a the bottom!) the task list scrolls one item up when (un)marking a task as completed. The scroll to top issue is solved.

If someone can confirm this, please reopen this bug.

Comment 11

[Joey Minta]

I'm inclined to say this patch should be backed out.  It has caused a serious regression in bug 360832 (and i've seen reports of other problems) that no one has stepped up to fix.

Comment 12

[Matthew (lilmatt) Willis]

(In reply to comment #11)
> I'm inclined to say this patch should be backed out.

Reopening and backing this out.

Comment 13

[Joey Minta]

Re-assigning my bugs to [e-mail] due to recent developments.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.13pre) Gecko/20080221 Calendar/0.8pre

WFM. The issues listed in this bug no longer seem to exist.

Comment 15

[Lars Wohlfahrt (thetux)]

WFM confirmed. Calendar 20080221 on Linux

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> WFM confirmed. Calendar 20080221 on Linux

Verifying.