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

RESOLVED FIXED

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: myk, Assigned: Karsten Düsterloh)

Tracking

(Blocks: 1 bug, {fixed-seamonkey1.1, fixed1.8.1.1, verified1.8.1.3})

fixed-seamonkey1.1, fixed1.8.1.1, verified1.8.1.3
Dependency tree / graph
Bug Flags:
blocking-thunderbird2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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

11 years ago
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 :).
Status: NEW → ASSIGNED
Flags: blocking-thunderbird2+
(Assignee)

Comment 2

11 years ago
> 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?
(Assignee)

Updated

11 years ago
Blocks: 362567
(Reporter)

Comment 3

11 years ago
(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.
(Assignee)

Comment 4

11 years ago
> 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...

(Reporter)

Comment 5

11 years ago
> 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

11 years ago
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!)
(Reporter)

Comment 7

11 years ago
> 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

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

(Assignee)

Updated

11 years ago
Blocks: 201500
(Assignee)

Comment 9

11 years ago
Created attachment 248134 [details] [diff] [review]
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.
Assignee: mscott → mnyromyr
Attachment #248134 - Flags: superreview?(mscott)
Attachment #248134 - Flags: review?(neil)

Comment 10

11 years ago
(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.

(Assignee)

Comment 11

11 years ago
> 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

11 years ago
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.
Attachment #248134 - Flags: review?(neil) → review+

Updated

11 years ago
Depends on: 362741

Comment 13

11 years ago
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.
Attachment #248134 - Flags: superreview?(mscott)
Attachment #248134 - Flags: superreview+
Attachment #248134 - Flags: approval-thunderbird2+
(Assignee)

Comment 14

11 years ago
Created attachment 248447 [details] [diff] [review]
updated patch for checkin
Attachment #248134 - Attachment is obsolete: true
(Assignee)

Comment 15

11 years ago
Landed on trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed-seamonkey1.1, fixed1.8.1.1
Resolution: --- → FIXED
(Assignee)

Comment 16

11 years ago
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>. :(
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. 
Keywords: verified1.8.1.3
You need to log in before you can comment on or make changes to this bug.