332193 - Agenda shows past and finished tasks

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Florian Schnabel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Thunderbird version 1.5 (20051201), Lightning Nightly from today (2006-03-30)

The "Agenda"-list still lists items that are marked as completed, also past completed items are still listed under "today" rendering the agenda view almost unusable

Reproducible: Always

Comment 1

[Stefan Sitter [:ssitter]]

Duplicate of bug 310258?

Comment 2

[Florian Schnabel]

nope ... bug 310258 only refers to todo list ... this one is about agenda
that is another thing i notice but did not report yet.

Comment 3

[Andreas Hölscher]

additional comment:
if thunderbird stays open for more than a day "today" and "tomorrow" still refer to the day thunderbird was started at (no refresh).

Comment 4

[Stefan Sitter [:ssitter]]

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

Comment 5

[Chris J]

I see the same problem on my Lightning (2006072608), but it looks like it may have been resolved on 329775. Could this be considered a duplicate of 329775?

Comment 6

[Robert Rothenberg]

I have this problem with Lightning 0.1 2006031006 in Thunderbird.

What makes this particularly annoying is that I cannot find a "purge completed" option.

The status should be changed from UNCONFIRMED to NEW. 

Comment 7

[Andras Keri]

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

Comment 8

[Andras Keri]

> I see the same problem on my Lightning (2006072608), but it looks like it may
> have been resolved on 329775. Could this be considered a duplicate of 329775?
Nope, but the root is the same I guess: Event and Todo list do not get refreshed periodically (like once every second).

Confirmed for build 2006093006.

Comment 9

[Alexander Grimalovsky]

Confirm this problem in 0.3 release for Windows (build 200610061). 
Quite disappointing to see lots of already completed tasks into today's view.

Comment 10

[Edward Rudd]

It also sees that the days never rotate either. as I leave thunderbird up for weeks at a time and lightning always thinks that "today" is the day that I started thunderbird.. 

Comment 11

[Florian Schnabel]

confirmed to still exist in version 0.3 (build 2006100206)

Comment 12

[Robert "Unlogic" Olofsson]

This bug still exists in Lightning 0.4a1 build 2006103107

Comment 13

[Xunhua Guo]

Glad to see it solved in nightly build 20061204.

Comment 14

[Matthew (lilmatt) Willis]

Florian,
Can you test with a recently nightly version and verify that this is indeed now working?

Comment 15

[Xunhua Guo]

(In reply to comment #13)
> Glad to see it solved in nightly build 20061204.
> 

Sorry I was wrong. It still exists.

Comment 16

[Markus Adrario [:Taraman]]

I like to add, that in the month view the completed tasks are also shown if you choose to show tasks in view.
It is because the filter settings in all these views are set to show all completed  items.
In Sunbird the same settings in the Month- and Multiday-Views don't produce this error - don't know why yet.
A solution to this problem for Lightning would be to set the filter-Rules to "ITEM_FILTER_COMPLETED_NO".
Since I think there is no reason to have completed items shown in this location that would be my preferred way.

Let me know your opinion and I create a patch for that.

Comment 17

[Florian Schnabel]

@markus yes that sounds nice :-)

confirmed the behaviour still exist in yesterdays nightly btw

Comment 18

[Markus Adrario [:Taraman]]

Attachment: Hide completed Tasks

This patch not only addresses the problem with the Lightning Agenda, but also with the Calendar-views where the completed Items are shown. The "hide completed"-checkbox (or "show completed" in lightning) is taken into account for that.
The calendar-views show the effect of "completing" an item with the checkbox right away. Unfortunately in the agenda in lightning seems to have a problem with the calendar-observer so it won't recognise the change and update the view.
Since I am not able to fix that now, I would suggest to open a new bug for that.

In the Views I decided not just to set the filter to the current "hide completed"-setting, because that won't update the view on change of a single item - unless I refresh the whole view. This would take much longer than just keeping the Item from being added to the view again in the change progress.

On changing the "hide completed"-setting the whole view and the agenda view in lightning are rebuilt.

Happy testing.
Markus

Comment 19

[Joey Minta]

Comment on attachment 264403 [details] [diff] [review]
Hide completed Tasks

I am no longer a peer in the calendar project, and, as a result, I am not an appropriate person to ask for a review here.

In passing, I'll note that under no circumstances should you use getElementById in an xbl binding.  Additionally, although I am not the UI reviewer, I'll note that it's rather unintuitive for a checkbox in one part of the application to alter another, unrelated part of the app.

Canceling review request.

Comment 20

[Daniel Boelzle [:dbo]]

