'Open task' menu via context menu is broken

VERIFIED FIXED in 1.0b1

Status

Calendar
Tasks
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Andreas Treumann, Assigned: Berend Cornelius)

Tracking

({regression})

Trunk
1.0b1
regression
Bug Flags:
blocking-calendar1.0 +
tb-integration +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
Flags: wanted-calendar1.0?
(Reporter)

Comment 1

10 years ago
lightning build 2008101505
OS: Windows XP → All
Hardware: PC → All

Updated

10 years ago
Flags: wanted-calendar1.0? → blocking-calendar1.0+
Flags: tb-integration?

Updated

10 years ago
Flags: tb-integration? → tb-integration+
(Assignee)

Updated

10 years ago
Assignee: nobody → Berend.Cornelius
(Assignee)

Comment 2

10 years ago
Created attachment 349225 [details] [diff] [review]
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.
Attachment #349225 - Flags: review?(philipp)
(Assignee)

Comment 3

10 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
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 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.
Attachment #349225 - Flags: review?(philipp) → review-
(Assignee)

Comment 5

10 years ago
Created attachment 350777 [details] [diff] [review]
[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.
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?
(Assignee)

Comment 8

10 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
(Reporter)

Updated

10 years ago
Duplicate of this bug: 463014
Status: RESOLVED → VERIFIED

Comment 10

10 years ago
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

10 years ago
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"
(Assignee)

Comment 12

10 years ago
Created attachment 353058 [details] [diff] [review]
[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!
Attachment #353058 - Flags: review?(philipp)
(Assignee)

Updated

10 years ago
Status: VERIFIED → REOPENED
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"
(Assignee)

Comment 14

10 years ago
patch v. #3 pushed to comm-central:

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

->fixed
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Attachment #353058 - Attachment description: patch v. #3 → [checked in] patch v. #3
(Reporter)

Comment 15

10 years ago
checked in lightning build 20090106 -> VERIFIED.
Status: RESOLVED → 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.