Closed Bug 207485 Opened 17 years ago Closed 11 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b1

People

(Reporter: sven.burmeister, Assigned: bugzilla)

References

Details

Attachments

(1 file, 4 obsolete files)

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:
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Browser → Seamonkey
Assignee: sspitzer → mail
also in bug 234665 for Thunderbird, old also ..
Assignee: mail → nobody
Depends on: 234665
QA Contact: esther → message-display
Hardware: PC → All
Target Milestone: --- → seamonkey2.0a2
Depends on: 455502
Attached patch Port of the TB fix v1 (obsolete) — Splinter Review
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.
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
Attachment #358638 - Flags: superreview?(neil)
Attachment #358638 - Flags: review?(mnyromyr)
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 on attachment 358638 [details] [diff] [review]
Port of the TB fix v1

Cancelling sr due to bitrot.
Attachment #358638 - Flags: superreview?(neil)
Attached patch Unbitrotted patch v2 (obsolete) — Splinter Review
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.
Attachment #358638 - Attachment is obsolete: true
Attachment #375991 - Flags: superreview?(neil)
Attachment #375991 - Flags: review?(mnyromyr)
Attachment #358638 - Flags: review?(mnyromyr)
Blocks: 491676
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).
Attachment #375991 - Flags: superreview?(neil) → superreview+
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. ;-)
Attachment #375991 - Flags: review?(mnyromyr) → review-
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #375991 - Attachment is obsolete: true
Attachment #378123 - Flags: superreview?(neil)
Attachment #378123 - Flags: review?(mnyromyr)
(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 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.
(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.
Attached patch patch v4 (obsolete) — Splinter Review
Essentially v3 plus a fix similar to the fix in bug 491676.
Attachment #378123 - Attachment is obsolete: true
Attachment #378579 - Flags: superreview?(neil)
Attachment #378579 - Flags: review?(mnyromyr)
Attachment #378123 - Flags: superreview?(neil)
Attachment #378123 - Flags: review?(mnyromyr)
(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?
(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.
Attachment #378579 - Flags: review?(mnyromyr) → review+
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 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;
Attachment #378579 - Flags: superreview?(neil) → superreview+
Attached patch Patch v5Splinter Review
Fixing the review nits from #c16 and #c17, taking over r+/sr+
Attachment #378579 - Attachment is obsolete: true
Attachment #381258 - Flags: superreview+
Attachment #381258 - Flags: review+
Marking the bug as fixed, the CSS issue can now be fixed for delete and junk button together in bug 491676.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: seamonkey2.0a2 → seamonkey2.0b1
No longer blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.