Comment on attachment 264403 [details] [diff] [review]
Hide completed Tasks

shifting review on view code to mickey

Comment 21

[Michael Büttner (no reviews TFN)]

Comment on attachment 264403 [details] [diff] [review]
Hide completed Tasks

>+          // since the checkbox to show/hide completed ToDos is named differently in Sunbird
>+          // and Lightning, we have to check vor the brand.
>+          var hideCompleted = false;
>+          if (isSunbird()) {
>+            hideCompleted = document.getElementById("hide-completed-checkbox").checked;
>+          } else {
>+            hideCompleted = !document.getElementById("completed-tasks-checkbox").checked;
>+          }
This code makes the views fragile by introducing a direct relationship to the options of the application. The need to differentiate between sunbird and lightning is almost always an indication that the attempt is not optimal.

I'd suggest to add a new boolean attribute to the appropriate interface, see [1]. The checkbox that controls whether or not completed tasks should be displayed should also be moved to an appropriate location. But that's up to Christian.

>+           // since the checkbox to show/hide completed ToDos is named differently in Sunbird
>+           // and Lightning, we have to check vor the brand.
>+           var hideCompleted = false;
>+           if (isSunbird()) {
>+               hideCompleted = document.getElementById("hide-completed-checkbox").checked;
>+           } else {
>+               hideCompleted = !document.getElementById("completed-tasks-checkbox").checked;
>+           }
some comment as above.

>+    // since the checkbox to show/hide completed ToDos is named differently in Sunbird
>+    // and Lightning, we have to check vor the brand.
>+    var hideCompleted = false;
>+    if (isSunbird()) {
>+        hideCompleted = document.getElementById("hide-completed-checkbox").checked;
>+    } else {
>+        hideCompleted = !document.getElementById("completed-tasks-checkbox").checked;
>+    }
and again in the agenda code. as a side note, modeling this feature by introducing a new attribute in calIDecoratedView would also avoid code duplication here.

>     document.getElementById("calendar-todo-list").showCompleted =
>         !document.getElementById("calendar-todo-list").showCompleted;
>+
>+    currentView().viewElem.refresh();
>+    agendaTreeView.refreshCalendarQuery();
>+    agendaTreeView.rebuildAgendaView();
It would be much better if the views observe this option and refresh themselves. At least it would separate the responsibilities in a natural way and follow object oriented design better.

[1] http://lxr.mozilla.org/seamonkey/source/calendar/base/public/calIDecoratedView.idl#100

Comment 22

[Markus Adrario [:Taraman]]

Attachment: Patch V1.5

This is an intermediate version of the patch, incorporating mickey's comments.
It works for the views and the Lightning Agenda.
Changing the "Show Completed" checkbox is not observed by the views yet.

In calendar.js there seems to be a change in the latest cvs Version that is not mine. But I don't have time now to check this before leaving for holiday for a week, so I leave it like it is fo now... Anyway I wanted this to be online, so I can look forward to the comments when I come back :-)

Comment 23

[Michael Büttner (no reviews TFN)]

This might be of public interest, it's a snippet from some mail correspondence I had with Markus...

> I took a try at your suggestion and the interface and implementation in the views are done.
> But I have a problem with making an observer for the "Hide Completed" (or "show Completed" in Lightning) Checkox.
> I hoped I could do something like with the "tasks in View" setting, but the toggleTasksInView Method just calls rebuildView, which I understood was not desireable.
>
probably i didn't make my point clear, sorry for that. i didn't say that rebuildDisplay() is in any way not the way to go. rebuildDisplay() is just the internal method of the todo-list, it just rebuilds the internal stuff of that list only. it is just unrelated to what you're trying to do. see [1] for the handler that gets called upon changing the state of the checkbox, we're talking about. this just grabs the todo list and toggles its state (by setting a property, which in turn calls rebuildDisplay()). you're now trying to let the views know about that. see [2] for how this happens in case the "tasks in view" switch has been toggled. it just iterates the views and sets a property (which in turn gives the views to take appropriate actions - triggering a refresh, etc). basically you could take the snippet from [2] and paste the appropriately modified version to [1]. 

[1] http://lxr.mozilla.org/seamonkey/source/calendar/lightning/content/todo-list.js#77
[2] http://lxr.mozilla.org/seamonkey/source/calendar/lightning/content/messenger-overlay-sidebar.js#520

Comment 24

[Markus Adrario [:Taraman]]

Attachment: Patch V2

So, this seems to work well and I hope I got all the adressed issues in.

Comment 25

[Simon Paquet [:sipaq]]

Comment on attachment 266802 [details] [diff] [review]
Patch V2

