Closed Bug 274628 Opened 20 years ago Closed 15 years ago

Preference to close message window on delete instead of loading next message.

Categories

(Thunderbird :: Preferences, enhancement)

Product:
Thunderbird
Component:
Preferences
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 3.0b3

People

(Reporter: pkopacz, Assigned: sheppy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 9 obsolete files)

Unbitrotted patch
6.15 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review 
When deleting a message from a message window, the next message is automatically
loaded.  A preference setting where the message window is closed upon delete
instead would be useful.
Here's another vote for this feature.  I would find this much more intuitive
than the current behavior.  I'm constantly closing windows after deletion, very
rarely do I want to read the message that is then displayed.
The majority of one's messages these days are suspect or trivial, and an option
to not have them open automatically is a priority.  Why is it not confirmed? 
The problem is real.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 302241 has been marked as a duplicate of this bug. ***
The buttons! extension has a close-on-delete option, but it isn't nicely
integrated as it could be (for instance, undo delete undoes normal
keep-window-open-and-advance delete, but not delete-and-close-window). This
seems like such a dirt-simple option to build into thunderbird, instead of
requiring an extension. 
Buttons! does work, but this should be a built-in feature for Thunderbird.
Especially for newbs like me to the whole Mozilla scene, it took two heads about
10 minutes to figure out how to make Buttons! do the close on delete.
This bug is annoying whenever you are sifting spam, which is often.  You know
the next message is rubbish that you do not want opened, but Thunderbird opens
it anyway.  It should not be too hard to fix.  At the last message it already
knows how to close the message window and return to the folder, so the code is
there.  We need a switch in preferences so the user can choose.
As of 1.5, the buttons! extension no longer works, so adding this feature to the client is even more important. 
I spent hours trying to figure out how to do this, only eventually to give up in despair. I can't think of another email client I've used that doesn't have this functionality. I find it annoying enough to consider not using Thunderbird.
There is an extention called "buttons!" that gives you a delete button that does this, but this quite a mess - it should be implemented anyhow.
Re: Comment #9,

Buttons! only works with 1.0x, so it's not really an acceptable solution.
I'm using buttons! with Thunderbird 1.5, I originally installed it on 1.0x and then upgraded without a hitch.
Attached patch a one-liner patch (obsolete) — — Splinter Review 
Here's a one-liner that adds this feature. It uses a disused Netscape preference, mail.close_message_window.on_delete, which controlled this behavior in Communicator at some point in this past. This preference is still in Thunderbird's default prefs (in mozilla/mailnews/mailnews.js), and defaults to true. The patch was created against the 1.5 tree, but is straightforwardly adaptable to newer trees as well.
Comment on attachment 216478 [details] [diff] [review]
a one-liner patch

I think this is reasonable, but I'd rename the pref to replace the window.on_delete with window_on_delete - there's no reason for there to be a '.' there.
I have verified that this patch works great under TB 1.5 & Windows 2k. 
Given that this will probably take forever to make it into a release version, I have made a patched version of messenger.jar at http://bork.hampshire.edu/~alan/code/ which can be used with 1.5 (any OS). 
(In reply to comment #13)
> (From update of attachment 216478 [details] [diff] [review] [edit])
> I think this is reasonable, but I'd rename the pref to replace the
> window.on_delete with window_on_delete - there's no reason for there to be a
> '.' there.
> 

I didn't pick the name (as I said, it's an old Netscape preference), but the dot is there because there is another preference mail.close_message_window.on_file, so the naming scheme is <component>.<action>.<trigger-event>, which is pretty standard.
Attached patch a much more functional patch (obsolete) — — Splinter Review 
Here's a new version of my patch, though now it's much more than a one-liner, I think it's way more useful. In addition to closing the standalone window on delete, it tells the thread pane to select the message that the standalone window would show if this preference was turned off. This parallels the behavior of filing a message from the standalone window, as well as mimicing the behavior of other popular mail programs, such as Apple's Mail.app.
Attachment #216478 - Attachment is obsolete: true
>so the naming scheme is
> <component>.<action>.<trigger-event>, which is pretty standard.

