User-Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; SLCC1; .NET CLR 2.0.50727; Media Center PC 5.0; .NET CLR 1.1.4322; .NET CLR 3.5.21022; .NET CLR 3.5.30729; .NET CLR 3.0.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090613 Lightning/1.0pre Shredder/3.0b3pre The close window on delete preference (created with bug 274628) no longer works. Reproducible: Always Steps to Reproduce: 1. Select the close window on delete preference. 2. Delete a message in a standalone window. 3. The next message appears in the window instead of closing the window. Actual Results: The window does not close. Expected Results: The window should close.

I was asked to put this bug blocks 497199.

Confirming on Mac - Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090613 Lightning/1.0pre Shredder/3.0b3pre

I can confirm that I had the same issue on Mac with 20090613 and 20090614. But it now works for me with the latest nightly: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090621 Shredder/3.0b3pre

(In reply to comment #3) > I can confirm that I had the same issue on Mac with 20090613 and 20090614. But > it now works for me with the latest nightly: > Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) > Gecko/20090621 Shredder/3.0b3pre Strange - still occurs for me on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090621 Lightning/1.0pre Shredder/3.0b3pre - I'm quite sure I did "Select the close window on delete preference." Nomis, would you like to retest?

> Nomis, would you like to retest? OK, I've retested it. For my test in #3 there was only one email in my inbox. After deleting this single email, my standalone window get closed. In build 20090613 and 20090614 the window of the deleted email was still in front after deleting it. Now I moved a bunch of emails into my inbox and deleted this. And in this case I still see the behaviour describet in this bug. So it now works for me with just one mail in my inbox, but I still see the issue with more than one mails. Sorry for the confusion.

I am using IMAP against google apps - thousands of messages When I started this message the following happened. 1. with option - open messages in "a new message window" the message is deleted and the next message opens - not sure if it should work that way - I would have said if you say delete under this option it shoudl close the window only. 2. with option - open messages in "an existing emssage window" the message is marked as deleted (according to a message at the bottom left of the screen), but the window remains open. The window behind that lists all the emails has the message removed. * I was testing a recent bug fix 498473 * I am using shreader 20090630 Then I went to retest my scenarios and under both conditions - only (1) occurs). Tried in vain to make it do it again but couldn't. Perhaps selecting an option reset the option correctly in the options panel.

Sounds like there's not yet agreement on whether this bug is still around; adding qawanted for testing with current builds. If this is still around, please nominate to block Thunderbird 3.

Yes, the bug is still in version Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.1pre) Gecko/20090714 Lightning/1.0pre Shredder/3.0b3pre.

Definitely here yeah. Fallout from attachment 382997 [details] [diff] [review]. We don't do anything with the pref atm - http://mxr.mozilla.org/comm-central/search?string=mail.close_message_window.on_delete I'll take this

The fix, and correct a wrong variable name in MsgDeleteMessageFromMessageWindow

s/have knows/have known

I tested again on 20090714 Delete still loads the next message whether in a tab, or in own window. I reset the option - "close message window on delete" - restarted same thing. I am thinking from reading above you are in the process of working on it. Being in Australia we are sometimes either to early or up-side-down! Will test again tomorrow morning.

Comment on attachment 388535 [details] [diff] [review] proposed fix This needs mozmill unit tests or we should just lose the short-lived feature. Skimming the patch (not doing a deep review without unit tests), a few minor nits/questions: "We might have knows it's coming" should probably be "We might know it's coming" I'm not sure what the comment "Tell the main window to select the next message" means. I read that as meaning that it should change the 3-pane's selection, which the code does not do, and I think would probably need some form of ui-review if it did.

If we're doing it for message windows, we should also logically do it for message tabs. The fix I was thinking about was different though -- in the window's/tab's onSelectedMessagesChanged, if we have no messages selected (which is something that should always happen in the middle of deleting a message) and the pref is true, close the window/tab.

Yeah we should do it for tabs too.

