Closed Bug 460266 Opened 11 years ago Closed 11 years ago

'Open task' menu via context menu is broken


(Calendar :: Tasks, defect)

Not set


(Not tracked)



(Reporter: andreas.treumann, Assigned: berend.cornelius09)



(Keywords: regression)


(2 files, 1 obsolete file)


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


- nothing happens


- this should open the edit task dialog


- always
Flags: wanted-calendar1.0?
lightning build 2008101505
OS: Windows XP → All
Hardware: PC → All
Flags: wanted-calendar1.0? → blocking-calendar1.0+
Flags: tb-integration?
Flags: tb-integration? → tb-integration+
Assignee: nobody → Berend.Cornelius
Attached patch patch v. #1 (obsolete) — Splinter Review
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.
Attachment #349225 - Flags: review?(philipp)
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.
Blocks: 463014
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 ?

>         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.
Attachment #349225 - Flags: review?(philipp) → review-
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.
Attachment #349225 - Attachment is obsolete: true
Attachment #350777 - Flags: review?(philipp)
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.
Attachment #350777 - Flags: review?(philipp) → review+
Andreas, could you give this patch some testing?
pushed patch  'patch v. #2'  to comm-central:


>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.
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Duplicate of this bug: 463014
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
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 @@
     <menupopup id="taskitem-context-menu" onpopupshowing="changeContextMenuForTask(event);">
       <menuitem id="task-context-menu-modify"
-                command="calendar_modify_todo_command"/>
+                observes="calendar_modify_todo_command"/>
       <menuitem id="task-context-menu-new"
       <menuseparator id="calendar-menuseparator-beforemarkcompleted"/>
       <menuitem id="calendar-context-markcompleted"
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!
Attachment #353058 - Flags: review?(philipp)
Resolution: FIXED → ---
Attachment #350777 - Attachment description: patch v. #2 → [checked in] patch v. #2
Attachment #353058 - Flags: review?(philipp) → review+
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"
patch v. #3 pushed to comm-central:

Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #353058 - Attachment description: patch v. #3 → [checked in] patch v. #3
checked in lightning build 20090106 -> VERIFIED.
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".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.