386596 - Option to show completed Tasks should be moved to the menu

Status: VERIFIED FIXED

(Calendar :: Preferences, defect)

Description

[Markus Adrario [:Taraman]]

This Option is positioned and named differently in Lightning and Sunbird.
This should be harmonized.
It results from a discussion in Bug 332193.

Comment 1

[Markus Adrario [:Taraman]]

Attachment: Patch v1.0

well - it seems to work... ;-)

Comment 2

[Christian Jansen]

Getting the "Show completed tasks" option out of the main view is the right approach. But I'm not sure, if this option makes sense in the Calendar menu [1, 2]. The option should stick to the context where it has effect on.

Momentarily we are figuring out, if it would make sense to use the Unifinder Task List instead of the current one [3]. The Unifinder Task List comes with a context menu [4] which should be extended by a "Customize..." entry. "Customize..." should call up a dialog [5], where users can modify the content of task list. From my point of the "Show completed tasks" option should be placed there, or as an interims solution directly in the Context menu.

[1] Calendar Mode Menu Proposal:
http://www.cjansen.com/lightning/calendar-mode-menu.xul

[2] Mail Mode Menu Proposal:
http://www.cjansen.com/lightning/mail-mode-menu.xul

[3] Bug 372830 – Integrate UnifinderToDo into Lightning as it is in Sunbird

[4] http://wiki.mozilla.org/Calendar:Mail_View_Integration#Task_Tree

[5] http://wiki.mozilla.org/Calendar:Mail_View_Integration#Customize_Tasks_List

Comment 3

[Markus Adrario [:Taraman]]

Since the option does not only affect the task list, but also the tasks which are displayed in the views, I think the location in the Menu is appropriate.
In Sunbird ist is in the view-Menu just below the "show Tasks in View" option.
In TB it is the same in the Calendar-->View Menu.
It acts on all views, so the view-Menu ist the place to look for it.

Comment 4

[Christian Jansen]

Thanks for the hint. I've downloaded a Sunbird nightly and I saw that the "Show completed tasks" options really affects nowadays the views. Which is from my point of view somehow strange... I think we would be doing good, if we separate this option into two parts:

- One "Show completed tasks" option for the Task List, which is
  by default switch ON.
  
- One "Show completed tasks" option for the Views, which is by default switch OFF

The "Show completed tasks" option for the Views should occur in the "Calendar Mode Menu"  under "View -> Current View -> ..." See proposal:

http://cjansen.com/ligthning/calendar-mode-menu.xul

I've also recognized that the "Show Compled Tasks"  has been renamed to "Hide Completed Tasks". IMHO this should turned back again to "Show". This would align much better with the Checkbox. Checked=Show, Unchecked=Hide; like the Calendars.

Comment 5

[Markus Adrario [:Taraman]]

Attachment: Screenshot of the new menu in sunbird

The Hide Completed was in there quite a while and I didn't like it as well.
So This patch introduces the change to "show completed" (as it already is in Lightning.)

If you have the option to show tasks in the view it is - in my opinion - the logical next step to also have the possibility to see the completed tasks. Why not link them together in one option to keep it simple?

To visualize it better, I attached a screenshot of Sunbird with the new menu.

Comment 6

[Markus Adrario [:Taraman]]

Attachment: the new look in Lightning

Comment 7

[Stefan Sitter [:ssitter]]

"Show completed" functionality has been removed from Lightning. Both Sunbird and Lightning now use the "Hide completed" -> Patch is obsolete.

See also related Bug 201664. If the option moves to the view menu Bug 201664 should be resolved wontfix.

Comment 8

[Markus Adrario [:Taraman]]

Now I've got some time again, I'd like start working on this again.
Christian: Is the solution of splitting the option for tasklist and views ok?
for now I would leave the option for the tasklist where it is but rename it to "show completed". This option will still also affect the agenda, but a discussion on that on #calendar-qa showed that that could very well be worth another discussion.
The option for the views wuld be placed as in the attechments to this bug.

Comment 9

[Christian Jansen]

Markus, +1 for that what you've proposed in comment #8.
It might happen that we'll have to do some changes (again) caused by the task mode...., but this is future stuff.

Comment 10

[Martin Schröder [:mschroeder]]

Comment on attachment 276304 [details] [diff] [review]
Patch v1.0

This patch is obsolete, and I think Markus will create an updated patch.

Comment 11

[Markus Adrario [:Taraman]]

Attachment: Patch V2.0

The Options arenow split:
- the "show-completed-tasks" checkbox in the sidebar only affects the tasklist and in Lightning the Agenda.
- The Views-Menu and the views-context-menu change the setting for the views.

I thought about greying the option, when "Tasks in view" is not checked, but I thought it would do no harm, if its accessible. So I just kept the view from updating then.

In messenger-overlay-sidebar.js I also found a ";" missing in the view initialization. It didn't seem to produce an error, but I added it anyway. If it was indeed correct as it was, please let me know...

Comment 12

[Markus Adrario [:Taraman]]