ah, ok, I didn't know that.
QA Contact: preferences
How do we get this into a stable release? It seems like a no-brainier to add this preference, at the very least, as an advanced option. 
You get it into a stable release by first realizing that hidden prefs are actually more risky and expensive than visible prefs, because you then have to support behavior that nobody expects or sees, and thus everyone is more likely to break it, and then if you still think it shouldn't have visible UI, either take it over or help Jim learn to check at least that one file out from CVS, to create a |cvs diff -up8| patch, which will among other things automatically be in the right direction (the current patch says to remove the lines which it actually wants to add), and then ask one of the module owners or peers for review, by setting the review flag for the patch to "?" and pasting in his email. 
This version has been updated to apply cleanly to the current development trunk, and also applies fine to Thunderbird 2.0.0. (I wish someone had pointed out long ago that I'd generated a backwards patch!).
(In reply to comment #20)

I'm agnostic on the question of whether there should be UI for this preference. It seems to me that it's not worth the effort because I believe that that this feature has a very small (if vocal) audience.

As for pushing for a review, someone else will have to take that job up. I actually don't think my patch is ultimately the right solution because it's essentially just copying equivalent functionality from msgMail3PaneWindow.js. The messageWindow.js and msgMail3PaneWindow.js naturally have a lot of what should be identical functionality, but the implementations are (imperfectly) duplicated when they should be shared. I don't have time to take this task on at the moment. My patch is, however, in keeping with the rest of the message window code, so if someone wants to push for review, go for it.

I also think perhaps the preference name should be changed to  mail.close_message_window.on_delete_or_file or similar since the patch will close on either event right now. This is because there is only one event that covers both of these cases, DeleteOrMoveMsgCompleted, so there is currently no way to separate those two cases. I don't see a reason anyone would want to separate these two cases with regard to automatically closing the message window - the preference name is just a bit imprecise.

The old Netscape mail app had this feature, and was listed in the preference UI.  I don't know what the argument was for removing it in Thunderbird. Most of the other mail readers that I've used have this option, so I'm not so sure it's such a niche option. There is a Thunderbird extension which is reasonably popular (Buttons) which adds this option, which is perhaps the reason why we don't hear about the need for it on bugzilla so much. The downside to the extension is that it's not always compatible with the latest release of Thunderbird.
(In reply to comment #24)
> *** Bug 420799 has been marked as a duplicate of this bug. ***
> 

Okay, but what does it take to get this functionality flaw addressed? This has been open for over 2 years now.
Well, comment 20 had three parts, of which only one was addressed. Luckily, now we get to stick clarkbw with nasty decisions about which is the one true behavior, and what should or shouldn't have exposed pref UI :)
(In reply to comment #23)
>The old Netscape mail app had this feature, and was listed in the preference
>UI.  I don't know what the argument was for removing it in Thunderbird.
Remember, Thunderbird was rewritten from scratch, so it was simply never added. (But it would be interesting to know what preference name Netscape used.)
It clearly should be an exposed pref. This is a standard feature in most mail readers, and a make or break feature for some people, myself included.

As for the other issues, none of us are close enough to the development team to know what to do next. Can somebody help us out here? 
Well, I for one disagree this is something that should have UI.

Jim Alexander: are you planning on updating the patch? The attached version does not apply to current trunk. The hidden pref should also be added to all-thunderbird.js
Assignee: mscott → nobody
First time poster here, but I honestly someone does make a permanent fix for this, where after deleting a message, the next message does not come up.

Thanks,
Dave C.
Yikes... I just wasted a few hours trying to figure out why this wasn't working. :(  It didn't seem possible that it simply wouldn't be present, as it's such a commonly-available feature... I don't think that I've *ever* encountered another GUI email client which didn't have this behavior, at least as an option.

Hard to believe this has languished for nearly 4 years now.
(In reply to comment #16)
> I didn't pick the name (as I said, it's an old Netscape preference), but the
> dot is there because there is another preference
> mail.close_message_window.on_file, so the naming scheme is
> <component>.<action>.<trigger-event>, which is pretty standard.

No, we don't have such a pref.
You might want to look at the date on that comment before making that claim. The preference was there at the time, but were removed starting with CVS version
<A HREF="http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=mailnews.js&branch=&root=/cvsroot&subdir=mozilla/mailnews&command=DIFF_FRAMESET&rev1=3.297&rev2=3.298">3.298</A>
Is there a reason why the default isn't just to close the window when the message is deleted? Opening another message in the same window seems kind of odd.
Happy 2009! So my bug has been redirected here. And I've read from start to
here and still am unsure what I can do to get the functionality in TB. Do I
apply Jim Alexander's patch that in his May 2007 note? Or should I start
MUTTering instead?
(In reply to comment #36)
> Is there a reason why the default isn't just to close the window when the
> message is deleted? 

Yes, that way you can read through your mails in the standalone window.
Flags: wanted-thunderbird3?
Since the TB developers seem to view this as a feature request rather than a bug, is there some other avenue that should be pursued? I find it hard to believe that, given TB has gone through 2.0 and 3.0 since this request was put in that NOBODY officially with TB has had time to look at what seems to be a simple request. I don't believe this is something that can be accomplished by end users themselves, unless the official TB developers want end users making UI decisions unilaterally.
I now use Outlook 2003 at work and it as the feature of closing the windows after deleting the message, so this bug should be linked to Bug 423488 -  Outlook Express/Outlook parity bugs

I test it into TB3beta1 and the problem is still here, even in tab...
I feel this is more of a UI consistency issue than a feature request. If you search your mailbox and then open a message in a new window from there you get the desired results of the message window closing when you delete the message.
What version of TB are you using? If I open email in its own window and delete that message it gets replaced by the next one in my queue.  I switched to Evolution couple months ago just to try it out and I get the same sort of behavior. Only MSOutlook acts as I expect - maybe Bill got something right here.
I think that is Evolution emulating TB...every previous GUI email reader I can think of that I've tried (Eudora and Netscape amongst them) have had close on delete, most of them as a mandatory default.
For the record, this patch appears to work fine on comm-central, at least after a few minutes of trying it out.
Bryan, any input on this? Do we want UI for such a pref, if we take the patch?
I think we could take this patch.  We've made space in the Advanced preferences for a Reading & Display tab; I think we could use a little of that space for this.

My UI diff would look something like this:

Open Messages In:
....
[ ] Close message window on delete

-- Display -----
It looks like this only works if you opened the message from a regular folder; if you opened the message from a saved search, clicking the delete button does nothing (doesn't even delete the message).
So, anybody up for finalizing the patch (adding the pref UI and checking whatever needs to be done about comment 47)?
Keywords: helpwanted
Hardware: x86 → All
Comment 47, it turns out, affects all the buttons in the toolbar, not just delete, so it's an unrelated issue (see bug 395495).
I'm taking a whack at turning this into a real patch including the UI.  Will be my first time trying to do anything with the UI patch-wise, so this'll be fun!
Here's a patch that includes the UI update, adds the pref to the defaults file, and handles the close on delete behavior.  I'm not super experienced with this process so I'm not setting any flags on this; would appreciate it if someone would try this out and do so if it looks good.
Attachment #217293 - Attachment is obsolete: true
Attachment #264163 - Attachment is obsolete: true
Comment on attachment 367106 [details] [diff] [review]
Close on delete patch for comm-central, with UI updated

You shouldn't use X as an accesskey, as it's not included in any of the words.

Since you're not implementing it for seamonkey, the pref should not be in mailnews.js but in all-thunderbird.js

However, this patch doesn't work when you use the imap mark as deleted model. (The window doesn't close.)
I suspect that the problem with not closing is related to bug 395495.
Attached patch Revised close-on-delete patch (obsolete) — — Splinter Review 
Changed the access key and moved the pref to all-thunderbird.js
Attachment #367106 - Attachment is obsolete: true
(In reply to comment #53)
> I suspect that the problem with not closing is related to bug 395495.

No, looks like it's due to 

  if ((folder.URI == gCurrentFolderUri) && gCurrentMessageIsDeleted)

check earlier in HandleDeleteOrMoveMsgCompleted.
Assignee: nobody → eshepherd
Keywords: helpwanted
Looks like the HandleDeleteOrMoveMsgCompleted method doesn't get called if the message is only marked as deleted and not actually removed.  I'm looking into adding the required code where it belongs to handle closing the window when the message is simply marked for deletion.  I hope to have a new patch soon.
Looking at this further, I'm not convinced the message window should close in "mark as deleted" mode.  The code in nsImapMailFolder.cpp, starting at line 2149, is specifically only sending the notification of a message being moved or deleted if you're not using the mark as deleted model.  The fact that the toolbar icon changes to "Undelete" in this situation also hints that perhaps in the mark as deleted mode, the window shouldn't close.

Opinions?
Another reason to perhaps not close the window on delete in "mark as deleted" mode: deleting the message in this mode doesn't seem to automatically advance to the next message.  Any thoughts?
Sounds like a behavior bug to me.  I'm sure fixing that now will cause a stir with people who have been experiencing it for a while now and expect the alternate behavior.

If we choose a behavior that's based on I think we will have a difficult time explaining to users the different behavior of the window closing depending on the delete mode the IMAP account is using.  Closing it would conflict with the fact, you bring up, that it's never loaded a new message before; but then closing the window seems like what users would end up doing in that situation anyway.
I'd be inclined to say that the patch I've submitted here (or a variation on it if any other issues come up) resolves this particular bug, and that a separate bug should be filed to consider changing the behavior when deleting a message in mark-as-deleted mode.
Who should review this patch?
Attachment #367222 - Flags: review?(mkmelin+mozilla)
Comment on attachment 367222 [details] [diff] [review]
Revised close-on-delete patch

>+      if (pref.getBoolPref("mail.close_message_window.on_delete")){
>+        // tell the main window to select the next message since we
>+        // won't be viewing it automatically in the standalone 
window

Please capitalize and end the sentence with a dot.

>+        var mainWindow = window.opener;
>+        var mainDoc = mainWindow.document;

No need for the temporary mainWindow varibable.

>+        var treeView = mainDoc.getElementById("threadTree").view;
>+        var treeSelection;
>+        if (treeView)
>+          treeSelection = treeView.selection;
>+
>+        gDBView.suppressCommandUpdating = true;                 
>+
>+        if (gNextMessageViewIndexAfterDelete >= 0)
>+          treeSelection.select(gNextMessageViewIndexAfterDelete);
>+
>+        if (treeView)
>+          treeView.selectionChanged();

The above is a bit odd. Wither we have a treeView or we will have an undefined treeSelection anyway. The whole thing should be inside the if block - if it's needed at all...

Also remove the trailing spaces there.

>+        window.opener.EnsureRowInThreadTreeIsVisible(gNextMessageViewIndexAfterDelete);
>+        gDBView.suppressCommandUpdating = false;
>+        window.close();
>+      } else if (nextMstKey != nsMsgKey_None) {

Please put the "else if..." on a new line.

>+<!ENTITY closeMsgWindowOnDelete.label  "Close message window on delete">
>+<!ENTITY closeMsgWindowOnDelete.accesskey "c">

Accesskeys are actually case sensitive (for performance, both upper and lower "works" though.
Attached patch Address Magnus's comments (obsolete) — — Splinter Review 
New patch to address Magnus's comments in c#62.
Attachment #368990 - Flags: review?(mkmelin+mozilla)
Attachment #367222 - Attachment is obsolete: true
Attachment #367222 - Flags: review?(mkmelin+mozilla)
Depends on: 486954
Attached patch updated fix (obsolete) — — Splinter Review 
Unbitrotted the fix. I've played with it some - don't think the if (treeView) was necessary + when the pref to close is set I don't think it should advance with imap-mark-as-deleted.

This part seems to work well now with this patch. However, from the search results the behavior is quite weird: the next selected msg gets all wrong it seems... (only when the close-pref is set). Can you investigate that?
Attachment #368990 - Attachment is obsolete: true
Attachment #368990 - Flags: review?(mkmelin+mozilla)
There are many things that don't work right with search results.  Is there something specifically different with the threadTree when it's showing search results?  I can't figure that part out, but there must be something...
OK, HandleDeleteOrMoveMsgCompleted is never getting called when you hit the delete button on a message opened from search results... investigation continues.
Looks like even though the folderListener is being set up by calling AddFolderListener, the listener never gets called.  Not sure why not.  Anyone have a guess?
OK, I've got this figured out now.  Testing a patch that fixes the delete, mark, and flag buttons to all work.  Seems to be working so far; I expect to attach a patch sometime Saturday.
OK, my patch so far is only working for the delete button; investigating why it's not helping for the others.
Attached patch Revised patch (obsolete) — — Splinter Review 
I believe this fixes the problem, both for the delete button and the mark button, and handles exceptions a little better.  I'd like someone with a better internal knowledge of Thunderbird to give it a look and see what they think of the patch; I wonder if it might be done better, but am unsure how.
Attachment #373487 - Attachment is obsolete: true
What exception are you catching?
That's a good question; I based my patch on other code, which doesn't actually check what exception is thrown that I can see.
Should I change the patch to only catch the invalid pointer exception that's responsible for the problem here in particular? Not sure whether that matters or not in this context, but certainly I can do that.
Attached patch More intelligent patch (obsolete) — — Splinter Review 
This version of the patch now actually checks the exception being thrown and only handles NS_ERROR_INVALID_POINTER, which is the one that means the message is standalone in this context.  It does this for the delete, flagged, and mark buttons.
Attachment #374789 - Attachment is obsolete: true
Attachment #374951 - Flags: review?(mkmelin+mozilla)
(In reply to comment #66)
> OK, HandleDeleteOrMoveMsgCompleted is never getting called when you hit the
> delete button on a message opened from search results... 

I don't know what you were seeing but HandleDeleteOrMoveMsgCompleted gets called alright over here. I'm also unable to get those exceptions you catch... 

Have to investigate more tomorrow.
Blocks: 475126
Conditions have changed in recent builds since other bugs were fixed; I'm working on tweaking this patch; much of it may no longer be needed. Can't get a build to go tonight, looks like I checked out broken code. Going to try again tomorrow.
Comment on attachment 374951 [details] [diff] [review]
More intelligent patch

Cancelling this in anticipation of the new patch.
Attachment #374951 - Attachment is obsolete: true
Attachment #374951 - Flags: review?(mkmelin+mozilla)
This patch takes out a bunch of stuff that's not needed now that other stuff elsewhere has been fixed (the previous patch also fixed another problem, which no longer needs to be done).
Attachment #376838 - Flags: review?(mkmelin+mozilla)
Attachment #376838 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 376838 [details] [diff] [review]
Unbitrotted patch

Looks good, thx for hanging in there! r=mkmelin
I changed a couple of style nits and checked this in.

changeset:   2664:1cdf3c60c94c
http://hg.mozilla.org/comm-central/rev/1cdf3c60c94c

->FIXED
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
Blocks: TB2SM
This new preference no longer works starting with the Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090613 Lightning/1.0pre Shredder/3.0b3pre build.
(In reply to comment #81)
> This new preference no longer works starting with the Mozilla/5.0 (Windows; U;
> Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090613 Lightning/1.0pre
> Shredder/3.0b3pre build.

Please file a new bug and mark it as blocking bug 497199. It is most likely bug 474701 that has broken this, and therefore we should deal wqith it in a new bug.
Done.  File as bug 498141.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: