303895 - Threadpane drop-down list 'View->Customize..." opens Saved Search

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Jean-Marc Desperrier]

Steps to reproduce.

1. Open the thread-pane with the Search Bar at top
2. Click on the 'View' drop-down list
3. Select the last choice in the list : "Customize..."
4. The "New Saved Search Folders" dialog opens.

The "Customize Message Views" dialog should open instead.

If you do it by using the menu (View/Messages/Customize...), the correct dialog
opens.

In addition, 'Save Search as a Folder' that is present in the middle of the
drop-down list in Thunderbird is missing. This is the one choice that should
open the "New Saved Search Folders" dialog.

What's more I can confirm bug 271792 fully. As "New Saved Search Folders" as a
cancel button and "Customize Message Views" doesn't, the reporter of bug 271792
was in fact seeing the behavior described here in addition to the problem he
reported.

Comment 1

[Ian Neal]

Attachment: Add missing entry and renumber patch v0.1

This patch:
* Renumbers the Customize menuitem from 7 to 8
* Adds missing menuitem 7 for saved search

This was missed out from the patch for bug 11051 and probably one of my bugs
too.

Comment 2

[Jean-Marc Desperrier]

The patch works well for me.

It doesn't really change bug 271792, just "moves" it. As the mis-direction from
"Customize..." to the "New Saved Search Folders" dialog is corrected, you can no
more reproduce bug 271792 with the "Customize..." choice after the patch, but
you still get it when you take the "Save Search as a Folder..." choice in the list.

I'll still leave this as blocking 271792, because it would make no sense to try
to correct bug 271792 without correcting this first.

Comment 3

[Ian Neal]

Attachment: Moved items patch v0.1a

Changes since v0.1:
* Alters order of items in menu
* Adjusts JS so that it does not rely on certain items being in certain places
in the menu - the JS is shared between mailnews and TB so has to work with
both.

I can spin this off into another bug if you prefer.

Comment 4

[[e-mail]]

Comment on attachment 192511 [details] [diff] [review]
Moved items patch v0.1a

One thing that I noticed with both patches is that if you custom after save,
then the menuitem gets set to save instead of the original view. So this r- is
really for both patches.

>+    if (menupopupNode.childNodes[i].getAttribute('id').substr(0, 15) == "userdefinedview")
You can use .id rather than getAttribute

>+         <menuitem id="createCustomView" value="8" label="&viewPickerCustomView.label;"/>
>+         <menuseparator/>
>+         <menuitem id="saveAsVirtualFolder" value="7" label="&viewPickerSaveAsVirtualFolder.label;"/>
I think save should go before custom, without a separator.

>+<!ENTITY viewPickerSaveAsVirtualFolder.label "Save Search as a Folder...">
I think this should say "Save View as Folder..." as it doesn't save the Quick
Search, right?

Comment 5

[Jean-Marc Desperrier]

(In reply to comment #3)
> Changes since v0.1:
> * Alters order of items in menu
> * Adjusts JS so that it does not rely on certain items being in certain places
> in the menu - the JS is shared between mailnews and TB so has to work with
> both.

From what I see the organisation looks ugly. The DTD and XUL were separated,
only that .js is still imported from seamonkey mail to TB. 
It sounds it would be better to include msgViewPickerOverlay.js inside TB's
mailWindowOverlay.js, just like the respective .xul were. Maybe not now :-)
 
> I can spin this off into another bug if you prefer.

Maybe it's indeed simpler to keep here only the correction for the wrong menu
entry, and move corrections for the selection problems in bug 271792
(SeaMonkey)/bug 267688 (TB). But if you manage to get approval for everything
without annoyingly delaying the correction of 303895, it's not necessary.

