Last Comment Bug 746524 - Incomplete Tasks filter should include tasks with future start dates
: Incomplete Tasks filter should include tasks with future start dates
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Tasks (show other bugs)
: Lightning 1.4
: All All
: -- normal (vote)
: 1.9
Assigned To: Matthew Mecca [:mmecca]
:
Mentors:
Depends on: 529771 745081
Blocks: 673751
  Show dependency treegraph
 
Reported: 2012-04-18 05:08 PDT by jwq
Modified: 2012-10-02 19:59 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix v1 (23.45 KB, patch)
2012-04-21 22:45 PDT, Matthew Mecca [:mmecca]
no flags Details | Diff | Splinter Review
Fix v2 (23.48 KB, patch)
2012-07-01 14:59 PDT, Matthew Mecca [:mmecca]
philipp: review+
philipp: approval‑calendar‑aurora+
Details | Diff | Splinter Review
[checked-in to comm-central] Final (23.94 KB, patch)
2012-10-02 09:12 PDT, Matthew Mecca [:mmecca]
matthew.mecca: review+
matthew.mecca: approval‑calendar‑aurora+
Details | Diff | Splinter Review

Description jwq 2012-04-18 05:08:29 PDT
The Incomplete Tasks filter has been broken by a recent checkin (probably Bug 350174, as discussed in Bug 617277). The result is that some tasks can't be viewed with any of the available task filters. Confusingly, the user can perform an innocuous-seeming action like setting a start date in the future or adding a reminder to a task and see as a result the incomplete task disappearing from the Incomplete Tasks view!

Steps to Reproduce:
1. Create a new task with only a title and a due date in the future (relative to whatever date is selected in the mini calendar) with the Incomplete Tasks filter active. This task will be visible in the task list.
2. Either (a) add a start date in the future to the task or (b) add to the task a "standard" reminder, i.e. not a custom reminder, which implicitly sets the start date to the same as the due date.

Actual results:
The task disappears from the Incomplete Tasks view (and, potentially, all other views except All). The only way to edit this task is through the All filter - which is non-trivial if a large number of completed tasks is present and the appropriate sort columns are not displayed.

Expected result:
The incomplete task remains visible in the Incomplete Tasks view, exactly as it used to.

 While I appreciate the sentiments in Bug 673751 Comment 3, the fix accepted for Bug 350174 has broken the UI. "Incomplete Tasks" should mean one thing and one thing only "tasks whose status is not complete". Either find a better fix for Bug 350174 or change the filter text so it accurately reflects the filter action. Something like

     [*] Incomplete Tasks whose start date (which might be 
         implicitly set by setting a reminder) is not in the  
         future relative to the date currently selected in the 
         mini calendar

