Closed Bug 80574 Opened 19 years ago Closed 10 years ago

Standalone: [pref] Delete should close the standalone window

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b1

People

(Reporter: hwaara, Assigned: InvisibleSmiley)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Quoting from the spec (URL is attached):

``The "Delete" toolbar button deletes (moves to Trash) the message and closes
the standalone window.''

Currently, the next message is viewed in the standalone window if Delete is
performed.
keywords...
what does 4xp do? perhaps we should change the spec...
I think the spec hasn't been updated.  Before the recent performance
improvements we didn't have the ability to do navigation in the standalone
window so we made delete and move close the window.  Now that's been
implemented.  So, I'm going to reassign this to jglick to update the spec.
Assignee: sspitzer → jglick
No, why should we just update the spec to reflect the current behaviour?

The reason I filed this bug in the first place was that this behaviour is wrong
and illogical. 

Jglick, comments?
4.x does not close the window but appears to display the next message message. I
like this functionality because I know there are many people who view all their
messages in a stand alone window. After deleting a message it's convenient to
see the next message. If they want to close the window then they select
File|Close or the "x" box.
This is one I actually think deserves a preference. Some users like to see the 
next message when they delete a message and other users like to see the message 
close when they delete it. (There might be an older bug with a similar 
discussion...) Displaying the next message would be the default.

What do folks think of having this as a pref?
I wouldn't mind a pref (default==4xp). but if we don't create a pref I want the 
4xp (which would mean this=>WONTFIX).
Severity: normal → enhancement
Summary: Standalone: Delete should close the standalone window → Standalone: [pref] Delete should close the standalone window
How would the pref be worded?
Hardware: PC → All
QA Contact: esther → sheelar
How about in Preferences/Mail and Newsgroups/General settings...

[*] Read next message after delete

I would like to see this as a pref.  I prefer messages to close so that I don't
accidentaly open a message I didn't really want to open (spam).  But I can see
where people with large volumes of mail would want to proceed to the next
message after deleting or replying to a message.
Just another thought, I was visiting an office and someone pointed out that 
they like to read their mail from bottom to top, so the pref should allow for 
that.

After deleting a message go to [mail folder|next message|previous message|v]

But that can be left as a future enhancement.
See also bug 90539, delete button on keyboard does not work when navigating 
from mail window.
If you are on the last message (there is no "next" message) and you press the
delete button, the message is deleted, but the window remains open and continues
to display the now-deleted message. This behavior is pretty confusing.
Blocks: 104166
Given the amount of spam that I and no doubt many others must deal with today I
very much miss the 4.x behavior that allows me to select and delete messages in
the preview pane without advancing to the next message.

The problem with mozilla 0.9.7 is that MailNews always advances to the next
unread (or last) message when one or more messages are deleted. This usually
results in having to view some overcomplicated html spam message that then
occupies mozilla while it loads up for a few seconds before I can do anything else.

For example:

In 3 pane mode.

1) Select messages to de deleted including the last (or most current message) in
the preview pane.
2) Hit delete key - messages are marked for deletion
3) Mailnews then advances and displays the last message in the preview pane even
though it has been marked for deletion
4) Compact folder
5) I still see the last deleted message in the preview pane (this is a bug)
which is usually spam anyway.
6) Manually select another message in list to clear preview pane of unwanted
spam message that has already been deleted.

reproducable 100%

Solaris 8
Mozilla 0.9.7

I much prefer the 4.x behavior which does not advance to *any* message after the
delete key is pressed. When the currently displayed message is actually removed
then n4.x either advances to the next message after the deleted message or if
the last message was deleted then it displays whatever the last message is (read
or unread) left in the list.

Call this a feature if you want but in this day and age of constant spam
anything that can help deal with it would be greatly appreciated.
Adding link to proposed pref panel. Reassigning back to default owner.
Assignee: jglick → sspitzer
QA Contact: sheelar → esther
*** Bug 153837 has been marked as a duplicate of this bug. ***
*** Bug 194112 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
Assignee: sspitzer → mail
*** Bug 239306 has been marked as a duplicate of this bug. ***
Assignee: mail → nobody
QA Contact: esther → message-display
Attached patch proposed patch (obsolete) — Splinter Review
Port from TB bug 274628. I had to remove the separators in the pref pane to make it fit (else at least on Windows the bottom hbox is cut off).
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #380048 - Flags: superreview?(neil)
Attachment #380048 - Flags: review?(mnyromyr)
Comment on attachment 380048 [details] [diff] [review]
proposed patch

>+        // Tell the main window to select the next message since we
>+        // won't be viewing it automatically in the standalone window.
How do you know that the main window was showing the deleted message? Instead of this change I think it might be worth a separate patch/bug for what to do when deleting a message that's being shown in another window.