(In reply to comment #4)
> (From update of attachment 192511 [details] [diff] [review] [edit])
> One thing that I noticed with both patches is that if you custom after save,
> then the menuitem gets set to save instead of the original view. So this r- is
> really for both patches.

If we'd separate the patch, solving the selection problem would be the matter of
bug 271792/267688, so the r- wouldn't apply here.

> >+         <menuitem id="saveAsVirtualFolder" value="7"
label="&viewPickerSaveAsVirtualFolder.label;"/>
> I think save should go before custom, without a separator.
> 
> >+<!ENTITY viewPickerSaveAsVirtualFolder.label "Save Search as a Folder...">
> I think this should say "Save View as Folder..." as it doesn't save the Quick
> Search, right?

This is the way thunderbird does it, I don't think it would be a great idea to
disalign TB and Seamonkey mail on that.

But it's right that "Save Search as a Folder..." saves the view and not the
quick search, so there's a problem and maybe we should open a TB bug about that.

Comment 6

[Ian Neal]

Attachment: Tweaked patch v0.1b

Changes since v0.1b:
* Changed order and removed separator as suggested
* Using .id instead of getAttribute('id')
* Saved View instead of Saved Search in dropdown

The other issue is Bug 271792 but as far as I am aware it is not part of this
regression.

Comment 7

[Ian Neal]

Attachment: Revised patch v0.1c

Changes since v0.1b:
* Fixed a problem with certain "userdefinedview" menuitems not being deleted as
counter was being increased even when a child was being removed.
* Fixed the issue reported in bug 267688 and bug 271792
(msgViewPickerOverlay.js is shared so a single change will fix for both
mailnews and TB)

Neil - does this fix your issue as I cannot reproduce it?

Comment 8

[[e-mail]]

Comment on attachment 193093 [details] [diff] [review]
Revised patch v0.1c

Yeah, my issue was covered by or related to the two bugs you mentioned.

Comment 9

[[e-mail]]

I noticed that the backend code that opens the saved search folder dialog
differs from the code that opens the customize view dialog - bug 267688 was
probably caused by the difference between the two, and it might make sense to
clean up the saved search option code to work like the customize view option.

Comment 10

[Ian Neal]

Attachment: Tidied up dialog launcher v0.1d

Changes since v0.1c:
* Cleaned up the saved search option code to work like the customize view
option

Comment 11

[[e-mail]]

Comment on attachment 193288 [details] [diff] [review]
Tidied up dialog launcher v0.1d

>   var oldViewValue = gCurrentViewValue;
r=me if you remove this...

>-     val = oldViewValue;
...because this was its only use.

>+  for (var i = 0; i < menupopupNode.childNodes.length; i++)
>   {
>+    if (menupopupNode.childNodes[i].id.substr(0, 15) == "userdefinedview")
>+    {
>+      menupopupNode.removeChild(menupopupNode.childNodes[i]);
>+      --i;
>+    }
>   }
It occurs to me that the code might be simpler if you looped down.

Comment 12

[Ian Neal]

Attachment: Looped down slimmer patch v0.1e

Changes since v0.1d:
* Removed unneeded variable as suggested
* Looped down the for statement for child removal

Comment 13

[[e-mail]]

Comment on attachment 193300 [details] [diff] [review]
Looped down slimmer patch v0.1e

>+  for (var i = menupopupNode.childNodes.length; i > 0; --i)
>   {
>+    if (menupopupNode.childNodes[i - 1].id.substr(0, 15) == "userdefinedview")
>+      menupopupNode.removeChild(menupopupNode.childNodes[i - 1]);
>   }
I don't like those -1s ... there are other ways, such as WeirdAl's attachment
192604 [details] [diff] [review] (although that's not my favourite).

Comment 14

[Ian Neal]

Attachment: Minus 1 patch v0.1f (Checked in trunk and 1.8 branch)

Changes since v0.1e:
* Go from length -1 to 0 so the i - 1 bits become just i

Comment 15

[Ian Neal]

Comment on attachment 193316 [details] [diff] [review]
Minus 1 patch v0.1f (Checked in trunk and 1.8 branch)

Checking in (trunk only)
content/msgViewPickerOverlay.js;
new revision: 1.18; previous revision: 1.17
content/msgViewPickerOverlay.xul;
new revision: 1.8; previous revision: 1.7
locale/en-US/msgViewPickerOverlay.dtd;
new revision: 1.4; previous revision: 1.3
done

Requesting a= for this faily simple, low risk regression fix.

Comment 16

[Asa Dotzler [:asa]]

scott, does this have any potential impact on Tbird?

Comment 17

[Scott MacGregor]

Comment on attachment 193316 [details] [diff] [review]
Minus 1 patch v0.1f (Checked in trunk and 1.8 branch)

this should be fine.

Comment 18

[Scott MacGregor]

Attachment: minor thunderbird change to take advantage of this

Comment 19

[Ian Neal]

Comment on attachment 193316 [details] [diff] [review]
Minus 1 patch v0.1f (Checked in trunk and 1.8 branch)

Checking in (1.8 branch)
mailnews/extensions/mailviews/resources/content/msgViewPickerOverlay.js;
new revision: 1.17.2.1; previous revision: 1.17
mailnews/extensions/mailviews/resources/content/msgViewPickerOverlay.xul;
new revision: 1.7.20.1; previous revision: 1.7
mailnews/extensions/mailviews/resources/locale/en-US/msgViewPickerOverlay.dtd;
new revision: 1.3.76.1; previous revision: 1.3
mail/base/content/mailWindowOverlay.xul;
new revision: 1.116.2.4; previous revision: 1.116.2.3
mail/locales/en-US/chrome/messenger/messenger.dtd;
new revision: 1.17.2.1; previous revision: 1.17
done

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with trunk build 2006-03-25-08 on Windows XP, SeaMonkey.