207485 - Message marked as delete does not change button-text to undelete

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[S. Burmeister]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.4b) Gecko/20030526
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.4b) Gecko/20030526

When one deletes a message in an IMAP folder it is marked as deleted, however
the button DELETE does not change to UNDELETE, when that message is selected, as
for example JUNK changes to NOT JUNK for messages marked as junk.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Stefan Borggraefe]

Confirming.

We do something similar with the "Junk"-button. Furthermore the context menu
item for the selected message in this mode also change from "Delete" to
"Undelete" in the described case. So we should change this button's text, too.

Comment 2

[ovidiu]

also in bug 234665 for Thunderbird, old also ..

Comment 3

[Bruno 'Aqualon' Escherl]

Attachment: Port of the TB fix v1

I've ported the thunderbird patch from bug 234665 including the changes from bug 455502.

The changes from bug 473143 weren't necessary, cause I gave the deck the same id as the delete button had before. Don't know if that's a good thing to do or if we should move the deck into a toolbaritem like thunderbird.

Comment 4

[[e-mail]]

Comment on attachment 358638 [details] [diff] [review]
Port of the TB fix v1

>+  if (!deleteButtonDeck)
>+    return;
How does this help?

>+  // Never show "Undelete" in the 3-pane for folders, when delete would
>+  // apply to the selected folder.
>+  if (this.WhichPaneHasFocus &&
>+      WhichPaneHasFocus() == document.getElementById("folderTree") &&
>+      GetNumSelectedMessages() == 0)
>+    deleteButtonDeck.selectedIndex = 0;
>+  else
>+    deleteButtonDeck.selectedIndex = SelectedMessagesAreDeleted() ? 1 : 0;
Since SelectedMessagesAreDeleted() is always false (well, zero) when GetNumSelectedMessages() is zero, you don't actually need to check the pane.

>@@ -2453,6 +2469,8 @@
>     var msgHdr = msgHdrForCurrentMessage();
>     gMessageNotificationBar.setJunkMsg(msgHdr);
> 
>+    goUpdateCommand('button_delete');
How does this help?

>       </deck>
> 
>-      <toolbarbutton class="toolbarbutton-1" id="button-delete" label="&deleteButton.label;" tooltiptext="&deleteButton.tooltip;" observes="button_delete" oncommand="goDoCommand(event.shiftKey ? 'button_shiftDelete' : 'button_delete')"/>
>+      <deck id="button-delete" observes="button_delete">
Don't worry about making this a toolbaritem; that's Ratty's job.

Comment 5

[[e-mail]]

Comment on attachment 358638 [details] [diff] [review]
Port of the TB fix v1

Cancelling sr due to bitrot.

Comment 6

[Bruno 'Aqualon' Escherl]

Attachment: Unbitrotted patch v2

I finally had time to unbitrot that patch.

(In reply to comment #4)
> Since SelectedMessagesAreDeleted() is always false (well, zero) when
> GetNumSelectedMessages() is zero, you don't actually need to check the pane.
GetNumSelectedMessages() returns the number of the selected messages, even if the threadpane has focus. So we need to check this.

The delete-button and the junk-button have still problems with the customizing toolbar feature, I'll open a new bug for it.

Comment 7

[[e-mail]]

Comment on attachment 375991 [details] [diff] [review]
Unbitrotted patch v2

> 			case "cmd_delete":
> 			case "cmd_shiftDelete":
> 			case "button_delete":
>+            // Make sure the button doesn't show "Undelete" for folders.
>+            UpdateDeleteToolbarButton();
> 			case "button_shiftDelete":
> 			if ( command == "cmd_delete" )
> 				goSetMenuValue(command, 'valueFolder');
Nit: shouldn't update the button for cmd_delete or cmd_shiftDelete. Similarly for the other occurrences of this code pattern.

>+  // Never show "Undelete" in the 3-pane for folders, when delete would
>+  // apply to the selected folder.
Nit: would be easier to pass in a flag from the caller to indicate whether the folder pane has focus (i.e. the FolderPaneController passes in a different value [of your choice] to everyone else).

Comment 8

[Karsten Düsterloh]

Comment on attachment 375991 [details] [diff] [review]
Unbitrotted patch v2

Minussing just to see Neil's nits of comment 7 addressed.

Otherwise just a then-irrelevant sidenote:

>+++ b/suite/mailnews/mailWindowOverlay.js
>+function UpdateDeleteToolbarButton()
...
>+  if (this.WhichPaneHasFocus &&
>+      WhichPaneHasFocus() == document.getElementById("folderTree"))

You'd better used GetFolderTree() here, that's what WhichPaneHasFocus() would provide anyway. ;-)

Comment 9

[Bruno 'Aqualon' Escherl]

Attachment: patch v3

The patch fixes the nits from Neil. I made one exception from the first nit:

> case "cmd_delete":
> case "button_delete":
>  MsgDeleteMessage(false);
>  UpdateDeleteToolbarButton(false);
>  break;

We have to update the toolbar button here in both cases. Without, a delete of multiple mails via cmd_delete doesn't update the button since no new mail is loaded as it's the case in the deletion of a single mail. The same thing is needed in the shift-Case.

I also needed to change the MsgDeleteMessage() function to enable undelete with cmd_shiftDelete when IMAP delete model is enabled. Since these are bigger changes, I'm asking for r?/sr? again.

Comment 10

[[e-mail]]

(In reply to comment #9)
> I made one exception from the first nit:
> > case "cmd_delete":
> > case "button_delete":
> >  MsgDeleteMessage(false);
> >  UpdateDeleteToolbarButton(false);
> >  break;
Correct; this is in a doCommand block, and I only meant isCommandEnabled.

Comment 11

[[e-mail]]

Comment on attachment 378123 [details] [diff] [review]
patch v3

>+function UpdateDeleteToolbarButton(aFolderPaneHasFocus)
>+{
>+  var deleteButtonDeck = document.getElementById("delete-deck");
>+
>+  // Never show "Undelete" in the 3-pane for folders, when delete would
>+  // apply to the selected folder.
>+  if (aFolderPaneHasFocus)
>+    deleteButtonDeck.selectedIndex = 0;
>+  else
>+    deleteButtonDeck.selectedIndex = SelectedMessagesAreDeleted() ? 1 : 0;
>+}
This doesn't actually work with customisable toolbars :-( See bug 419676.

>+  // execute deleteNoTrash only if IMAP delete model is not used
>+  if (aReallyDelete && !imapDeleteModelUsed)
>+    gDBView.doCommand(nsMsgViewCommandType.deleteNoTrash);
>+  else
>+    gDBView.doCommand(nsMsgViewCommandType.deleteMsg);
So you want Shift+Undelete to still undelete? Note that the way you've written the code it also affects Shift+Del key, which might not be expected.

Comment 12

[Bruno 'Aqualon' Escherl]

(In reply to comment #11)
> (From update of attachment 378123 [details] [diff] [review])
> This doesn't actually work with customisable toolbars :-( See bug 419676.
Ok, will change it the way Philip did it for the junk button in bug 491676 attachement 378243.

> >+  // execute deleteNoTrash only if IMAP delete model is not used
> >+  if (aReallyDelete && !imapDeleteModelUsed)
> >+    gDBView.doCommand(nsMsgViewCommandType.deleteNoTrash);
> >+  else
> >+    gDBView.doCommand(nsMsgViewCommandType.deleteMsg);
> So you want Shift+Undelete to still undelete? Note that the way you've written
> the code it also affects Shift+Del key, which might not be expected.

Yes, I talked with Karsten about it and he thought that shift should not influence the undelete. Shift+Del is influenced, but only if mail.server.%serverkey%.delete_model is set to 0 and in that case there seems to be no difference between deleteNoTrash and deleteMsg. So if I don't overlook something here, there should be no difference to the previous code.

Comment 13

[Bruno 'Aqualon' Escherl]

Attachment: patch v4

Essentially v3 plus a fix similar to the fix in bug 491676.

Comment 14

[[e-mail]]

(In reply to comment #12)
> Yes, I talked with Karsten about it and he thought that shift should not
> influence the undelete. Shift+Del is influenced, but only if
> mail.server.%serverkey%.delete_model is set to 0 and in that case there seems
> to be no difference between deleteNoTrash and deleteMsg. So if I don't overlook
> something here, there should be no difference to the previous code.
My point was that currently neither Shift+Del nor Shift+Button undeletes the message; this patch changes both, was that intentional?

Comment 15

[Bruno 'Aqualon' Escherl]

(In reply to comment #14)
> My point was that currently neither Shift+Del nor Shift+Button undeletes the
> message; this patch changes both, was that intentional?
I wasn't fully intentional, the code from TB just worked like that ;) But I believe it's a better behavior to undelete an e-mail even if shift is pressed.

Comment 16

[Karsten Düsterloh]

Comment on attachment 378579 [details] [diff] [review]
patch v4

>+++ b/suite/mailnews/mail3PaneWindowCommands.js
>@@ -77,6 +77,9 @@
>+            // Make sure the button doesn't show "Undelete" for folders.
>+            if (command == "button_delete")
>+              UpdateDeleteToolbarButton(true);

Way too much indentation, use 2 spaces per level => 8 spaces.

>+++ b/suite/mailnews/mailWindowOverlay.xul
>+    <toolbaritem id="button-delete">
>+      <deck id="delete-deck" observes="button_delete">
>+        <toolbarbutton id="button-mark-deleted"
>+	               class="toolbarbutton-1"

Don't use tabs, just spaces, 2 per indentation level.

r=me with that fixed on checkin.

Comment 17

[[e-mail]]

Comment on attachment 378579 [details] [diff] [review]
patch v4

>+  var imapDeleteModelUsed = server instanceof Components.interfaces.nsIImapIncomingServer &&
>+                            !server.deleteModel;
const kIMAPDelete = Components.interfaces.nsMsgImapDeleteModels.IMAPDelete;
var imapDeleteModelUsed = server instanceof Components.interfaces.nsIImapIncomingServer &&
                          server.deleteModel == kIMAPDelete;

Comment 18

[Bruno 'Aqualon' Escherl]

Attachment: Patch v5

Fixing the review nits from #c16 and #c17, taking over r+/sr+

Comment 19

[Karsten Düsterloh]

Comment on attachment 381258 [details] [diff] [review]
Patch v5

Pushed as <http://hg.mozilla.org/comm-central/rev/40565392732f>.

Comment 20

[Bruno 'Aqualon' Escherl]

Marking the bug as fixed, the CSS issue can now be fixed for delete and junk button together in bug 491676.