430659 - Saved search in sidebar does not work.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-us; rv:1.9pre) Gecko/2008042402 Minefield/3.0pre Firefox/2.0.0.14
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-us; rv:1.9pre) Gecko/2008042402 Minefield/3.0pre

Saved search(saved query) in sidebar does not work.

Reproducible: Always

Steps to Reproduce:
1.Create saved search(saved query)
2.Open sidebar
3.Crick the saved search
Actual Results:  
Nothing result.

Expected Results:  
Show search result.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042400 Minefield/3.0pre

Does indeed nothing. I don't know if this is intended.

Comment 2

[Tracy Walker [:tracy]]

This should show the search results

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

I bet we just forgot to tell the sidebar that these things are folders.

Comment 4

[Marco Bonardo [:mak]]

there was a previous fix for this IIRC so could have regressed recently? Can someone take a look if this is a recent or an old one?

Comment 5

[Ria Klaassen (not reading all bugmail)]

The search broke on 19 Nov 2007: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1195521720&maxdate=1195524719
The next day, 20 Nov 2007, the search was fixed but the new saved searches did not expand anymore in the sidebar. But the old saved searches (from before the range) are still expanding, even now.
I don't know if it was repaired later, couldn't find it with random checks.

Comment 6

[Alice0775 White]

When I create New Search, Library generate a query with expandQueries=0.

I think it must be expandQueries=1.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is still any QA work needed on this bug? It looks like a regression from bug 387746?

Comment 8

[Marco Bonardo [:mak]]

why are we doing this in treeView.js

  isContainer: function PTV_isContainer(aRow) {
    this._ensureValidRow(aRow);

    var node = this._visibleElements[aRow].node;
    if (PlacesUtils.nodeIsContainer(node)) {
      // the root node is always expandable
      if (!node.parent)
        return true;

      // treat non-expandable queries as non-containers
      if (PlacesUtils.nodeIsQuery(node)) {
        asQuery(node);
        return node.queryOptions.expandQueries;
      }
      return true;
    }
    return false;
  },

should not expandQueries be referred to children of the query instead of to the query itself?
would not be more correct doing return asQuery(node.parent).queryOptions.expandQueries;?
This way treeView.isContainer returns false in the sidebar, so the query does not expand, and i also cannot middle-click it to open contents (since we use isContainer to check if middle-click is enabled).

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> When I create New Search, Library generate a query with expandQueries=0.
> 
> I think it must be expandQueries=1.
> 

expandQueries=1 means that any *children* of the container that are queries should not be expanded.

we don't want place: URI bookmarks in search results and query contents to be shown as containers. (considering they're containers, maybe we shouldn't show them at all...)

(In reply to comment #8)
> 
> should not expandQueries be referred to children of the query instead of to the
> query itself?

yes, this is correct.

> would not be more correct doing return
> asQuery(node.parent).queryOptions.expandQueries;?

yes.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: fix, per comment #8

with this, queries are properly expandable in the sidebar, and not so in the library.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 318686 [details] [diff] [review]
fix, per comment #8

you need to check if node.parent is a query first.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: v2 - comment fixed

Comment 13

[Asaf Romano (gone)]

Comment on attachment 318690 [details] [diff] [review]
v2 - comment fixed

null-check parent.

r=mano otherwise.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 318690 [details] [diff] [review]
v2 - comment fixed

drivers: low impact fix

Comment 15

[Damon Sicore (:damons)]

Comment on attachment 318690 [details] [diff] [review]
v2 - comment fixed

a1.9+=damons

Comment 16

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.68; previous revision: 1.67
done

Comment 17

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050104 Minefield/3.0pre as well as the Win Vista nightly.

Comment 18

[Aakash Desai [:aakashd]]

Test case https://litmus.mozilla.org/show_test.cgi?id=7506 was created on the litmus 3.0 test run for regression testing this bug.

Comment 19

[Aakash Desai [:aakashd]]

Test case https://litmus.mozilla.org/show_test.cgi?id=7507 was created on the
litmus 3.1 test run for regression testing this bug.

Comment 20

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv