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

VERIFIED WORKSFORME

Status

Calendar
Tasks
--
minor
VERIFIED WORKSFORME
15 years ago
11 years ago

People

(Reporter: Laurent Duperval, Unassigned)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.80 KB, patch
cmtalbert
: first-review+
Michiel van Leeuwen (email: mvl+moz@)
: second-review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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

15 years ago
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.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Summary: Clicking "completed task" radiobutton in sidebar scrolls to top of listbox → Checking "done" checkbox in Tasks sidebar scrolls to top of listbox

Comment 2

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

Updated

13 years ago
QA Contact: gurganbl → sunbird

Comment 3

12 years ago
Created attachment 216370 [details] [diff] [review]
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.
Assignee: mostafah → jminta
Status: NEW → ASSIGNED
Attachment #216370 - Flags: first-review?(mvl)

Updated

12 years ago
Blocks: 204187
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

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

Comment 6

12 years ago
Created attachment 218427 [details] [diff] [review]
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.
Attachment #216370 - Attachment is obsolete: true
Attachment #218427 - Flags: first-review?(mvl)
Attachment #216370 - Flags: first-review?(mvl)

Updated

12 years ago
Attachment #218427 - Flags: second-review?(mvl)
Attachment #218427 - Flags: first-review?(mvl)
Attachment #218427 - Flags: first-review?(cmtalbert)
Comment on attachment 218427 [details] [diff] [review]
use better tree methods v2

r2=mvl
Attachment #218427 - Flags: second-review?(mvl) → second-review+

Comment 8

12 years ago
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.
Attachment #218427 - Flags: first-review?(cmtalbert) → first-review+

Comment 9

12 years ago
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.)
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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

12 years ago
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.
(In reply to comment #11)
> I'm inclined to say this patch should be backed out.

Reopening and backing this out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 360832

Comment 13

11 years ago
Re-assigning my bugs to nobody@mozilla.org due to recent developments.
Assignee: jminta → nobody
Status: REOPENED → NEW
Component: Sunbird Only → Tasks
QA Contact: sunbird → tasks
Hardware: PC → All
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → WORKSFORME
WFM confirmed. Calendar 20080221 on Linux

> WFM confirmed. Calendar 20080221 on Linux

Verifying.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.