Markus, please always request review from a person. I've moved this review to mickey, since he already reviewed the first patch iteration.

Comment 26

[Markus Adrario [:Taraman]]

Thx Simon.
I had mickey in there - at least I thought so.
Have been working too much lately...

Comment 27

[Michael Büttner (no reviews TFN)]

Comment on attachment 266802 [details] [diff] [review]
Patch V2

>           onAddItem: function (aItem) {
>             if (this.mBatchCount) {
>                 return;
>             }
> 
>             if (isToDo(aItem) && !this.calView.mTasksInView) {
>-                return;
>+              return;
>+            }
>+
>+            if (aItem.isCompleted && !this.calView.mShowCompleted) {
>+              return;
>             }
Please keep the same identation, the above code uses 4 blanks per tab. Furthermore, 'isCompleted' is a boolean attribute that's defined for tasks only. I'd suggest to structure the code as follows:

              if (isToDo(aItem)) {
                  if (!this.calView.mTasksInView) {
                      return;
                  }
                  if (aItem.isCompleted && !this.calView.mShowCompleted) {
                      return;
                  }
              }

This makes the code clearer and has the additional benefit of not executing the query for events.

>           onModifyItem: function (aNewItem, aOldItem) {
>             if (this.mBatchCount) {
>                 return;
>             }
> 
>             if (isToDo(aNewItem) && isToDo(aOldItem) &&
>                 !this.calView.mTasksInView) {
>-                return;
>+              return;
>             }
Watch identation.

> 
>             var occs;
>             occs = aOldItem.getOccurrencesBetween(this.calView.startDate,
>                                                   this.calView.queryEndDate,
>                                                   {});
>             for each (var occ in occs)
>               this.calView.doDeleteItem(occ);
> 
>             if (isToDo(aNewItem) && !this.calView.mTasksInView) {
>-                return;
>+              return;
>+            }
Watch identation.

>+
>+            if (aNewItem.isCompleted && !this.calView.mShowCompleted) {
>+              return;
>             }
Please see my comment from above and don't run the query for events.

>+
The line above introduces blanks.

>           onDeleteItem: function (aItem) {
>             if (this.mBatchCount) {
>                 return;
>             }
> 
>             if (isToDo(aItem) && !this.calView.mTasksInView) {
>                 return;
>             }
> 
>+            if (aItem.isCompleted && !this.calView.mShowCompleted) {
>+              return;
>+            }
>+
Please see my comment from above and don't run the query for events.

>       <field name="mObserver"><![CDATA[
>         // the calIObserver, and calICompositeObserver
>         ({
>             QueryInterface: function QueryInterface(aIID) {
>@@ -1927,17 +1929,21 @@
>                 if (this.mBatchCount) {
>                     return;
>                 }
> 
>                 if (isToDo(aItem) && (!aItem.entryDate || !aItem.dueDate)) {
>                     return;
>                 }
> 
>-                if (isToDo(aItem) && !this.calView.mTasksInView) {
>+                if (isToDo(aItem) && !this.calView.mTasksInView){
>+                    return;
>+                }
>+
>+                if (aItem.isCompleted && !this.calView.mShowCompleted) {
>                     return;
>                 }
Same here, don't run the query for events. You could be smart and check isToDo() just once and consolidate the code a bit.

>+                if (aNewItem.isCompleted && !this.calView.mShowCompleted) {
>+                    return;
>+                }
Same here, don't run the query for events.

>             onDeleteItem: function onDeleteItem(aItem) {
>@@ -1986,16 +1996,20 @@
>                 if (isToDo(aItem) && !this.calView.mTasksInView) {
>                     return;
>                 }
> 
>                 if (isToDo(aItem) && (!aItem.entryDate || !aItem.dueDate)) {
>                     return;
>                 }
> 
>+                if (aItem.isCompleted && !this.calView.mShowCompleted) {
>+                    return;
>+                }
Same here, don't run the query for events.

> agendaTreeView.calendarObserver.onAddItem =
> function observer_onAddItem(item)
> {
>     if (this.mBatchCount) {
>         return;
>     }
>+
>+    var showCompleted = document.getElementById("completed-tasks-checkbox").checked;
>+    if (item.isCompleted && !showCompleted) {
>+        return;
>+    }
Same here, don't run the query for events. This piece of code is a bit ugly since it searches a totally unrelated element in the tree I think we just go ahead as it stands since other codes follows exactly the same pattern. Another reason why I don't care for now is that we're going to move the "show completed" option into the menu very soon.

> agendaTreeView.calendarObserver.onDeleteItem =
> function observer_onDeleteItem(item, rebuildFlag)
>@@ -526,16 +538,21 @@ function observer_onDeleteItem(item, reb
> 
> agendaTreeView.calendarObserver.onModifyItem =
> function observer_onModifyItem(newItem, oldItem)
> {
>     if (this.mBatchCount) {
>         return;
>     }
>     this.onDeleteItem(oldItem, "no-rebuild");
>+
>+    var showCompleted = document.getElementById("completed-tasks-checkbox").checked;
>+    if (item.isCompleted && !showCompleted) {
>+        return;
>+    }
>     this.onAddItem(newItem);
> };
Same here, don't run the query for events. Don't check the state of the checkbox if this item is an event.

>                 this.todoList.removeTodo(aOldItem);
>-                this.todoList.addTodo(aNewItem);
>+                if (this.todoList.mShowCompleted || !aNewItem.isCompleted)
>+                    this.todoList.addTodo(aNewItem);
Same here, don't run the query for events.

Generally, the patch looks much better than the previous version. Great work, we're almost there. Marking r- since there are quite some bits and pieces that need to be addressed. Keep up the good work.

Comment 28

[Markus Adrario [:Taraman]]

> (From update of attachment 266802 [details] [diff] [review])
> >           onAddItem: function (aItem) {
> >             if (this.mBatchCount) {
> >                 return;
> >             }
> > 
> >             if (isToDo(aItem) && !this.calView.mTasksInView) {
> >-                return;
> >+              return;
> >+            }
> >+
> >+            if (aItem.isCompleted && !this.calView.mShowCompleted) {
> >+              return;
> >             }
> Please keep the same identation, the above code uses 4 blanks per tab.
In fact I tried to correct the indentation, because the rest of the file uses 2 blanks indentation. Only the first line that appears in this part of the patch uses 4.
I corrected that in one place and overlooked this first line.
In the next version of the patch should I leave the inconsistency in there and work with 2 blanks in the new code or correct the indentation where necessary?

Comment 29

[Michael Büttner (no reviews TFN)]

(In reply to comment #28)
> In the next version of the patch should I leave the inconsistency in there and
> work with 2 blanks in the new code or correct the indentation where necessary?
Feel free to fix wrong indentations at lines that you're touching anyway. I'd suggest to leave anything else as is, just in order to make the patch as small as possible.

Comment 30

[Markus Adrario [:Taraman]]

Attachment: Patch V3

I changed the .isCompleted queries so as to only check on todo-items and looked again for the indentation.

in calendar-todo-list.xml at:
>@@ -95,17 +95,18 @@
>             },
>             
>             onModifyItem: function onModifyItem(aNewItem, aOldItem)
>             {
>                 if (!(aNewItem instanceof Components.interfaces.calITodo) ||
>                     this.mBatchCount)
>                     return;
>                 this.todoList.removeTodo(aOldItem);
>-                this.todoList.addTodo(aNewItem);
>+                if (this.todoList.mShowCompleted || !aNewItem.isCompleted)
>+                    this.todoList.addTodo(aNewItem);
>             },
I didn't change it because the first if-statement in the onModifyItem function exits if it isn't a todo.

Comment 31

[Markus Adrario [:Taraman]]

Comment on attachment 269874 [details] [diff] [review]
Patch V3

I just discovered some errors in the patch, so please excuse me for not testing it thoroughly enough and disregard the patch...

Comment 32

[Markus Adrario [:Taraman]]

Attachment: patch V3 fixed

After fixes same as last comment for patch V3

In addition I added observing changes of the Hide-Completed checkbox in Sunbird, so the view is updated on clicking.
In calendar-ToDo-list.xml for lightning I added the check for completed Tasks vs. the setting to onAddItem and onDeleteItem so that Items that are created and set completed in the dialog right away (whyever someone would do that) don't show up in the todo List if show completed is not selected. Also the Program won't try to delete Items which are abviously not in view.
That is to stay consistent with all the other views.

Comment 33

[Christian Jansen]

Comment on attachment 270226 [details] [diff] [review]
patch V3 fixed

From an UI-perspective the patch is (nearly) ok, we should add a "Show Completed Task" entry to the "Calendar" menu. I suggest to put the entry under:
Calendar -> View

add a seperator above "show tasks in calendar"
add 
"show completet tasks" below "show tasks in calendar"

Comment 34

[Markus Adrario [:Taraman]]

(In reply to comment #21)
> The checkbox that controls whether or not completed tasks should be
> displayed should also be moved to an appropriate location. But that's up to
> Christian.

As you can see Mickey also adressed this issue and I understood it so that you would implement that.
I could also work on it but would prefer to do that as a seperate bug.

Comment 35

[Michael Büttner (no reviews TFN)]

I support the request of addressing this issue in a separate bug. Markus, it would be more than welcome if you could do just that and also volunteer for providing a patch ;-)

Comment 36

[Markus Adrario [:Taraman]]

Entered as Bug 386596

Comment 37

[Michael Büttner (no reviews TFN)]

Comment on attachment 270226 [details] [diff] [review]
patch V3 fixed

>? mozconfig.debug
>? obj-sb-static
The first two lines of the patch are garbage...

>+          var filter = null;
>+          if (this.showCompleted) {
>+            filter = this.mCalendar.ITEM_FILTER_COMPLETED_ALL;
>+          } else {
>+            filter = this.mCalendar.ITEM_FILTER_COMPLETED_NO;
>+          }
>+          filter |= this.mCalendar.ITEM_FILTER_CLASS_OCCURRENCES;
I'd write this as follows, since I find it easier to read:
            var filter = this.mCalendar.ITEM_FILTER_CLASS_OCCURRENCES;
            if (this.showCompleted) {
              filter |= this.mCalendar.ITEM_FILTER_COMPLETED_ALL;
            } else {
              filter |= this.mCalendar.ITEM_FILTER_COMPLETED_NO;
            }

>+                if (isToDo(aItem)) {
>+                    if (!aItem.entryDate || !aItem.dueDate) {
>+                        return;
>+                    }
>+                    if(!this.calView.mTasksInView){
>+                        return;
>+                    }
Here's a missing ^^^ blank between if and the opening brace.
>+                    if (aItem.isCompleted && !this.calView.mShowCompleted) {
>+                        return;
>+                    }
>                 }

>+                if (isToDo(aItem)) {
>+                    if(!this.calView.mTasksInView) {
>+                        return;
>+                    }
Here's a missing ^^^ blank between if and the opening brace.
>+                    if (!aItem.entryDate || !aItem.dueDate) {
>+                        return;
>+                    }
>+                    if (aItem.isCompleted && !this.calView.mShowCompleted) {
>+                        return;
>+                    }
>                 }

>-          var filter = this.mCalendar.ITEM_FILTER_COMPLETED_ALL |
>-                       this.mCalendar.ITEM_FILTER_CLASS_OCCURRENCES;
>+          var filter = null;
>+          if(this.mShowCompleted)
Here's a missing ^^^ blank between if and the opening brace.
>+            filter = this.mCalendar.ITEM_FILTER_COMPLETED_ALL;
>+          else
>+            filter = this.mCalendar.ITEM_FILTER_COMPLETED_NO;
>+
>+          filter |= this.mCalendar.ITEM_FILTER_CLASS_OCCURRENCES;
Furthermore, I'd add some curly braces to the if-statement.

>+    var filter = null;
>+    if (document.getElementById("completed-tasks-checkbox").checked)
>+        filter = this.calendar.ITEM_FILTER_COMPLETED_ALL;
>+    else
>+        filter = this.calendar.ITEM_FILTER_COMPLETED_NO;
>+
>+    filter |= this.calendar.ITEM_FILTER_CLASS_OCCURRENCES;
Again, I'd add the braces to the if-statement and initialize the filter with ITEM_FILTER_CLASS_OCCURRENCES instead of adding it at the end.

>+
>+        view.showCompleted = document.getElementById("completed-tasks-checkbox").checked;
I'd would want to add blank lines here.

Since these are just minor style nits, I'm going to fix those before checking the patch in. Furthermore, I'll remind Christian that he didn't set the review flag on this bug. Sorry, that the review took so long. Feel free to poke me if this happens again. Thanks for the patch, r=mickey.

Comment 38

[Michael Büttner (no reviews TFN)]

Attachment: final patch

patch as it is going to be checked in, all nits from review addressed, carried over review flag.

Comment 39

[Christian Jansen]

Comment on attachment 271522 [details] [diff] [review]
final patch

ui‑review=+

Comment 40

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 42

[Omar Bajraszewski]

Marking a task as a completed doesn't remove it from agenda

Comment 43

[Martin Schröder [:mschroeder]]

(In reply to comment #42)
> Marking a task as a completed doesn't remove it from agenda
> 

Omar, please file a new bug for this problem.

Comment 44

[Omar Bajraszewski]

(In reply to comment #43)

> Omar, please file a new bug for this problem.
> 

It works for me now: I forgot to check 'hide completed tasks'. 

Verified with lightning 0.7 RC1