>-      <separator class="thin"/>
Maybe we should put this pref in another pane that has a bit more room?
Attached patch alternate patch (obsolete) — Splinter Review
(In reply to comment #19)
> (From update of attachment 380048 [details] [diff] [review])
> >+        // Tell the main window to select the next message since we
> >+        // won't be viewing it automatically in the standalone window.
> How do you know that the main window was showing the deleted message? Instead
> of this change I think it might be worth a separate patch/bug for what to do
> when deleting a message that's being shown in another window.

Agreed, removed from the alternate patch.

> >-      <separator class="thin"/>
> Maybe we should put this pref in another pane that has a bit more room?

The only other place would be the main Mail & Newsgroups category but it fits so much better in Message Display, also because there's already "When opening messages..." there which refers to standalone message windows. Since I placed the new relating pref directly below it there's no real need for a separator between those, only possibly below the new checkbox line. But IMO that's not really that much of an issue either because a list of checkboxes pleases the eye.

In my alternate patch I've also changed the Font pref under "Plain text messages" from an ugly 2-items dropdown to a horizontal radiogroup and removed the separator below it, allowing me to bring back the separator above "Automatically mark messages as read". I think the pref pane looks better that way overall and hope you agree.
Attachment #380048 - Attachment is obsolete: true
Attachment #380048 - Flags: superreview?(neil)
Attachment #380048 - Flags: review?(mnyromyr)
Attachment #380249 - Flags: superreview?(neil)
Attachment #380249 - Flags: review?(mnyromyr)
Comment on attachment 380249 [details] [diff] [review]
alternate patch

>+++ b/suite/locales/en-US/chrome/mailnews/pref/pref-viewing_messages.dtd
>+<!ENTITY closeMsgWindowOnDelete.label     "Close message window on delete">

I'd prefer something like "Close message window when deleting the message".

>+<!ENTITY closeMsgWindowOnDelete.accesskey "l">
>\ No newline at end of file

And while you're at it, please fix this as well. ;-)

>+++ b/suite/mailnews/messageWindow.js
> function HandleDeleteOrMoveMsgCompleted(folder)
> {
>+  var folderResource = folder.QueryInterface(Components.interfaces.nsIRDFResource);
>+  if (!folderResource)
>+    return;

if (!(folderResource instanceof Components.interfaces.nsIRDFResource))
  return;
 
>   if ((folderResource.Value == gCurrentFolderUri))

Kill the double braces while you're at it.

>+++ b/suite/mailnews/prefs/pref-viewing_messages.xul
>+        <radiogroup id="mailFixedWidthMessages"
>+                    orient="horizontal" class="indent"
>+                    preference="mail.fixed_width_messages">
>+          <radio value="true" label="&fontFixedWidth.label;"
>+                 accesskey="&fontFixedWidth.accesskey;"/>
>+          <radio value="false" label="&fontVarWidth.label;"
>+                 accesskey="&fontVarWidth.accesskey;"/>

Either put all attributes onto one line or use one line per attribute and align them vertically; generally order 'id' before 'label' before 'accesskey' before stuff before command handlers.

r=me with that.
Attachment #380249 - Flags: review?(mnyromyr) → review+
Comment on attachment 380249 [details] [diff] [review]
alternate patch

>+// close standalone message window when deleting the displayed message
>+pref("mail.close_message_window.on_delete", false);
[I would have thought this would have been more appropriate near the reuse message window pref, see below.]

>\ No newline at end of file
[As Mnyromyr mentioned.]

>-    var folderResource = folder.QueryInterface(Components.interfaces.nsIRDFResource);
>-    if (!folderResource)
>-        return;
>+  var folderResource = folder.QueryInterface(Components.interfaces.nsIRDFResource);
>+  if (!folderResource)
>+    return;
> 
>   if ((folderResource.Value == gCurrentFolderUri))
If you're going to fiddle with this you might as well write (I think)
if (folder.URI == gCurrentFolderUri)

>                     preference="mailnews.reuse_message_window">
>           <radio value="false" label="&newWindowRadio.label;"
>                  accesskey="&newWindowRadio.accesskey;" id="new"/>
>           <radio value="true" label="&existingWindowRadio.label;"
>                  accesskey="&existingWindowRadio.accesskey;" id="existing"/>
>         </radiogroup>
>       </hbox>
> 
>-      <separator class="thin"/>
>-
>+      <checkbox id="closeMsgWindowOnDelete"
>+                label="&closeMsgWindowOnDelete.label;"
>+                accesskey="&closeMsgWindowOnDelete.accesskey;"
>+                preference="mail.close_message_window.on_delete"/>
I think this checkbox looks as if it has some sort of relationship to the previous preference (after all if you reuse the message window you are less likely to want it to close on delete) and it therefore might be worth adding class="indent" to it (and then also move the pref above).

>+        <radiogroup id="mailFixedWidthMessages"
>+                    orient="horizontal" class="indent"
On the other hand this class="indent" is wrong and should be removed. sr=me with that fixed.
Attachment #380249 - Flags: superreview?(neil) → superreview+
Attached patch patch v2, sr=neil (obsolete) — Splinter Review
(In reply to comment #21)
> >+++ b/suite/mailnews/messageWindow.js
> > function HandleDeleteOrMoveMsgCompleted(folder)
> > {
> >+  var folderResource = folder.QueryInterface(Components.interfaces.nsIRDFResource);
> >+  if (!folderResource)
> >+    return;
> 
> if (!(folderResource instanceof Components.interfaces.nsIRDFResource))
>   return;

With that change, folderResource would be undefined. ;-) I followed Neil's suggestion here but of course using 'folder' (in two places) would work, too.

> >+++ b/suite/mailnews/prefs/pref-viewing_messages.xul
> >+        <radiogroup id="mailFixedWidthMessages"
> >+                    orient="horizontal" class="indent"
> >+                    preference="mail.fixed_width_messages">
> >+          <radio value="true" label="&fontFixedWidth.label;"
> >+                 accesskey="&fontFixedWidth.accesskey;"/>
> >+          <radio value="false" label="&fontVarWidth.label;"
> >+                 accesskey="&fontVarWidth.accesskey;"/>
> 
> Either put all attributes onto one line or use one line per attribute and align
> them vertically; generally order 'id' before 'label' before 'accesskey' before
> stuff before command handlers.

OK but I kept 'value' in front (cf. reuseMessageWindow). Or do you count that as "stuff"?

(In reply to comment #22)
> >                     preference="mailnews.reuse_message_window">
> >           <radio value="false" label="&newWindowRadio.label;"
> >                  accesskey="&newWindowRadio.accesskey;" id="new"/>
> >           <radio value="true" label="&existingWindowRadio.label;"
> >                  accesskey="&existingWindowRadio.accesskey;" id="existing"/>
> >         </radiogroup>
> >       </hbox>
> > 
> >-      <separator class="thin"/>
> >-
> >+      <checkbox id="closeMsgWindowOnDelete"
> >+                label="&closeMsgWindowOnDelete.label;"
> >+                accesskey="&closeMsgWindowOnDelete.accesskey;"
> >+                preference="mail.close_message_window.on_delete"/>
> I think this checkbox looks as if it has some sort of relationship to the
> previous preference (after all if you reuse the message window you are less
> likely to want it to close on delete)

I was also thinking that.

> and it therefore might be worth adding
> class="indent" to it (and then also move the pref above).

The radio button and checkbox aren't perfectly aligned now but I guess we have to live with that.
Attachment #380249 - Attachment is obsolete: true
Attachment #381719 - Flags: superreview+
Attachment #381719 - Flags: review?(mnyromyr)
Comment on attachment 381719 [details] [diff] [review]
patch v2, sr=neil

(In reply to comment #23)
> I followed Neil's suggestion here 

Yeah, that's even better.

> OK but I kept 'value' in front (cf. reuseMessageWindow). 
> Or do you count that as "stuff"?

I do, yes.

> > and it therefore might be worth adding
> > class="indent" to it (and then also move the pref above).
> 
> The radio button and checkbox aren't perfectly aligned now but I guess we have
> to live with that.

No, we don't. ;-)
You need to put the class onto the first radio, not onto the outer hbox.
Attachment #381719 - Flags: review?(mnyromyr) → review-
(In reply to comment #23)
> (In reply to comment #22)
> > >         </radiogroup>
> > >       </hbox>
> > > 
> > >+      <checkbox id="closeMsgWindowOnDelete"
> > and it therefore might be worth adding
> > class="indent" to it (and then also move the pref above).
> The radio button and checkbox aren't perfectly aligned now but I guess we have
> to live with that.
About 9 years ago the box was added to indent the radiogroup, when in fact it wasn't necessary because class="indent" works fine on a radiogroup, although nobody noticed (including me, when I reviewed several subsequent patches).

However class="indent" is not good on a checkbox, because the checkbox has its own default margin which conflicts, and I should have realised that.

You could put the checkbox inside its own box. Or, you could change the radiogroup's box into a vbox and put the checkbox inside that. (You would want to remove the align if you did that though!)
Attachment #381719 - Attachment is obsolete: true
Attachment #381785 - Flags: superreview?(neil)
Attachment #381785 - Flags: review?(mnyromyr)
Attachment #381785 - Flags: superreview?(neil) → superreview+
Comment on attachment 381785 [details] [diff] [review]
patch v3 [Checkin: Comment 28]

[I wouldn't have gone to the trouble of rewrapping the existing radios]
Attachment #381785 - Flags: review?(mnyromyr) → review+
Comment on attachment 381785 [details] [diff] [review]
patch v3 [Checkin: Comment 28]

Landed as <http://hg.mozilla.org/comm-central/rev/eaaa196d5af0> with the radios' value attribute made last, so that id is first.
Attachment #381785 - Attachment description: patch v3 → patch v3 [Checkin: Comment 28]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b1
You need to log in before you can comment on or make changes to this bug.