Closed Bug 416007 Opened 12 years ago Closed 12 years ago

Bookmark details persist during search

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: madhava, Assigned: Techrazy.Yang)

References

(Depends on 1 open bug)

Details

(Whiteboard: [has patch][has review])

Attachments

(2 files, 3 obsolete files)

(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.
You want the bottom panel temporarily go away during a search?
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.
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?
Flags: blocking-firefox3?
(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
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
We can just clear the selection and be ok here, right?
Assignee: nobody → mak77
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!

Attached patch A proposal patch (obsolete) — Splinter Review
Just call onContentTreeSelect in showSearchUI and doSearch method.
Comment on attachment 311563 [details] [diff] [review]
A proposal patch

Request review
Attachment #311563 - Flags: review?(mano)
Bo, imo the correct way of dealing with this would be reset the selection on the content tree on search
(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!
Attached patch new patch (obsolete) — Splinter Review
Clear selection to make onselect handler to deal with the panel displaying.
Attachment #311563 - Attachment is obsolete: true
Attachment #311563 - Flags: review?(mano)
change comments

//Display the panel

to something like

// Clear right pane selection to update details panel

then ask review to Mano
Assignee: mak77 → Techrazy.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!
Attached patch Change the comment (obsolete) — Splinter Review
Make the comment more readable. Thanks to Macro(mak77) very much!
Attachment #311732 - Attachment is obsolete: true
Attachment #311953 - Flags: review?(mano)
Whiteboard: [has patch][needs review mano]
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.
Attachment #311953 - Flags: review?(mano) → review-
(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 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
Attachment #311563 - Attachment is obsolete: false
Attachment #311953 - Attachment is obsolete: true
Whiteboard: [has patch][needs review mano] → [has patch][has review]
Attached patch Revised patchSplinter Review
Change back to onContentTreeSelect function call.
Attachment #311563 - Attachment is obsolete: true
Attachment #313039 - Flags: review?(mano)
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
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
Flags: in-litmus?
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 426610
Flags: in-litmus? → in-litmus+
Test case 6295 has been updated to reflect this bug for future regression testing.
Depends on: 498777
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.