If that's deemed too big to fit in the UI then I'd strongly suggest that's evidence that it was a mistake to make the change to the Incomplete Tasks filter in the first place.
Comment 1 Matthew Mecca [:mmecca] 2012-04-18 06:41:44 PDT
(In reply to jwq from comment #0)
> The task disappears from the Incomplete Tasks view (and, potentially, all
> other views except All). The only way to edit this task is through the All
> filter - which is non-trivial if a large number of completed tasks is
> present and the appropriate sort columns are not displayed.

You can also select a future date in the mini-month to expand the date range for the Incomplete filter, but I agree that isn't very intuitive.

>      [*] Incomplete Tasks whose start date (which might be 
>          implicitly set by setting a reminder) is not in the  
>          future relative to the date currently selected in the 
>          mini calendar

I think "Incomplete Current Tasks" would work, but that doesn't really solve the problem of not being able to show future incomplete tasks. 

Expanding the filtering options as proposed in Bug 728011 will allow a custom filter that would show an unbound date range like the All filter without including completed tasks, which would at least work for non-repeating tasks. 

I think the real fix for this will require providing a filter option to show the next matching occurrence for repeating tasks with an unbound filter date range.
Comment 2 Matthew Mecca [:mmecca] 2012-04-21 22:45:38 PDT
Created attachment 617281 [details] [diff] [review]
Fix v1

Adds a new occurrences filter property to specify how occurrences are returned, including an option to show past occurrences and the next matching occurrence. The Incomplete task list filter now uses this filter with an unbound date range.

The search for the next matching occurrence is potentially resource intensive, so is limited by the new preference calendar.filter.maxiterations.

The same argument for an unbound date range on the Incomplete filter could also be applied to the Completed filter, but I think that due to the higher number of completed tasks in the typical use case, the need for displaying future completed tasks is more of an edge case. I suggest leaving the Completed filter date bound by default, if future completed tasks are wanted that should be possible with a custom filter after Bug 728011.
Comment 3 Matthew Mecca [:mmecca] 2012-07-01 14:59:33 PDT
Created attachment 638229 [details] [diff] [review]
Fix v2

de-bitrotted, requires patch for Bug 529771
Comment 4 Philipp Kewisch [:Fallen] 2012-08-20 13:08:07 PDT
Comment on attachment 638229 [details] [diff] [review]
Fix v2

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

Just some small comments below. r=philipp

::: calendar/base/content/calendar-task-tree.xml
@@ +416,5 @@
>                    }
>                }, this);
>  
> +              // modified items need to be update
> +              modItems.mArray.forEach(function(item) {

No need to create an extra function, you can just use for each() here.

@@ +430,5 @@
>                    this.treebox.rowCountChanged(index, -1);
>                }, this);
>  
>                // add the new items
> +              addItems.mArray.forEach(function(item) {

Same here

@@ +953,5 @@
>                    this.calendar = aCalendar;
>                    this.items = [];
>  
> +                  let op = this.binding.mFilter.getItems(aCalendar,
> +                                                         aCalendar.ITEM_FILTER_TYPE_TODO,

I guess we should revisit the ITEM_FILTER_COMPLETED_* at some point. If you need to get all items anyway, then we are not saving processing time anymore, which was the original intent of this item filter.

I'm happy to do this in a separate bug, but if possible it would be great if you could look into it. I could imagine something like a custom filter for providers, where optimized providers could use direct queries, and not-so-optimized providers could use some default implementation that filters live.

::: calendar/base/src/calFilter.js
@@ +778,5 @@
> +                start = next.startDate || next.entryDate;
> +            }
> +
> +            // we've hit the maximum number of iterations without finding a match
> +            cal.WARN("[calFilter] getNextOccurrence: reached maximum iterations for " + aItem.title);

Is this going to happen often?

@@ +856,5 @@
> +        // we use a local proxy listener for the calICalendar.getItems() call, and use it
> +        // to handle occurrence expansion and filter the results before forwarding them to
> +        // the listener passed in the aListener argument.
> +        let listener = {
> +            cf: this,

I'd prefer using:

let self = this;
let listener = {
  ...

};

cf works, but others casually reading the code will have to look up cf each time :)

@@ +859,5 @@
> +        let listener = {
> +            cf: this,
> +
> +            onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDateTime) {
> +                aListener.onOperationComplete(aCalendar, aStatus, aOperationType, aId, aDateTime);

you could also use:

onOperationComplete: aListener.onOperationComplete.bind(aListener),

@@ +871,5 @@
> +                    // function handle occurrence expansion.
> +                    items = [];
> +                    aItems.forEach(function(item) {
> +                        items = items.concat(this.cf.getOccurrences(item));
> +                    }, this);

You can use a simple for each() here.

@@ +878,5 @@
> +                    // return expanded occurrences appropriately, we only need to filter them.
> +                    items = this.cf.filterItems(aItems);
> +                }
> +
> +                aListener.onGetResult(aCalendar, aStatus, aItemType, aDetail, items.length, items);

aListener.onGetResult.apply(aListener, arguments);
Comment 5 Matthew Mecca [:mmecca] 2012-08-26 20:32:30 PDT
(In reply to Philipp Kewisch [:Fallen] from comment #4)
> @@ +953,5 @@
> I guess we should revisit the ITEM_FILTER_COMPLETED_* at some point. If you
> need to get all items anyway, then we are not saving processing time
> anymore, which was the original intent of this item filter.

calFilter.getItems still uses these for the internal getItems calls, based on the applied filter properties.

> 
> I'm happy to do this in a separate bug, but if possible it would be great if
> you could look into it. I could imagine something like a custom filter for
> providers, where optimized providers could use direct queries, and
> not-so-optimized providers could use some default implementation that
> filters live.

Maybe we could pass a calFilterProperties object to the providers instead of the ITEM_FILTER flags? I could also see making calFilter available for the providers to use internally.

 
> ::: calendar/base/src/calFilter.js
> @@ +778,5 @@
> > +                start = next.startDate || next.entryDate;
> > +            }
> > +
> > +            // we've hit the maximum number of iterations without finding a match
> > +            cal.WARN("[calFilter] getNextOccurrence: reached maximum iterations for " + aItem.title);
> 
> Is this going to happen often?

This shouldn't happen very often under normal use cases.


> @@ +878,5 @@
> > +                    // return expanded occurrences appropriately, we only need to filter them.
> > +                    items = this.cf.filterItems(aItems);
> > +                }
> > +
> > +                aListener.onGetResult(aCalendar, aStatus, aItemType, aDetail, items.length, items);
> 
> aListener.onGetResult.apply(aListener, arguments);

I don't think this will work, since we need to override the aCount and aItems arguments with the filtered item count and items.
Comment 6 Philipp Kewisch [:Fallen] 2012-08-27 09:43:24 PDT
(In reply to Matthew Mecca [:mmecca] from comment #5)
> Maybe we could pass a calFilterProperties object to the providers instead of
> the ITEM_FILTER flags? I could also see making calFilter available for the
> providers to use internally.
Yeah, something like that makes sense. Definitely a different bug though :)


> > @@ +878,5 @@
> > > +                    // return expanded occurrences appropriately, we only need to filter them.
> > > +                    items = this.cf.filterItems(aItems);
> > > +                }
> > > +
> > > +                aListener.onGetResult(aCalendar, aStatus, aItemType, aDetail, items.length, items);
> > 
> > aListener.onGetResult.apply(aListener, arguments);
> 
> I don't think this will work, since we need to override the aCount and
> aItems arguments with the filtered item count and items.