I can confirm that this is still a problem for my current build: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.4pre) Gecko/20090907 Lightning/1.0pre Shredder/3.0b4pre And from a user interface perspective I thing that it is important to realise that the user would expect for the setting "Close Message window on delete" to apply to tabs, i.e. for this setting to affect tabs is the expected behaviour. Although for the sake of clarity I would suggest changing that string to be "Close message window or tab on delete", but this bit is not necessary.

I agree we should do this for tabs too (and fix the labels). Anyway, i had this patch sitting, sid's suggestion may work well too. mozmill test passes, as does the other folder-display tests make SOLO_TEST=folder-display/test-message-window.js mozmill-one

I'm sorry, I'm having various mozmill explosions that are making it hard to verify this. If anyone else is able to get a clean full mozmill run with this (or at least no new failures), then it's cool by me. I hope to have mozmill things worked out later today (friday) or Saturday at the worst.

Here's what I found: trunk: TEST-UNEXPECTED-FAIL | test_put_view_picker_on_toolbar TEST-UNEXPECTED-FAIL | test_save_view_as_folder TEST-UNEXPECTED-FAIL | test_verify_saved_mail_view TEST-UNEXPECTED-FAIL | test_save_quick_search TEST-UNEXPECTED-FAIL | test_verify_saved_search trunk > 498141: TEST-UNEXPECTED-FAIL | test_delete_last_message_in_folder_tab TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_tab TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_window TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_nth_open TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_last_open TEST-UNEXPECTED-FAIL | test_load_folder_with_invalidDB TEST-UNEXPECTED-FAIL | test_view_sort_maintained TEST-UNEXPECTED-FAIL | test_put_view_picker_on_toolbar TEST-UNEXPECTED-FAIL | test_save_view_as_folder TEST-UNEXPECTED-FAIL | test_verify_saved_mail_view Since 465269 fixes some test failures, I thought I would try with it layered on in front of the patch to 498141. The results of that are: trunk > 465269: TEST-UNEXPECTED-FAIL | test_save_quick_search TEST-UNEXPECTED-FAIL | test_verify_saved_search trunk > 465269 > 498141: TEST-UNEXPECTED-FAIL | test_delete_last_message_in_folder_tab TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_tab TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_window TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_nth_open TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_last_open TEST-UNEXPECTED-FAIL | test_folder_toolbar_shows_correct_item TEST-UNEXPECTED-FAIL | test_folder_toolbar_disappears_on_message_tab test Full error log: TEST-UNEXPECTED-FAIL | test_delete_last_message_in_folder_tab EXCEPTION: this.folderDisplay is undefined at: test-window-helpers.js line 723 test-window-helpers.js 723 _process_row_message_arguments([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1027 assert_selected([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1098 assert_selected_and_displayed([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1234 _verify_message_is_displayed_in(15,[object XPCWrappedNative_NoHelper],2) test-deletion-with-multiple-displays.js 161 test_delete_last_message_in_folder_tab() test-deletion-with-multiple-displays.js 290 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_tab EXCEPTION: this.folderDisplay is undefined at: test-window-helpers.js line 723 test-window-helpers.js 723 _process_row_message_arguments([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1027 assert_selected([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1098 assert_selected_and_displayed([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1234 _verify_message_is_displayed_in(15,[object XPCWrappedNative_NoHelper],1) test-deletion-with-multiple-displays.js 161 test_delete_last_message_in_message_tab() test-deletion-with-multiple-displays.js 304 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_window EXCEPTION: Synthesizing key event failed. TypeError: aWindow.KeyEvent is undefined at: events.js line 119 Error("Synthesizing key event failed. \nTypeError: aWindow.KeyEvent is undefined") 0 events.js 119 controller.js 203 press_delete([object Object]) test-folder-display-helpers.js 790 test_delete_last_message_in_message_window() test-deletion-with-multiple-displays.js 321 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_nth_open EXCEPTION: this.folderDisplay is undefined at: test-window-helpers.js line 723 test-window-helpers.js 723 _process_row_message_arguments([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1027 assert_selected([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1098 assert_selected_and_displayed([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1234 _verify_message_is_displayed_in(14,[object XPCWrappedNative_NoHelper]) test-deletion-with-multiple-displays.js 161 test_delete_multiple_messages_including_the_last_one_with_nth_open() test-deletion-with-multiple-displays.js 667 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_last_open EXCEPTION: Messages never finished loading. Timed Out. at: test-folder-display-helpers.js line 826 Error("Messages never finished loading. Timed Out.") 0 wait_for_all_messages_to_load() test-folder-display-helpers.js 826 enter_folder([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 245 be_in_folder([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 263 _open_message_in_all_four_display_mechanisms_helper([object XPCWrappedNative_NoHelper],9) test-deletion-with-multiple-displays.js 102 test_delete_multiple_messages_including_the_last_one_with_last_open() test-deletion-with-multiple-displays.js 683 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_load_folder_with_invalidDB EXCEPTION: Messages never finished loading. Timed Out. at: test-folder-display-helpers.js line 826 Error("Messages never finished loading. Timed Out.") 0 wait_for_all_messages_to_load() test-folder-display-helpers.js 826 enter_folder([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 245 be_in_folder([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 263 test_load_folder_with_invalidDB() test-invalid-db-folder-load.js 73 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_view_sort_maintained EXCEPTION: view sort type not restored from invalid db at: test-invalid-db-folder-load.js line 82 Error("view sort type not restored from invalid db") 0 test_view_sort_maintained() test-invalid-db-folder-load.js 82 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_put_view_picker_on_toolbar EXCEPTION: toolbar is null at: test-mail-views.js line 64 test_put_view_picker_on_toolbar() test-mail-views.js 64 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_save_view_as_folder EXCEPTION: aViewPopup is null at: msgViewPickerOverlay.js line 206 RefreshViewPopup(null) msgViewPickerOverlay.js 206 RefreshAllViewPopups(null) msgViewPickerOverlay.js 190 test_save_view_as_folder() test-mail-views.js 79 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_verify_saved_mail_view EXCEPTION: aViewWrapper.dbView is null at: viewWrapperTestUtils.js line 473 verify_messages_in_view([object Array],[object Object]) viewWrapperTestUtils.js 473 assert_messages_in_view([object Object],[object Object]) test-folder-display-helpers.js 987 test_verify_saved_mail_view() test-mail-views.js 111 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 Magnus, feel free to ping me on irc if you need more info on my setup. Later, Blake.

