Closed Bug 438546 Opened 16 years ago Closed 16 years ago

"Select All" (Ctrl+A) & "Undo" (Ctrl+Z) in Mail mode doesn't work anymore

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mschroeder, Assigned: berend.cornelius09)

References

Details

(Keywords: regression)

Attachments

(3 files)

"Select All" (Ctrl+A) in Mail mode doesn't work anymore, but unlike bug 412613 no message in error console.
Flags: blocking-calendar0.9?
Flags: blocking-calendar0.9? → blocking-calendar0.9+
Confirmed... If I type Ctrl-A in Mail mode, all of the events are selected in month view (when I switch to Calendar mode).
Perhaps related, these things also don't work anymore:

1) Ctrl-Z / "Edit > Undo" in Mail mode

2) The Delete command when right click in "Message Pane" (AKA "Preview Pane") in Mail mode.

(I haven't updated Thunderbird since 2 May 2008 -- still using 2.0.0.14 -- so think it's a Lightning problem.)
So where (in the lighning.xpi tree structure) would I start looking to work on it?  Is this where the keypress is handled by thunderbird and passed to lightning in a structure or would lightning be grabbing and processing all keypresses and passing back to thunderbird what it doesn't need?  
I posted on http://forums.mozillazine.org/viewtopic.php?f=46&t=675995&p=3471065&e=3471065 and got an answer "submit a patch".  100% accurate, but usless.

Any ideas where to start looking would help.

thanks.
The patch in bug 429685 probably caused these problems.  In particular I think that the following code must be restored to function cC_doCommand(aCommand) in "calendar-common-sets.js":

> if (this.defaultController && !this.isCalendarInForeground()) {
>     // If calendar is not in foreground, let the default controller take
>     // care. If we don't have a default controller (i.e sunbird), just
>     // continue.
>     this.defaultController.doCommand(aCommand);
>     return;
> }

I restored that code in the "default:" case of the first Switch statement and now Ctrl-A and Undo work again in Mail mode.

Unfortunately that doesn't fix another problem:  the delete command doesn't work in the context menu when I right click in Thunderbird's Preview Pane.  I've traced this second problem to the same Switch statement.  The following code that's in capital letters should execute but it doesn't:

> case "calendar_delete_event_command":
> case "cmd_delete":
> case "button_delete":
>     var focusedElement = document.commandDispatcher.focusedElement;
>     if (focusedElement) {
>         var focusedRichListbox = getParentNodeOrThis(focusedElement, "richlistbox");
>         if (focusedRichListbox && focusedRichListbox.id == "agenda-listbox") {
>             agendaListbox.deleteSelectedItem(false);
>         } else if (focusedElement.className == "calendar-task-tree") {
>             deleteToDoCommand(null, false);
>         } ELSE IF (THIS.DEFAULTCONTROLLER && !THIS.ISCALENDARINFOREGROUND()) {
>             THIS.DEFAULTCONTROLLER.DOCOMMAND(ACOMMAND);
>         } else {
>             deleteSelectedEvents();
>         }
>     }
>     break;
Would you mind showing your changes to us as in uni-diff notation? It is unclear to me where exactly to change the code in that file. Maybe you could also point to a SVN/CVS changeset where we can see which other patch you are referring to.

Sorry for asking for convenience here. I am used to linking comments to repositories in Trac. Is this not possible in Bugzilla?
Okay, I found the LXR service, still being a newbie and not having known about it before. You are probably referring to this changeset: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/calendar/base/content&command=DIFF_FRAMESET&file=calendar-common-sets.js&rev2=1.1.2.7&rev1=1.1.2.6

Consequently, in order to fix Ctrl-A, the following patch needs to be applied:

