416007 - Bookmark details persist during search

Status: VERIFIED FIXED

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

Description

[Madhava Enros [:madhava]]

Attachment: bookmark details persist though a search is going on

(see attached image)

If I select a bookmark, its details are visible in the details pane (bottom half of the screen) -- so far so good.  If I then do a search, though, the search results begin filling in the upper half of the screen, leaving the details of the previously selected bookmark in the bottom half of the screen.  This is problematic in that the details (a) are about a bookmark that has nothing to do with the search results and (b) occupy space that could be better used for the potentially long list of result-set bookmarks.

Comment 1

[Ria Klaassen (not reading all bugmail)]

You want the bottom panel temporarily go away during a search?

Comment 2

[Tracy Walker [:tracy]]

same thing happens when looking at History. There is a bug filed on that.  Bookmark properties should not be present unless a Bookmark is selected.

Comment 3

[Madhava Enros [:madhava]]

Ria: not exactly -- once a search is initiated (ie. when the user hits enter after typing in the search terms), the previously selected bookmark should no longer be selected.  It has no relevance to the current task.

Tracy: which bug is that?

Comment 4

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

(In reply to comment #3)
> Ria: not exactly -- once a search is initiated (ie. when the user hits enter
> after typing in the search terms), the previously selected bookmark should no
> longer be selected.  It has no relevance to the current task.

Indeed. As soon as the search is run, the panel should revert to how it looks when a folder is selected ("n items; select an item to see it's properties") - to see what I mean, just select any folder like the "Unsorted Bookmarks" folder.

> Tracy: which bug is that?

Bug 407541

Comment 5

[Mike Connor [:mconnor]]

We can just clear the selection and be ok here, right?

Comment 6

[Bo Yang]

I think we can call the onContentTreeSelect method to display the bottom panel when searching. And I upload this proposal patch. 

Could anybody can help review it? Thanks!

Comment 7

[Bo Yang]

Attachment: A proposal patch

Just call onContentTreeSelect in showSearchUI and doSearch method.

Comment 8

[Bo Yang]

Comment on attachment 311563 [details] [diff] [review]
A proposal patch

Request review

Comment 9

[Marco Bonardo [:mak]]

Bo, imo the correct way of dealing with this would be reset the selection on the content tree on search

Comment 10

[Bo Yang]

(In reply to comment #9)
> Bo, imo the correct way of dealing with this would be reset the selection on
> the content tree on search
> 

Do you mean in doSearch and search, call clearSelection() of the content tree to make the onselect handler to display the panel? Just like the new patch?

Thanks for your advice!

Comment 11

[Bo Yang]

Attachment: new patch

Clear selection to make onselect handler to deal with the panel displaying.

Comment 12

[Marco Bonardo [:mak]]

change comments

//Display the panel

to something like

// Clear right pane selection to update details panel

then ask review to Mano

Comment 13

[Bo Yang]

(In reply to comment #12)
> change comments
> 
> //Display the panel
> 
> to something like
> 
> // Clear right pane selection to update details panel
> 
> then ask review to Mano
> 

Thank you very much!

Comment 14

[Bo Yang]

Attachment: Change the comment

Make the comment more readable. Thanks to Macro(mak77) very much!

Comment 15

[Asaf Romano (gone)]

Comment on attachment 311953 [details] [diff] [review]
Change the comment

I would rather just call onContentTreeSelect directly, the old view is already gone at this point, thus clearing the selection doesn't make sense.

Comment 16

[Bo Yang]

(In reply to comment #15)
> (From update of attachment 311953 [details] [diff] [review])
> I would rather just call onContentTreeSelect directly, the old view is already
> gone at this point, thus clearing the selection doesn't make sense.
> 

Do you mean this old patch is ok? 
https://bugzilla.mozilla.org/attachment.cgi?id=311563

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 311563 [details] [diff] [review]
A proposal patch

Yes the first patch, this is my fault, sorry Bo :(

+    
+    //Update the bottom panel
+    PlacesOrganizer.onContentTreeSelect();

fix trailing space in the first empty line
ad space after // in comments and use details pane instead of bottom panel.

Then ask for Mano review

Comment 18

[Bo Yang]

Attachment: Revised patch

Change back to onContentTreeSelect function call.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 313039 [details] [diff] [review]
Revised patch

r=mano

Comment 20

[Dietrich Ayala (:dietrich)]

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

Comment 21

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

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre

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

Comment 23

[Aakash Desai [:aakashd]]

Test case 6295 has been updated to reflect this bug for future regression testing.

Comment 24

[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