Closed Bug 405111 Opened 17 years ago Closed 17 years ago

Hidden completed tasks are visible in task list after Startup or Reload Remote Calendars

Categories

(Calendar :: Tasks, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bugzilla.i.sekler, Assigned: michael.buettner)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112211 Firefox/3.0b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.10pre) Gecko/20071123 Calendar/0.8pre

If the checkbox "Hide Completed Tasks" is checked, reloading remote calendars causes the completed tasks to appear in the todo list while the checkbox remains checked. To hide the completed tasks again, unchecking and checking the checkbox is required.

Reproducible: Always

Steps to Reproduce:
1. Open an *.ics file or add a remote calendar
2. check the "Hide Completed Tasks" checkbox
3. Reload remote calendars
Actual Results:  
Completed tasks are shown again, the checkbox "Hide Completed Tasks" remains checked.

Expected Results:  
Completed tasks remain hidden.

It is a recent regression. Works in a 2007-11-21 build (checkout finish shortly before 02:00 PST), fails 2007-11-22 (checkout finish shortly before 10:00 PST).
Caused by the checkin in Bug 253396?
Happens as expected also on Windows and on Linux with trunk builds as well, changing OS to "All".

Mozilla 1.8 Branch + Trunk.
OS: Linux → All
Confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.10pre) Gecko/20071126 Calendar/0.8pre.

The following is broken with "Hide completed tasks" active:
- reload calendars makes the completed tasks visible again - not as expected
- after startup the completed tasks are visible - not as expected
- marking a task as completed doesn't remove it from the list - not as expected

Only the task list is affected. The calendar view work fine.

Regression Range:
  WORKS in Sunbird 0.8pre (2007-11-21-05)
  FAILS in Sunbird 0.8pre (2007-11-22-05)

Checkins during regression range: http://tinyurl.com/32ko8c

Seems to be caused by the checkin for Bug 253396.
Blocks: 253396
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Hardware: PC → All
Summary: "Reload remote calendars" unhides completed tasks → "Reload remote calendars" unhides completed tasks in task list
Version: unspecified → Mozilla 1.8 Branch
Summary: "Reload remote calendars" unhides completed tasks in task list → Hidden completed tasks are visible in task list after Startup resp. Reload Remote Calendars
Major regression and feature lost compared to Sunbird 0.7. In my opinion this should be fixed for 0.8.
Flags: wanted-calendar0.8?
Summary: Hidden completed tasks are visible in task list after Startup resp. Reload Remote Calendars → Hidden completed tasks are visible in task list after Startup or Reload Remote Calendars
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Attached patch patch v1 (obsolete) — Splinter Review
This patch should do the trick...
Assignee: nobody → michael.buettner
Status: NEW → ASSIGNED
Attachment #292076 - Flags: review?(Berend.Cornelius)
As far as I tested this patch it works fine when you remove or add calendars but the testcase mentioned in comment #3:
> - marking a task as completed doesn't remove it from the list - not as expected. 
is not covered by this patch. The functions "addItem" and "modifyItem" do not consider the state of the checkbox "Hide Completed Task".
As the initial description of this patch is resolved by your patch I leave it up to you if you want to fix this within this issue or if you want to set up a follow up.

The implementation of the patch is good. I would consolidate the assignment of the filters or probably even better consolidate the implementation of the methods "onCalendarAdded" and "Refresh". As far as I can see both methods do a refresh - one method on a single calendar and one on the composite calendar.

Attachment #292076 - Flags: review?(Berend.Cornelius) → review+
Attached patch patch v2Splinter Review
> The functions "addItem" and "modifyItem" do not consider the state of
> the checkbox "Hide Completed Task".
Good catch. This version of the patch adds the necessary bits and pieces. Re-requesting review since this in an integral part of this patch.

> I would consolidate the assignment of the filters or probably even
> better consolidate the implementation of the methods "onCalendarAdded"
> and "Refresh". As far as I can see both methods do a refresh - one method
> on a single calendar and one on the composite calendar.
onCalendarAdded() issues addItem() calls, therefore adds items but leaves the existing ones intact. refresh() collects all items and completely replaces the existing array. We could factor this out in a separate method, but I hardly believe this buys us any benefit. But I'm open for suggestions on how you'd do that...
Attachment #292076 - Attachment is obsolete: true
Attachment #292934 - Flags: review?(Berend.Cornelius)
When I tested this patch I still found that modifying a task is not possible.
Removing the completed state from a task when the "Hide Completed Tasks" checkbox is checked removes another task from the tree.
I modified the function modifyItem a bit and found that the following code works well.
Maybe you can try this:
>          modifyItem: function tTV_modifyItem(aNewItem, aOldItem) {
>            var index = this.binding.mHash2Index[aOldItem.hashId];
>              if (index != undefined) {
>                  if (aNewItem.isCompleted != aOldItem.isCompleted) {
>                      if (aNewItem.isCompleted && this.binding.hideCompleted) {
>                          this.removeItem(aNewItem);
>                          delete this.binding.mHash2Index[aOldItem.hashId];
>                          return;
>                      }
>                  }
>                  delete this.binding.mHash2Index[aOldItem.hashId];
>                  this.binding.mHash2Index[aNewItem.hashId] = index;
>                  this.binding.mTaskArray[index] = aNewItem;
>                  this.tree.view.selection.select(index);
>                  this.treebox.invalidateRow(index);
>              }
>          },
Attachment #292934 - Flags: review?(Berend.Cornelius) → review+
Berend, thanks for spotting the missing return statement. Deleting the key from the hash table isn't necessary, since removeItem() rebuilds the table from scratch. I change the patch accordingly before checking it in.
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Version: Mozilla 1.8 Branch → unspecified
Verified with Lightning 2008012900 and Mozilla/5.0 (Windows; U; Windows NT 5.1;
pl; rv:1.8.1.12pre) Gecko/20080127 Calendar/0.8pre
Status: RESOLVED → VERIFIED
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071031 Lightning/0.8pre (2008012818) Thunderbird/2.0.0.9 ID:2007103104

VERIFIED
You need to log in before you can comment on or make changes to this bug.