506448 - Quicksearch - "Save Search as a Folder" not working gSearchSession is not defined

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[raoul bhatia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.2pre) Gecko/20090724 Shredder/3.0b4pre ID:20090724050314

i am unable to save my current quick-search as a folder.

using the dedicated "search messages" form/dialog and pressing "save as a search folder" works as expected.

Reproducible: Always

Steps to Reproduce:
1. enter any text in the quick search
2. press the magnifying lens on the left side of the quick search box
3. choose "save search as a folder"
Actual Results:  
nothing happens

Expected Results:  
"New Saved Search Folder" dialog should appear.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Confirming. Not yet searched for a dupe though. Sounds weird to have a menu item not working, so nominating blocking.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.1) Gecko/20090715 Lightning/1.0pre Thunderbird/3.0b3

Comment 2

[Magnus Melin [:mkmelin]]

Error: gSearchSession is not defined
Source File: chrome://messenger/content/searchBar.js
Line: 236

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: mkmelin's fix with a change to the dialog to be an OR search

mkmelin's fix was good, but while writing the unit test (coming next) I noticed that the "Subject or From" was getting translated into "Subject and From" because the dialog only looks at the booleanAnd attribute of the first term.  However, when any grouping normalization happens, the first guy is going to get marked as booleanAnd.  So if there is a second term, we look to him instead.

The refactoring did not change the logic about grouping search terms, so I expect that this is a long-standing deficiency and that SeaMonkey will benefit from this change, so there is no need to specialize it to be thunderbird-specific.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Attachment: unit test v1

Comment 7

[David :Bienvenu]

I'm having a little trouble with the mozmill test, perhaps because of assertions. I'll try it in a release build when I can...

Comment 8

[David :Bienvenu]

Attachment: mozmill test output

this is the mozmill output from the test. Other mozmill tests seem to work OK, so I think my setup is OK...

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #8)
> Created an attachment (id=392770) [details]
> mozmill test output
> 
> this is the mozmill output from the test. Other mozmill tests seem to work OK,
> so I think my setup is OK...

So the element with id="booleanAndGroup" does not exist.  He comes from an overlay (searchTermOverlay.xul), so this suggests that either the window helper code is either broadly not working or thinks a document is fully loaded before the overlays load.  I will look into this.

Comment 10

[raoul bhatia]

(In reply to comment #5)
> mkmelin's fix was good, but while writing the unit test (coming next) I noticed
> that the "Subject or From" was getting translated into "Subject and From"
> because the dialog only looks at the booleanAnd attribute of the first term. 
> However, when any grouping normalization happens, the first guy is going to get
> marked as booleanAnd.  So if there is a second term, we look to him instead.

might this be related to bug 477656?

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #10)
> might this be related to bug 477656?

No.  The quoting I used is misleading... it is intended to capture the state of the dialog's constraints rather than any literal string.  Bug 477656 would appear to be about the fallout of the quick search widget managing its own emptytext imperfectly.

Comment 12

[David :Bienvenu]

Comment on attachment 392643 [details] [diff] [review]
unit test v1

clearing review request

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #9)
> So the element with id="booleanAndGroup" does not exist.  He comes from an
> overlay (searchTermOverlay.xul), so this suggests that either the window helper
> code is either broadly not working or thinks a document is fully loaded before
> the overlays load.  I will look into this.

If we weren't on 1.9.1 we could use document.readyState (bug 347174).  But we are.  So we need to add a listener for the document loaded event for things we can actually detect opening, but we have a potential problem with the base case of the 3pane where it might already be open.  I'll see if I can abuse a side-effect.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #13)
> If we weren't on 1.9.1 we could use document.readyState (bug 347174).  But we
> are.  So we need to add a listener for the document loaded event for things we
> can actually detect opening, but we have a potential problem with the base case
> of the 3pane where it might already be open.  I'll see if I can abuse a
> side-effect.

It turns out mozmill has its own listener that does this already since r507, which should be in the 1.2 release.  bienvenu, please verify that you are using mozmill 1.2 or the corresponding revision or later.

Specifically mozmill/extension/resource/modules/init.js marks every window that is already open when it starts as "documentLoaded".  It then uses an observer to register for the "load" notification of each window in order to mark it "documentLoaded".  (It also registers for a content loaded notification for its content.)

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

fix pushed:
http://hg.mozilla.org/comm-central/rev/d642a2279822

I stole the blame from Magnus in case the change about the and/or stuff comes back to haunt us.

I'm going to keep this open until the test lands.

Comment 16

[David :Bienvenu]

taking off blocker list.

Yes, it turned out I was running a 1.2 beta mozmill, so it may really be fine with the finished 1.2 mozmill, but I haven't had a chance to try it with the updated mozmill.

Comment 17

[David :Bienvenu]

Comment on attachment 392643 [details] [diff] [review]
unit test v1

works with mozmill 1.2, sorry for the confusion.

Comment 18

[raoul bhatia]

for the records, wfm under ubuntu linux using

Mozilla/5.0 (X11; U; Linux i686 (x86_64); de-DE; rv:1.9.1.3pre) Gecko/20090818 Shredder/3.0b4pre

Comment 19

[Magnus Melin [:mkmelin]]

Landed the mozmill test
changeset:   3556:e2da8c3336ed
http://hg.mozilla.org/comm-central/rev/e2da8c3336ed

->FIXED