460266 - 'Open task' menu via context menu is broken

Status: VERIFIED FIXED

(Calendar :: Tasks, defect)

Description

[Andreas Treumann]

STEPS TO REPRODUCE:
===================

- try to open the 'edit task' dialog via context menu in the today pane or in the task mode

RESULT:
=======

- nothing happens

EXPECTED RESULT:
================

- this should open the edit task dialog

REPRODUCIBLE:
=============

- always

Comment 1

[Andreas Treumann]

lightning build 2008101505

Comment 2

[Berend Cornelius]

Attachment: patch v. #1

I can imagine that you are not quite happy with the patch:
I soon found out that the observer of the task trees was not notified. 
The reason was that due to an unload event of the window the task-tree-observer was removed from the composite calendar.
I could not get any valuable stack information from where the unloadhandler was called (except for the unloadhandler defined in the constructor).
My solution to call the unload handler by the destructor is working fine but not knowing what the actual reason for the bug is makes me unsecure - maybe you find objections against my solution.
In this context I am not sure if we still should follow the consolidation process of the trees as the future of the Lightning calendar-unifinder in its current form is most unsecure.

Comment 3

[Berend Cornelius]

This patch should also solve  Bug 463014 -  Columns in task list are not persistent. 
As I tested the persist attribute may still not be set at the respective tree columns, so we still have to workaround this by setting an attribute containing all summary information at the tree directly.

Comment 4

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

Comment on attachment 349225 [details] [diff] [review]
patch v. #1

>diff --git a/calendar/base/content/calendar-common-sets.xul b/calendar/base/content/calendar-common-sets.xul
>--- a/calendar/base/content/calendar-common-sets.xul
>+++ b/calendar/base/content/calendar-common-sets.xul
>@@ -49,6 +49,7 @@
...
>+  <script type="application/javascript" src="chrome://calendar/content/calendar-task-tree.js"/>
Why do we need to include this script here? Isn't it enough in calendar-scripts.inc ?

>         var load = function tTV_loadHandler() {
>             self.onLoad();
>+            window.removeEventListener("load", self.onLoad, false);            
>+            
>         };
>         window.addEventListener("load", load, true);
This doesn't really make sense. you are adding the load handler on the "load" function, but removing self.onLoad. Please use:

>+            window.removeEventListener("load", arguments.callee, false);


>+      <destructor><![CDATA[
>+        this.onUnload();
>+      ]]></destructor>
I'd prefer moving the onUnload function directly into the destructor. The same goes for the onLoad function, I'd prefer seeing that in the constructor if at all possible. Looking at those functions I don't understand why they need to happen on window load/unload.

Also, I took a look at the onLoad/onUnload functions. Instead of counting loads I think it should be sufficient to do the change you did above (removing the load handler after firing it) and executing the function in any case. Please test and change those functions.

I don't think it makes sense to call self.onLoad() on window load while self.onUnload() is being called from the destructor. If for some reason more than one load occurrs then the destructor code will never execute.

r- for now, please fix the counting stuff and align the ctor/dtor code. I can reconsider if you explain to me why this needs to be the way it is.

Comment 5

[Berend Cornelius]

Attachment: [checked in] patch v. #2

Indeed it seems that the counting stuff as well as the load/unload handlers are not needed (don't know if they ever were). So I removed them and put everything into the constructor or respectively destructor. I apologize for having delivered such an incomplete patch but to be true I was just confused why and from where the unload event was fired and all together this made me a bit indecisive.

Comment 6

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

Comment on attachment 350777 [details] [diff] [review]
[checked in] patch v. #2

r=philipp codewise. Please consider moving to let for lines you change whitespaces in.

Comment 7

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

Andreas, could you give this patch some testing?

Comment 8

[Berend Cornelius]

pushed patch  'patch v. #2'  to comm-central:

http://hg.mozilla.org/comm-central/rev/d3029a40763c

->fixed.

>Andreas, could you give this patch some testing?

Andreas tested already patch v. #1 and found it worked all right. As the differences between the patches are not too big I hope it's all right to have pushed this to comm-central already now.

Comment 10

[Decathlon]

I can still see this bug only for tasks' context menu in today pane.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081210 Lightning/1.0pre (20081210) Shredder/3.0b2pre

Comment 11

[Decathlon]

It seems the problem is calendar_modify_todo_command that is never enabled when 'open task' is selected via context menu on today pane.

A patch like the follow works although it merely delete this.todo_tasktree_focused from calendar-common-sets.js file:

diff --git a/calendar/base/content/calendar-common-sets.js b/calendar/base/content/calendar-common-sets.js
--- a/calendar/base/content/calendar-common-sets.js
+++ b/calendar/base/content/calendar-common-sets.js
@@ -111,18 +111,17 @@ var calendarController = {
                 return this.writable && this.calendars_support_events;
             case "calendar_modify_event_command":
                 return this.item_selected;
             case "calendar_delete_event_command":
                 return this.selected_items_writable;
             case "calendar_new_todo_command":
                 return this.writable && this.calendars_support_tasks;
             case "calendar_modify_todo_command":
-                return this.todo_items_selected &&
-                       this.todo_tasktree_focused;
+                return this.todo_items_selected;
             case "calendar_task_filter_command":
                 return true;
             case "calendar_delete_todo_command":
             case "calendar_percentComplete-0_command":
             case "calendar_percentComplete-25_command":
             case "calendar_percentComplete-50_command":
             case "calendar_percentComplete-75_command":
             case "calendar_percentComplete-100_command":
diff --git a/calendar/base/content/calendar-common-sets.xul b/calendar/base/content/calendar-common-sets.xul
--- a/calendar/base/content/calendar-common-sets.xul
+++ b/calendar/base/content/calendar-common-sets.xul
@@ -257,17 +247,17 @@
                 observes="cmd_paste"/>
     </popup>
 
     <!-- TASK ITEM CONTEXT MENU -->
     <menupopup id="taskitem-context-menu" onpopupshowing="changeContextMenuForTask(event);">
       <menuitem id="task-context-menu-modify"
                 label="&calendar.context.modifyorviewtask.label;"
                 accesskey="&calendar.context.modifyorviewtask.accesskey;"
-                command="calendar_modify_todo_command"/>
+                observes="calendar_modify_todo_command"/>
       <menuitem id="task-context-menu-new"
                 label="&calendar.context.newtodo.label;"
                 accesskey="&calendar.context.newtodo.accesskey;"
                 key="calendar-new-todo-key"
                 command="calendar_new_todo_command"
                 observes="calendar_new_todo_command"/>
       <menuseparator id="calendar-menuseparator-beforemarkcompleted"/>
       <menuitem id="calendar-context-markcompleted"

Comment 12

[Berend Cornelius]

Attachment: [checked in] patch v. #3

I followed the hint of Decathlon in comment #11 and prepared a patch that works also for Windows. Nevertheless the question remains why the focus is lost while raising the popup menu and only on the unifinder-todo. I filed ¨Bug 469684 -  Unifinder-todo: raising of the context menu fires blur -event¨. Thank you for your assistance!

Comment 13

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

Comment on attachment 353058 [details] [diff] [review]
[checked in] patch v. #3

>+                return this.todo_items_selected;
>+// This code is temporarily commented out due to
>+// bug 469684 Unifinder-todo: raising of the context menu fires blur-event
>+//                       this.todo_tasktree_focused;
Please indent the comment to align with "return"

Comment 14

[Berend Cornelius]

patch v. #3 pushed to comm-central:

http://hg.mozilla.org/comm-central/rev/e1d46719ab40

->fixed

Comment 15

[Andreas Treumann]

checked in lightning build 20090106 -> VERIFIED.

Comment 16

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".