--- calendar-common-sets.js.ori	Fri Jun 20 09:52:20 2008
+++ calendar-common-sets.js	Fri Jun 20 09:48:38 2008
@@ -200,6 +200,13 @@
     },
 
     doCommand: function cC_doCommand(aCommand) {
+        if (this.defaultController && !this.isCalendarInForeground()) {
+            // If calendar is not in foreground, let the default controller take
+            // care. If we don't have a default controller (i.e sunbird), just
+            // continue.
+            this.defaultController.doCommand(aCommand);
+            return;
+        }
         switch (aCommand) {
             // Common Commands
             case "calendar_new_event_command":
Summary: "Select All" (Ctrl+A) in Mail mode doesn't work anymore → "Select All" (Ctrl+A) & "Undo" (Ctrl+Z) in Mail mode doesn't work anymore
Alexander, thanks for quickly offering a fix, I hope that the calendar developers will also give this bug their full attention.

I helped as much as I could (took me several hours), but I know nothing about Mozilla's CVS so I didn't post anything in "uni-diff notation", but hoped that I found the source of the problems.

Regarding your patch, I suspect that you might have to put the lost IF statement at the end of the first Switch statement in its "default:" section instead of before the Switch statement.  I'm guessing this is true so that events that fire from the today pane can be properly handled even when Thunderbird is in Mail Mode.

Honestly, however, I don't think you're going to make much progress until Berend has time to take ownership of this bug.  In addition to the problem that you fixed, there is the second problem that I mentioned (delete in context menu is broken) that I think Berend is most qualified to investigate since it's "his baby".
It is not my fix, but yours. The credit goes to you. I know nothing about this module and just tried to find a way to apply it as a quick workaround until somebody takes care of it more thoroughly. At least it fixes my Ctrl-A problem, the delete case is not one I use a lot. I know that this patch is not the "real" fix.
Well and done, but who or when will this patch be applied to the lightning src?  It still is not there as of 20080623.  I've been self applying it but it needs to be blesed and incorporated into the tree.
Comment on attachment 325941 [details] [diff] [review]
Ctrl-A patch according to Pete Riley's posting

In order to get a patch into the tree, it needs to be reviewed. See http://wiki.mozilla.org/Calendar:Module_Ownership for a list of reviewers. In this case, best would be berend or philipp.
Attachment #325941 - Flags: review?(Berend.Cornelius)
Comment on attachment 325941 [details] [diff] [review]
Ctrl-A patch according to Pete Riley's posting

The location of your patch within calendar-common-sets.js is not quite where it should be. Basically the problem is that the commands 
cmd_selectAll, cmd_cut, cmd_copy, cmd_pase, cmd_print are so far only implemented for the calendar view. But as we meanwhile have a todaypane in all three modes the commands should execute upon the focusedElement. This is already implemented for the delete button but not yet other mentioned commands (in fact your patch did not consider the delete-Command, that's why I had to modify it)
For the commannds cmd_undo and cmd_redo I think a prior discussion is necessary how we want to handle this: Do we want to keep an undo redo queue separate from Thunderbird's queue in mail mode and in the other modes? I will set up two follow-up-bugs for this. For the time being this patch is Ok and I will check in  the modified version.
Attachment #325941 - Flags: review?(Berend.Cornelius) → review+
Attached patch patch v. #2Splinter Review
I submitted Bug 441308  as a follow-up to this issue and checked in the attached patch.
->fixed
I set up Bug 441316 – Undo/Redo does not work in Mail-mode for calendar-actions. I have not yet made any proposals on how realize and implement this, but maybe somebody else wants to go ahead.
Thanks for the patch, Berend, but it doesn't seem to fix the second part of the regression -- deletion is broken in Thunderbird's context menu. As I mentioned in comment 5, something is wrong with this code:

>         if (focusedRichListbox && focusedRichListbox.id == "agenda-listbox") {
>             agendaListbox.deleteSelectedItem(false);
>         } else if (focusedElement.className == "calendar-task-tree") {
>             deleteToDoCommand(null, false);
>         } ELSE IF (THIS.DEFAULTCONTROLLER && !THIS.ISCALENDARINFOREGROUND()) {
>             THIS.DEFAULTCONTROLLER.DOCOMMAND(ACOMMAND);

It seems that either
--> focusedRichListbox && focusedRichListbox.id == "agenda-listbox"
or
--> focusedElement.className == "calendar-task-tree"

is always true when people right click in Thunderbird's preview pane, so the deletion event is never passed to the default controller.
Guys, I now said twice at least that the patch only fixes Ctrl-A, please do read other people's comments more thoroughly.
Attached patch patch v. #3Splinter Review
in reply to comment #16:
Somehow I did not check the "previewpane" error. I am sorry. This is what caused that error: By the moment  the user right-clicks on the previewpane no pane has the focus and that's what "my" code is not prepared for. The patch attached should fix this problem.
In the future the code around this patch (where the focused element is retrieved) should be more generalized so that we can extract the "focus logic" to implement the "cut,copy,paste, print" and also the remaining "undo-redo" handling in the same way.
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED
Attachment #326446 - Flags: review?(philipp)
Attachment #326446 - Flags: review?(philipp) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I patched manually and tried Ctrl-Z as well as context menu -> delete. Both works, but I have not tried before having patched. I guess the fix works, though. But strange enough, Ctrl-Y (redo) does not work. Doing redo via menu works, though. Maybe another add-on is interfering, but would somebody double-check, please? I do not know if this effect is connected to the same problem, but if so, maybe there is something else which needs to be fixed.
Verified fixed in Lightning 0.9pre 2008-06-24 18 / Thunderbird 2.0.0.14 / WinXP.

Thank you!  :)

(Alexander, I don't think that Ctrl-Y has ever worked for me in Thunderbird, so it's probably not related to this bug.)
> (Alexander, I don't think that Ctrl-Y has ever worked for me in
> Thunderbird, so it's probably not related to this bug.)

Bug 365810, ought to be fixed in Thunderbird 3.
Attachment #326314 - Attachment is obsolete: true
Attachment #326314 - Attachment is obsolete: false
VERIFIED per comment #21
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.