Ah sorry, I didn't read this very closely it seems.
Comment 7 Matthew Mecca [:mmecca] 2012-09-02 13:56:43 PDT
Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/e77ae46c56d5
Comment 8 Stefan Sitter 2012-10-01 14:55:43 PDT
Comment on attachment 638229 [details] [diff] [review]
Fix v2

According to comment 0 some tasks will not be displayed with any filter applied > kind of dataloss. Maybe we should port this back to Lightning 1.9.
Comment 9 Philipp Kewisch [:Fallen] 2012-10-02 08:07:54 PDT
Comment on attachment 638229 [details] [diff] [review]
Fix v2

a=philipp. I can't take care of checkin atm, busy with calconnect. If noone gets to this before the merge, a=me for beta.
Comment 10 Stefan Sitter 2012-10-02 08:18:49 PDT
Matthew, is the attached patch identical to the one that was checked in? I'm not sure which  review comments have been addressed before checkin and which not.
Comment 11 Matthew Mecca [:mmecca] 2012-10-02 09:12:37 PDT
Created attachment 667021 [details] [diff] [review]
[checked-in to comm-central] Final

Final patch as checked in to comm-central for check-in to comm-aurora
Comment 12 Matthew Mecca [:mmecca] 2012-10-02 19:59:28 PDT
Pushed to comm-aurora - http://hg.mozilla.org/releases/comm-aurora/rev/50b39296cb20

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