FWIW, mozmill isn't very happy (before this patc). I get INFO | (runtestlist.py) | Directories Run: 6, Passed: 162, Failed: 6

(In reply to comment #21) > FWIW, mozmill isn't very happy (before this patc). > I get > > INFO | (runtestlist.py) | Directories Run: 6, Passed: 162, Failed: 6 We know mozmill works fine on Windows: http://tinderbox.mozilla.org/ThunderbirdTest/ (and it was working fine on my Mac). Linux could be an issue.

Moving out to rc1 as this isn't going to make b4.

Comment on attachment 399300 [details] [diff] [review] proposed fix, v2 Bug 494892 was about the introduction of the close-message-window-on-delete closing all message windows on delete, not just one. We should make sure this bug includes a test for that case as well.

I have no idea why we can't just use window.close(), but this works.

Comment on attachment 401088 [details] [diff] [review] proposed fix, v3 If I open message 'A' in a window, then delete message 'A' from the 3-pane, the 3-pane closes. This does not seem correct although it is pretty amusing. I think, at best, going through the window watcher-type APIs might change our concept of window from inner window to outer window or something. And if that helps us, it would probably be a bug shielding us from onclose logic or something.

Yeah that was certainly unexpected!

It seems like the most recent change is just a band-aid that compromises the correctness of the fix. The situation where the 3-pane was erroneously closed instead of the mail window is now a situation where we erroneously close no window. I'm still not clear on why window.close() was abandoned.

With the patch, if you delete a message open in a standalone window from the 3pane, that just deletes the message (doesn't also close the standalone window) - that's how it's always been afaik. Abandoned window.close() because it caused all open standalone windows to close. There's a lot of unexpected behavior (code wise) here so sure - i imagine there's some underlying bug - and i'd certainly want it to be prettier. But at least it works.

To me the correct solution seems to be to fix window.close() to not close all standalone windows.

That's probably hard to figure out, and even if we do (and it turns out it's a bug), chances to get it in would be low as it'd likely be a core bug. If someone has a prettier fix fine, but otherwise i think we should just go for what we have how.

Andrew, would you be comfortable delegating this review to sid0? (assuming he has some cycles)

Comment on attachment 402417 [details] [diff] [review] proposed fix, v4 I think sid is in finals for a bit, but I think he'll be out of finals and done partying about being out of finals before I am able to get to this. Another option is if sid has time to provide a fix that does not involve asking about the active windows and covers the cases this fix drops on the floor, that would be even better.

OK so I don't understand. Has this issue been fixed or not? Seems like there's been a bunch of patches over the last few years, but no actual solution... is that correct? This is a very important bug to fix for several reasons. I don't like opening spam and selecting the next message automatically eliminates control over what gets opened. Also, I am sometimes looking through a message list and not necessarily wanting to look at sequential messages, so when I delete or file one, I'd like the window to close so I can select my own next message without having to close the window manually. Functional "next" and "previous" toolbar buttons might be a nice addition for situations when I do want to view sequential emails in the same window without moving or deleting them. I found such toolbar buttons in the "Toolbar Buttons" extension but they don't work.

It's not fixed yet.

Ok thanks for the update. This feature would also lead to another enhancement... if TB doesn't automatically select the next message, then that would eliminate (or at least reduce) the need for not loading images in emails. Since I only open emails that I trust, I don't need that step, and find it to be a constant hassle in reading email to always have to approve the images. Nearly all my email comes with images so this is a significant hassle.

work in progress -- I still need to add tests for all the additional behaviour I've covered, but this seems to work and all tests pass. Magnus: so I had no idea what the mark imap as deleted clause is actually doing, but it breaks several tests. Could you please explain what it checks for, and if it's really necessary as part of this bug? Bryan indicated over IRC that it would be correct to close all the displays for a message if it's deleted from any one of them, or from a standalone window.

> or from a standalone window. or from a 3pane window, that is.

It meant if you have the imap delete model set to just mark as deleted, and you open mails in the standalone window and delete messages -> will move you forward in the list, when you come to the last message in the folder (and delete that msg) the message window should close.

Looks like the "should close msg win on delete of last msg in msg list getting deleted" behavior is also currently broken for other delete models, actually.

(In reply to comment #43) > Looks like the "should close msg win on delete of last msg in msg list getting > deleted" behavior is also currently broken for other delete models, actually. Yeah, we don't do that. We currently only close the window if there are no messages left in the view.

Taking into account comment 43 and comment 44, I'm not attempting to fix anything related to imap mark as deleted here. One thing I'm not clear about with the patch: >- "chrome://messenger/content/messageWindow.xul", "_blank", >+ "chrome://messenger/content/messageWindow.xul", "", Why was this needed?

I don't think it's strictly "needed". But you're not supposed to pass _blank as window name - it should be "" afaikt. The patch seems to work, except for when imap-mark-as-deleted is set. Comment 43 isn't fixed either, for the last msg in a list it's supposed to close no matter what the pref.

I'd imagine you can pull together v4 and v5 to get it all to work though.

(In reply to comment #46) > Comment 43 isn't fixed either, for the last msg in a list it's supposed to > close no matter what the pref. See <http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-deletion-with-multiple-displays.js#276> for what actually happens. If you delete the last message in a list, we're supposed to move to the next-to-last message.

For the 3-pane yes - as there's no other way to go. But for people reading their list of new mails by opening one in the standalone window (and moving forward, close on delete not set) that's not nice at all. You want to know when you're done.

(In reply to comment #49) > For the 3-pane yes - as there's no other way to go. But for people reading > their list of new mails by opening one in the standalone window (and moving > forward, close on delete not set) that's not nice at all. You want to know when > you're done. Yeah, I see. Since this seems unrelated to the close window on delete pref, could this be handled in a separate bug?

Sure, but i think it's very closely entangled with fixing the imap mark-as-deleted case which is quite common and should be fixed in this bug.

Comment on attachment 408687 [details] [diff] [review] [checked in] patch, v5 The behaviour discussed here seems correct to me.

(In reply to comment #49) > For the 3-pane yes - as there's no other way to go. But for people reading > their list of new mails by opening one in the standalone window (and moving > forward, close on delete not set) that's not nice at all. You want to know when > you're done. This sounds like an enhancement request which should be its own bug with ui-review and which should provide a mozmill test, as that type of functionality would be very easy to backslide on. Also, this sounds likely to be pretty hard given how much of the deletion logic is in the hands of the nsMsgDBView. The folder display code is generally not aware of the intent of why things happen involving the view which I think is partially why this bug and its kin are so annoying. (btw, I agree that in the specific use case you cite, deletion causing backsliding is annoying.) (In reply to comment #51) > Sure, but i think it's very closely entangled with fixing the imap > mark-as-deleted case which is quite common and should be fixed in this bug. IMAP mark-as-deleted is definitely not our default and in my (non-driver) opinion should not be accorded the same level of blocking-ness. That sounds like something that would be great to have fixed in a separate bug that does not break our mozmill-tested coverage of our actual defaults.

(In reply to comment #50) > (In reply to comment #49) > > For the 3-pane yes - as there's no other way to go. But for people reading > > their list of new mails by opening one in the standalone window (and moving > > forward, close on delete not set) that's not nice at all. You want to know when > > you're done. > > Yeah, I see. Since this seems unrelated to the close window on delete pref, > could this be handled in a separate bug? I reported this as bug #505604, but it was deemed a duplicate of this bug & closed. Now someone is saying it's not a duplicate & should be its own bug? Then re-open my bug.

Putting on my driver hat, I support asuth's reasoning and proposal in comment 53, particularly since this preference itself is not defaulted to true. Magnus, would you reconsider?

Preventing backslide isn't an enhancement request, it has (tb2) always worked like that until the refactoring broke it. Re marked-as-deleted, just because it's not default doesn't mean we should ignore it. All in all, i'd rather go for patch v4 since it actually fixes all these issues, even if it does it in not the cleanest fashion. (Has tests, and doesn't break any afair.) One should never let technology dictate software behavior. Why would we do something technically cleaner that only fixes some case over something that fixes all the cases but uglier (assuming that it "have to" be done the uglier way to achieve what we want in the end.)

My concern with the v4 patch is that onMessagesRemoved is not a precise notification. It is not a guarantee that something actually happened to the view. This is especially true in the case of search views. As such, taking action without verifying that the notification has an effect can result in bad things happening. This is why window.close() was doing horrible things; adding the window-watcher active window stuff was/is a blatant 'code smell' issue that something is amiss. The v5 patch deals with the hint-ness of onMessagesRemoved by only doing something if there is no selection. Also, the additional tests from v5 should not be lost. They provide a nice coverage of the different scenarios.

I don't know when onMessagesRemoved would be phony, but given that close would only happen if you're in the standalone window i don't think search views is an issue. Certainly, the more tests the better!

Any "DeleteOrMoveMsgCompleted" ItemEvent on a folder can potentially generate an onMessagesRemoved event. Single-folder/cross-folder views only subscribe for the relevant folders, search/synthetic (gloda) views subscribe to all folders. The DeleteOrMoveMsgCompleted event is not exclusive to user-initiated deletions and is not guaranteed to happen before the user changes their focus. It looks like we have a bug where when cloning a view where we fail to noteCuriosity() when cloning the backing view for message windows/tabs. Assuming you tested with search/gloda views, this is probably why you did not notice the pathologically bad situation of windows closing all the time. (I am going to log that bug right now.) I'm not opposed to keying off of user activity (and indeed, I think it would be a step forward), but the code should hang explicitly off the user action and not try to infer the action based on the active window and an asynchronous operation that may or may not be correlated with the desired user action.

(In reply to comment #59) > It looks like we have a bug where when cloning a view where we fail to > noteCuriosity() when cloning the backing view for message windows/tabs. > Assuming you tested with search/gloda views, this is probably why you did not > notice the pathologically bad situation of windows closing all the time. (I am > going to log that bug right now.) Bug 525419

As i said before, at least it gets the job done.

Comment on attachment 402417 [details] [diff] [review] proposed fix, v4 >+ if (this._nextViewIndexAfterDelete == nsMsgViewIndex_None || >+ pref.getBoolPref("mail.close_message_window.on_delete") || >+ (this._nextViewIndexAfterDelete != nsMsgViewIndex_None && >+ this.view.dbView.getKeyAt(this._nextViewIndexAfterDelete) == nsMsgKey_None)) { >+ Please remove this blank line. >+ let windowWatcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"] >+ .getService(Components.interfaces.nsIWindowWatcher); >+ let mailWindow = Components.classes["@mozilla.org/appshell/window-mediator;1"] >+ .getService(Components.interfaces.nsIWindowMediator) >+ .getMostRecentWindow("mail:3pane"); >+ // If we're doing the delete of the mail opened in a standalone window from the >+ // 3pane, don't close the 3pane due to that. >+ if (mailWindow != windowWatcher.activeWindow) >+ windowWatcher.activeWindow.close(); >+ And this one. Please also mark this as XXX, given how gratuitous this hack is. Please move the tests out to a separate file, as done in v5, since we're only going to add tests later. Please also put the (re)set_close_message_on_delete calls at the beginning and end of the function, since that's what we've been doing in similar tests. This also needs a ui-review from clarkbw for only closing the window where the delete command's been pressed, even when multiple standalone windows are open to the same message. I'm seeing test failures in test-deletion-with-multiple-displays.js. $ SOLO_TEST=folder-display/test-deletion-with-multiple-displays.js make mozmill-one TEST-UNEXPECTED-FAIL | test_delete_last_message_in_folder_tab EXCEPTION: Desired selection is: 2 but actual selection is: at: test-folder-display-helpers.js line 1156 Error("Desired selection is: 2 but actual selection is: ") 0 assert_selected([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1156 assert_selected_and_displayed([object Object],[object XPCWrappedNative_NoHelper]) test-folder-disp lay-helpers.js 1284 _verify_message_is_displayed_in(15,[object XPCWrappedNative_NoHelper],2) test-deletion-with-multip le-displays.js 161 test_delete_last_message_in_folder_tab() test-deletion-with-multiple-displays.js 290 frame.js 460 frame.js 512 frame.js 554 frame.js 412 frame.js 565 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_tab EXCEPTION: Desired selection is: 1 but actual selection is: at: test-folder-display-helpers.js line 1156 Error("Desired selection is: 1 but actual selection is: ") 0 assert_selected([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1156 assert_selected_and_displayed([object Object],[object XPCWrappedNative_NoHelper]) test-folder-disp lay-helpers.js 1284 _verify_message_is_displayed_in(15,[object XPCWrappedNative_NoHelper],1) test-deletion-with-multip le-displays.js 161 test_delete_last_message_in_message_tab() test-deletion-with-multiple-displays.js 304 frame.js 460 frame.js 512 frame.js 554 frame.js 412 frame.js 565 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_last_message_in_message_window EXCEPTION: Desired selection is: 0 but actual selection is: 1 at: test-folder-display-helpers.js line 1156 Error("Desired selection is: 0 but actual selection is: 1") 0 assert_selected(0) test-folder-display-helpers.js 1156 assert_selected_and_displayed(0) test-folder-display-helpers.js 1284 _verify_message_is_displayed_in(15,[object XPCWrappedNative_NoHelper],0) test-deletion-with-multip le-displays.js 144 test_delete_last_message_in_message_window() test-deletion-with-multiple-displays.js 324 frame.js 460 frame.js 512 frame.js 554 frame.js 412 frame.js 565 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_nth_open EXCEPTION: Desired selection is: 2 but actual selection is: at: test-folder-display-helpers.js line 1156 Error("Desired selection is: 2 but actual selection is: ") 0 assert_selected([object Object],[object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1156 assert_selected_and_displayed([object Object],[object XPCWrappedNative_NoHelper]) test-folder-disp lay-helpers.js 1284 _verify_message_is_displayed_in(14,[object XPCWrappedNative_NoHelper]) test-deletion-with-multiple -displays.js 161 test_delete_multiple_messages_including_the_last_one_with_nth_open() test-deletion-with-multiple-d isplays.js 667 frame.js 460 frame.js 512 frame.js 554 frame.js 412 frame.js 565 server.js 164 server.js 168 TEST-UNEXPECTED-FAIL | test_delete_multiple_messages_including_the_last_one_with_last_open EXCEPTION: Messages never finished loading. Timed Out. at: test-folder-display-helpers.js line 876 Error("Messages never finished loading. Timed Out.") 0 wait_for_all_messages_to_load() test-folder-display-helpers.js 876 enter_folder([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 252 be_in_folder([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 270 _open_message_in_all_four_display_mechanisms_helper([object XPCWrappedNative_NoHelper],9) test-del etion-with-multiple-displays.js 102 test_delete_multiple_messages_including_the_last_one_with_last_open() test-deletion-with-multiple- displays.js 683 frame.js 460 frame.js 512 frame.js 554 frame.js 412 frame.js 565 server.js 164 server.js 168 make: *** [mozmill-one] Error 1

WIP - just one test failure left (and figuring out how to test the checks now left out)

With this.__proto__.__proto__.onMessagesRemoved.call(this) before closing makes all the unit tests happy.

Comment on attachment 410308 [details] [diff] [review] proposed fix, v7 >+ >+ onMessagesRemoved: function StandaloneFolderDisplayWidget_onMessagesRemoved() { >+ // Close the standalone window if there's nothing selected (i.e. the >+ // message that was displayed is now deleted) and the pref to close window. >+ if (this.treeSelection.count == 0 && >+ pref.getBoolPref("mail.close_message_window.on_delete")) { >+ window.close(); >+ } >+ // Close the stand alone window if either of these are true: Please be consistent wrt "stand alone" and "standalone". Please also add an "also" here. >+ // - this was the last message in this view >+ // - this was the last message in this view (in imap mark-as-deleted) >+ else if (this._nextViewIndexAfterDelete == nsMsgViewIndex_None || >+ (this._nextViewIndexAfterDelete != nsMsgViewIndex_None && >+ this.view.dbView.getKeyAt(this._nextViewIndexAfterDelete) == nsMsgKey_None)) { >+ // XXX this is really hacky :( Works around that window.close() would >+ // close also other open standalone windows for some reason. We know why this is happening, right? This is happening because onMessagesRemoved is only a hint and not a firm notification. >+ let windowWatcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"] >+ .getService(Components.interfaces.nsIWindowWatcher); >+ let mailWindow = Components.classes["@mozilla.org/appshell/window-mediator;1"] >+ .getService(Components.interfaces.nsIWindowMediator) >+ .getMostRecentWindow("mail:3pane"); >+ // If we're doing the delete of the mail opened in a standalone window from the >+ // 3pane, don't close the 3pane due to that. >+ if (mailWindow != windowWatcher.activeWindow) >+ windowWatcher.activeWindow.close(); This doesn't work for the case where you delete a message from a search window and it's the last message. The search window closes. Does something like simply checking if (window == windowWatcher.activeWindow) work? Because anything more than that seems like it's going to complicate matters hugely. >+ // - Verify all displays... except for the standalone msg window. >+ _verify_message_is_displayed_in(VERIFY_FOLDER_TAB | >+ VERIFY_MESSAGE_TAB | >+ VERIFY_BACKGROUND_MESSAGE_TAB, >+ curMessage, 2); >+ // FIXME Here and in the other FIXMEs, the window gets closed, right? Would checking if the window's now closed not work here? >+ //FIXME - test ^^^ fails ?

Gah, that was the wrong patch :/ Uploaded v6 twice.

(In reply to comment #65) > We know why this is happening, right? This is happening because > onMessagesRemoved is only a hint and not a firm notification. I still don't really understand why that would play in, but i added that to the comment.

(In reply to comment #65) > Here and in the other FIXMEs, the window gets closed, right? Would checking if > the window's now closed not work here? I would have thought that too, but it doesn't work... I guess things somehow get left in an odd state.

Comment on attachment 410855 [details] [diff] [review] proposed fix, v8 >+ // Also close the standalone window if either of these are true: >+ // - this was the last message in the list - prevent backslide >+ // - the pref to close is set (imap mark-as-deleted) >+ // - this was the last message in the list (in imap mark-as-deleted) >+ else if (this._nextViewIndexAfterDelete == nsMsgViewIndex_None || >+ pref.getBoolPref("mail.close_message_window.on_delete") || >+ (this._nextViewIndexAfterDelete != nsMsgViewIndex_None && >+ this.view.dbView.getKeyAt(this._nextViewIndexAfterDelete) == nsMsgKey_None)) { This logic continues to be extremely dubious given the event it is hung off of. As module owner, I'm going to step in and say that this patch cannot land with this logic and without mozmill test permutations that use a search-view-backed view (which comes from a search window; a gloda search would also be okay; a virtual folder is not sufficient.)

Comment on attachment 410855 [details] [diff] [review] proposed fix, v8 minusing per asuth's comments.

(In reply to comment #69) > This logic continues to be extremely dubious given the event it is hung off of. What's so dubious about it? Again, this isn't anything new, it's just how things have always been. This same stuff was done earlier and still is in seamonkey - http://mxr.mozilla.org/comm-central/source/suite/mailnews/messageWindow.js#210 If gloda (or something else) is interpreting the event differently, they need to change. > As module owner, I'm going to step in and say that this patch cannot land with > this logic and without mozmill test permutations that use a search-view-backed > view (which comes from a search window; a gloda search would also be okay; a > virtual folder is not sufficient.) Are there similar tests somewhere?

(In reply to comment #67) > Created an attachment (id=410855) [details] > proposed fix, v8 > > (In reply to comment #65) > > We know why this is happening, right? This is happening because > > onMessagesRemoved is only a hint and not a firm notification. > > I still don't really understand why that would play in, but i added that to the > comment. STR (this is with window.close() instead of the activeWindow hackery): 1. Set the close window at delete option 2. Open several message windows to different messages within the same folder. 3. Delete one of the messages from either one of the windows or the 3-pane. Every standalone window receives the onMessagesRemoved notification, and with the |pref.getBoolPref("mail.close_message_window.on_delete")| clause in v8, every standalone window closes.

I see, thx for the explanation! Anyway, i have still to hear any evidence that the latest patch would have bad practical side effects. I'm getting really tired of jumping through hoops for this bug :(

Comment on attachment 408687 [details] [diff] [review] [checked in] patch, v5 Let's go with v5 for now.

(In reply to comment #73) > I see, thx for the explanation! > > Anyway, i have still to hear any evidence that the latest patch would have bad > practical side effects. A simple example would be that (close window on delete is set, and) while a standalone window with message A open is focused, something runs in the background and deletes a message B in the same folder.

I can't think of what that something would be?

retention settings are one possibility, but there are others

That haven't stopped us before... So what would be the solution then?

http://hg.mozilla.org/comm-central/rev/c34e7014c17d http://hg.mozilla.org/releases/comm-1.9.1/rev/c6a18c37c3c0 asuth thinks that the remaining stuff should be handled in another bug, so I'm going to close this one.

(In reply to comment #78) > That haven't stopped us before... The bottom line is 'folder display' has a feature matrix that is way too large. The pre-refactoring code was an evolved mess, but generally worked. Unfortunately "generally worked" is a relative concept since there was no definition of how things were supposed to work. There were approximately no comments that described how things were supposed to work, and there were no unit tests that codified things either. Because the feature matrix is so large, it is very difficult to test all cases. It is also not clear we should have all the cases that we have. Which is why in comment #13 I suggested the option that we simply remove this feature. What we have been doing is writing an expanding set of tests that cover the default or explicitly supported situations. The idea has been that as we enhance things or fix things that turn out to be broken we add additional unit tests to codify these things and avoid regressions. The core of the patch you have provided through the various iterations endangers the default use cases and maybe even your patch's own use case. That we used to optimistically land changes and hope for the best cannot be disputed. But now that we have unit tests and a more documented and saner implementation, it makes sense that we set the bar higher.

Please reopen, does not close window in RC2 on WinXP, instead shows next message.