Attachment: new look in Sunbird

The menu in the menubar looks similar.

Comment 13

[Markus Adrario [:Taraman]]

Attachment: new llok in lightning

Comment 14

[Christian Jansen]

Thanks for spliting the option. I'm sure that the option will move if we change the menu structure, but this is a different to-do. ui=christian

Comment 15

[Martin Schröder [:mschroeder]]

Markus, please un-bitrot your patch.

patching file `calendar/base/content/calendar-unifinder-todo.js'
Hunk #1 succeeded at 170 (offset 3 lines).
patching file `calendar/base/content/calendar-unifinder-todo.xul'
Hunk #1 succeeded at 129 (offset 1 line).
patching file `calendar/lightning/content/agenda-tree.js'
patching file `calendar/lightning/content/messenger-overlay-sidebar.js'
Hunk #1 FAILED at 488.
Hunk #2 FAILED at 668.
2 out of 2 hunks FAILED -- saving rejects to calendar/lightning/content/messenger-overlay-sidebar.js.rej
patching file `calendar/lightning/content/messenger-overlay-sidebar.xul'
Hunk #2 FAILED at 144.
1 out of 2 hunks FAILED -- saving rejects to calendar/lightning/content/messenger-overlay-sidebar.xul.rej
patching file `calendar/locales/en-US/chrome/calendar/calendar.dtd'
patching file `calendar/locales/en-US/chrome/lightning/lightning.dtd'
Hunk #1 succeeded at 71 (offset 9 lines).
patching file `calendar/resources/content/calendar.js'
Hunk #1 succeeded at 24 (offset -1 lines).
Hunk #2 FAILED at 98.
Hunk #3 FAILED at 358.
2 out of 3 hunks FAILED -- saving rejects to calendar/resources/content/calendar.js.rej
patching file `calendar/sunbird/base/content/calendar-menubar.inc'
Hunk #1 FAILED at 284.
1 out of 1 hunk FAILED -- saving rejects to calendar/sunbird/base/content/calendar-menubar.inc.rej
patching file `calendar/sunbird/base/content/calendar-sets.inc'
Hunk #1 FAILED at 94.
1 out of 1 hunk FAILED -- saving rejects to calendar/sunbird/base/content/calendar-sets.inc.rej
patching file `calendar/sunbird/base/content/calendar.xul'
Hunk #1 FAILED at 148.
1 out of 1 hunk FAILED -- saving rejects to calendar/sunbird/base/content/calendar.xul.rej

Comment 16

[Martin Schröder [:mschroeder]]

Comment on attachment 283819 [details] [diff] [review]
Patch V2.0

r- because the patch is bitrotted.

Comment 17

[Martin Schröder [:mschroeder]]

Markus, a question and two style nits:

Index: calendar/base/content/calendar-unifinder-todo.js
===================================================================
[...]
 function toDoUnifinderRefresh()
 {
-   var hideCompleted = document.getElementById("hide-completed-checkbox").checked;
-
+   var showCompleted = (document.getElementById("show-completed-checkbox")
+                                .getAttribute("checked") == 'true');

Why do you use the attribute and not the property as before?

[...]
-   if (hideCompleted)
-       filter |= ccalendar.ITEM_FILTER_COMPLETED_NO;
-   else
+   if (showCompleted)
        filter |= ccalendar.ITEM_FILTER_COMPLETED_ALL;
+   else
+       filter |= ccalendar.ITEM_FILTER_COMPLETED_NO;
 
    filter |= ccalendar.ITEM_FILTER_TYPE_TODO;

Please introduce curly braces .

Index: calendar/lightning/content/messenger-overlay-sidebar.js
===================================================================
[...]
+function toggleCompletedTasksInView() {
+    for each (view in getViewDeck().childNodes) {
+        view.showCompleted = !view.showCompleted;
+    }
+
+    // Refresh the current view only if tasks are shown
+    if (currentView().tasksInView) {
+		dump("view updated.\n");
+        currentView().goToDay(currentView().selectedDay);
+    }
+}

Please remove the dump() call.

Comment 18

[Markus Adrario [:Taraman]]

(In reply to comment #17)
> Markus, a question and two style nits:
> 
> Index: calendar/base/content/calendar-unifinder-todo.js
> ===================================================================
> [...]
>  function toDoUnifinderRefresh()
>  {
> -   var hideCompleted =
> document.getElementById("hide-completed-checkbox").checked;
> -
> +   var showCompleted = (document.getElementById("show-completed-checkbox")
> +                                .getAttribute("checked") == 'true');
> 
> Why do you use the attribute and not the property as before?
Hi Martin,

On some point during programming the property didn't work for some reason...
I checked now and it works. So I will change that.
Thanks for the comments. As soon as I can spare some time, I will update the patch.

Comment 19

[Simon Paquet [:sipaq]]

Setting wanted0.8- as the main Calendar developers will not devote any time to
this in the 0.8 timeframe. Patches are, of course, always welcome.

Comment 20

[Markus Adrario [:Taraman]]

Attachment: Patch V3.0

Due to lots of code-changes I had to redo the patch nearly from scratch, so it took some time.
The above mentioned issues are fixed or obsolete.

During this I wondered if the Agenda in Lightning really should show completed tasks at all?
If so, they should be marked somehow.

Comment 21

[Martin Schröder [:mschroeder]]

Comment on attachment 293448 [details] [diff] [review]
Patch V3.0

Shifting review to Daniel because reviewing this patch is above my competence, and Mickey is not available as the first choice.

Comment 22

[Daniel Boelzle [:dbo]]

Comment on attachment 293448 [details] [diff] [review]
Patch V3.0

Moving over review to Berend.

Comment 23

[Berend Cornelius]

Comment on attachment 293448 [details] [diff] [review]
Patch V3.0

Markus, could you once again unbitrot your patch. Your patch on first sight looks Ok. So far I only noticed that you introduced an already existing attribute "showCompleted" in "CalIDecoratedView.idl". Moreover the agenda-pane is no longer bound to the task pane as it does not display tasks anymore

Comment 24

[Markus Adrario [:Taraman]]

Attachment: Patch V3.1

OK, patch is up-to-date again.
The attribute "showCompleted" in "CalIDecoratedView.idl" was in there already from an earlier patch of mine. Must have slipped in here sometime in the early morning.
Concerning the Agenda-pane, I still find the code for Tasks in "agenda-tree.js". Shall I leave it as it is, since it is obsolete then or change it, in case it will be needed again? The current patch contains the changes to "agenda-tree.js"

Comment 25

[Martin Schröder [:mschroeder]]

Comment on attachment 298337 [details] [diff] [review]
Patch V3.1

This patch has l10n changes... l10n freeze is 2008-01-22 05:00 CET. I don't think it will be reviewed in time.

Comment 26

[Martin Schröder [:mschroeder]]

Attachment: [checked in] Patch V3.1a (l10n part only)

This is the l10n part of Patch V3.1. We shouldn't remove the old entity if we want to check in the l10n part only.

Comment 27

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 298348 [details] [diff] [review]
[checked in] Patch V3.1a (l10n part only)

r=philipp

Comment 28

[Martin Schröder [:mschroeder]]

Comment on attachment 298348 [details] [diff] [review]
[checked in] Patch V3.1a (l10n part only)

Checked in on HEAD and MOZILLA_1_8_BRANCH

Comment 29

[Markus Adrario [:Taraman]]

Attachment: Patch V3.2

I unbitrotted the patch again.

The "hide complted tasks" label is now deleted from calendar.dtd because its not used anymore.

Comment 30

[Berend Cornelius]

Markus, I thank you for your patience! I will immediately look at your patch when I am back from vacation on April 24th.

Comment 31

[Markus Adrario [:Taraman]]

Attachment: Patch V3.3

Some files changed agein already...

Comment 32

[Berend Cornelius]

Patch looks good and works as advertised. 
Before I will check it in just some remarks about minor issues:

-I changed the checkbox string "Show Completed Tasks" to "Show completed Tasks"
-I slightly modified our new function toggleShowCompletedInView":

>+function toggleShowCompletedInView() {
>+    var cmd = >document.getElementById("calendar_toggle_show_completed_in_view_command");
>+    var newValue = (cmd.getAttribute("checked") == "true" ? "false" : "true");
>+    cmd.setAttribute("checked", newValue);
>+
>+    var deck = getViewDeck();
>+    for each (var view in deck.childNodes) {
>+        view.showCompleted = !view.showCompleted;
>+    }
>+
>+    // Refresh the current view
>+    currentView().goToDay(currentView().selectedDay);
>+}

I changed as follows:
>+        view.showCompleted = (newValue == "true");

similar to the other "toggle" functions "toggleTasksInView()..." in calendar-views.js. Your implementation has already lead to bug 409966...

Besides that I would have preferred a more generic approach to solve this. We now have the same logic in several implementations where property values that are derived from command attributes are assigned to deck elements before the view is refreshed. We could consolidate this by following a more generic approach like

view["aPropertyName"] = (newValue == "true");

I thank you again for your patience and will check in your patch!

Comment 33

[Berend Cornelius]

patch checked in on trunk and MOZILLA_1_8_BRANCH

->> FIXED

Comment 34

[Martin Schröder [:mschroeder]]

(In reply to comment #32)
> -I changed the checkbox string "Show Completed Tasks" to "Show completed Tasks"

I think, having a lowercase word in a label is inconsistent with our common practice.

Comment 35

[Berend Cornelius]

In reply to comment #34:
Well Christian and me found this was not the case. Especially if you look at the widgets in the preference dialog you see many labels with lower case. I came to think about this because I found it not so nice to have a menu item and a checkbox both labeled with the same description but a different orthography.

Comment 36

[Andreas Treumann]

Checked in lightning 2008062603 and Sunbird 20080626 -> VERIFIED.