362990 - Customize Message Views dialog's selection should not affect View menu selection

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Closing the "Customize Message Views" dialog causes the View menu to activate the view that was selected in the dialog when it was closed.

But there's nothing about the "Customize Message Views" dialog that implies it is to be used as a selection mechanism, and this behavior seems unexpected (especially when you "cancel" the dialog by pressing escape or clicking its titlebar close button).

Rather, the dialog presents itself as a means for editing views, and in theory you don't even have to select a view before closing it (although at the moment bug 362567 causes it to reopen if you don't).

In fact, if you use the dialog to delete a view, the View menu will activate the View that happens to be after the one you deleted, since that one gets autoselected, even though there is little reason to expect that a user who deletes a view wants to switch to the view that happens to appear after it in the list.

It seems like the dialog's selection should not affect the selection in the View menu.

Comment 1

[Scott MacGregor]

I've always found that behavior confusing / unexpected as well.

And fixing this would have the added benefit of also fixing the bug where the add a view dialog won't go away when you hit OK without selecting a view :).

Comment 2

[Karsten Düsterloh]

> In fact, if you use the dialog to delete a view, the View menu will activate
> the View that happens to be after the one you deleted, since that one gets
> autoselected, even though there is little reason to expect that a user who
> deletes a view wants to switch to the view that happens to appear after it in
> the list.

Good point. We should probably then rename the OK button to Close or drop it entirely?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #2)

> Good point. We should probably then rename the OK button to Close or drop it
> entirely?

Renaming it to "Close" seems like a reasonable move.  But I would still suggest not applying the selection in the dialog to the View menu when the user clicks "Close", as there's no obvious connection between the view management that the dialog provides and the view activation that the menu offers.

Comment 4

[Karsten Düsterloh]

> But I would still suggest not applying the selection in the dialog to the View 
> menu when the user clicks "Close"

Yes, that's what my "then" implied: we'd just drop the view changing callback stuff and rename the button...

Comment 5

[Myk Melez [:myk] [@mykmelez]]

> Yes, that's what my "then" implied: we'd just drop the view changing callback
> stuff and rename the button...

Ah, d'oh! :-)  Sounds like a plan.

Comment 6

[Mike Cowperthwaite]

Dropping the OK/Close button is the direction TB and Fx have been going for "manager"-style dialogs like this.  Ideally, in this case, the dialog would no longer block -- that is, instead of a dialog, it would be a top-level window, like the Message Filters window.  After all, if this is no longer making a View selection, why shouldn't the user be able to select a different view while editing some other view?

But maybe not: what happens if I delete the currently-in-use view? (I notice, by the way, that now if I select a view and click Remove, then OK, the deleted view is applied!)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

> what happens if I delete the currently-in-use view?

I'd expect mail windows to then change to the default view (All?).

Comment 8

[Mike Cowperthwaite]

See also bug 311410, which might be affected by changes for this bug.

Comment 9

[Karsten Düsterloh]

Attachment: make view list manipulation asynchronous

With this patch, users can edit their view list without having their mal windows blocked; and the last or no selected view in that dialog won't affect the current folder and its view anymore.
The case when a folder is associated with a deleted view is currently (usually) not detectable, I will fix that when flattening the view menu.

Comment 10

[Scott MacGregor]

(In reply to comment #9)
> Created an attachment (id=248134) [edit]
> make view list manipulation asynchronous
> 
> With this patch, users can edit their view list without having their mal
> windows blocked; and the last or no selected view in that dialog won't affect
> the current folder and its view anymore.
> The case when a folder is associated with a deleted view is currently (usually)
> not detectable, I will fix that when flattening the view menu.
> 

I've been testing this patch out and have noticed two things:

1) The following warning (not sure if this is new)
--DOMWINDOW == 9
WARNING: Illegal character in window name mailnews:mailviewlist: file c:/build/t
rees/offline/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp,
 line 1326

2) When I start up for the first time, click on the views combo box and choose customize, the mail view list dialog opens up just fine. But the combo box itself goes from showing All in the toolbar to showing no  text. If I dismiss the mail view list dialog, I'm still left with an empty selection in the combo box. Once I manually select All (or any valid view) in the drop down list for the first time, I can't get back into this state for that session.

Comment 11

[Karsten Düsterloh]

> 1) The following warning (not sure if this is new)

It's old - at least I didn't touch the windowtype attribute...

> 2) When I start up for the first time, click on the views combo box and choose
> customize, the mail view list dialog opens up just fine. But the combo box
> itself goes from showing All in the toolbar to showing no  text. If I dismiss
> the mail view list dialog, I'm still left with an empty selection in the combo
> box. Once I manually select All (or any valid view) in the drop down list for
> the first time, I can't get back into this state for that session.

Neil noticed that in bug 362567; it's another fallout of the early return in ViewChange, and I think we're going to kill it in bug 362741.

Comment 12

[[e-mail]]

Comment on attachment 248134 [details] [diff] [review]
make view list manipulation asynchronous

> function LaunchCustomizeDialog()
> {
>-  // made it modal, see bug #191188
>   window.openDialog("chrome://messenger/content/mailViewList.xul",
>                     "mailnews:mailviewlist",
>-                    "chrome,modal,titlebar,resizable,centerscreen",
>-                    {onCloseCallback: ViewChangeByCustomValue});
>+                    "chrome,titlebar,resizable,centerscreen");
> }
This is the cause of the assertion - the window name parameter. Unfortunately chrome doesn't like you doing this (this seems particularly nasty for modal windows). One side-effect is that it doesn't raise an existing mail view list window. I think you could easily fix this by using OpenOrFocusWindow.

Comment 13

[Scott MacGregor]

Comment on attachment 248134 [details] [diff] [review]
make view list manipulation asynchronous

approving for the branch too. 

I also marked this dependent on 362741 since that bug fixes the 2nd issue I had when testing this patch. Let's make sure that gets on the branch too.

Comment 14

[Karsten Düsterloh]

Attachment: updated patch for checkin

Comment 15

[Karsten Düsterloh]

Landed on trunk and MOZILLA_1_8_BRANCH.

Comment 16

[Karsten Düsterloh]

Oops, I wrongly credited IanN for SM branch approval when checking this in, but I just took over KaiRo's a+ from the alternative solution in <https://bugzilla.mozilla.org/show_bug.cgi?id=362567#c14>. :(

Comment 17

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.3 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.3) Gecko/20070326 Thunderbird/2.0.0.0 Mnenhy/0.7.5.0 ID:2007032620 (Thunderbird 2 RC1). The Customized Messaged Views is working fine now.