Closed
Bug 207485
Opened 22 years ago
Closed 16 years ago
Message marked as delete does not change button-text to undelete
Categories
(SeaMonkey :: MailNews: Message Display, defect)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0b1
People
(Reporter: sven.burmeister, Assigned: bugzilla)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
bugzilla
:
review+
bugzilla
:
superreview+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
Updated•20 years ago
|
Assignee: sspitzer → mail
also in bug 234665 for Thunderbird, old also ..
Updated•16 years ago
|
Assignee: mail → nobody
Depends on: 234665
QA Contact: esther → message-display
Hardware: PC → All
Target Milestone: --- → seamonkey2.0a2
Assignee | ||
Comment 3•16 years ago
|
||
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 4•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 358638 [details] [diff] [review]
Port of the TB fix v1
Cancelling sr due to bitrot.
Attachment #358638 -
Flags: superreview?(neil)
Assignee | ||
Comment 6•16 years ago
|
||
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)
Comment 7•16 years ago
|
||
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 8•16 years ago
|
||
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-
Assignee | ||
Comment 9•16 years ago
|
||
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)
Comment 10•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
(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.
Assignee | ||
Comment 13•16 years ago
|
||
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)
Comment 14•16 years ago
|
||
(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?
Assignee | ||
Comment 15•16 years ago
|
||
(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.
Updated•16 years ago
|
Attachment #378579 -
Flags: review?(mnyromyr) → review+
Comment 16•16 years ago
|
||
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•16 years ago
|
||
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+
Assignee | ||
Comment 18•16 years ago
|
||
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+
Comment 19•16 years ago
|
||
Comment on attachment 381258 [details] [diff] [review]
Patch v5
Pushed as <http://hg.mozilla.org/comm-central/rev/40565392732f>.
Assignee | ||
Comment 20•16 years ago
|
||
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: 16 years ago
Resolution: --- → FIXED
Target Milestone: seamonkey2.0a2 → seamonkey2.0b1
You need to log in
before you can comment on or make changes to this bug.
Description
•