329180 - Places Popup

Status: RESOLVED FIXED

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

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Per: http://wiki.mozilla.org/Places:User_Interface#Places_Popup

Popup with:
- search tag-autocomplete field
- tag selector 
- browse tree view
- view content selector
- organize button
- collapso button

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Annie, 

after all that great work on Open in Tabs, I know you love popups. Enjoy!

;-)

Comment 2

[Annie Sullivan]

Attachment: First pass at some functionality

I'm not sure what the best way to pre-populate the menulist and tree with a default place is.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Looks good so far. One thing I would tweak is make the menu.xml binding have an option to not build children for containers at the top level, and then use that for the <menupopup> attached to the menulist instead of generating it yourself.

Comment 4

[Annie Sullivan]

Attachment: 2nd pass at popup

I got the popup mostly working on Windows.  There are a couple of problems:
1. The popup doesn't move with the window if you move the window around.  I can add some javascript to listen for move events and try to catch up, but I'm not sure if there's a better way to do this.
2.  On Mac, the "dependent" window argument doesn't work the same way it does on windows.  If I open the places popup, then focus another window, then focus the Firefox window, the places popup ends up behind the firefox window.
3.  This isn't working in Linux.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

You should be able to initialize the menupopup by doing:

place="place:&folder=1&group=3&excludeItems=1"

instead of constructing a query manually. It's OK to use a place: URI here I think because it's not ending up in the user's bookmarks file so if the format changes we can change the code.

Comment 6

[Brett Wilson]

(In reply to comment #5)
> You should be able to initialize the menupopup by doing:
> place="place:&folder=1&group=3&excludeItems=1"

I would say don't do this. The string format will change before final release. It will be easier to update everything if it's an API call instead of a hardcoded string. I also would warn people in the IDL not to use strings. You should treat them as opaque IDs whenever possible.

Besides, the code path is to parse this string, and initialize the structures that are being done here, which will be less efficient.

Comment 7

[Annie Sullivan]

Attachment: Places popup tested on Mac, Windows, and Linux

I tested this patch on Mac, Windows, and Linux.  The main bug to work out is that the popup doesn't move with the window.  Additionally, on Mac-only, there isn't really a concept of a dependent window, so we need to figure out how to make that work.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 218035 [details] [diff] [review]
Places popup tested on Mac, Windows, and Linux

Brett, the reason I had you add this place: URI is so that we could do things like this. 

I don't see any problem with updating the place: URIs everywhere if you change the format. I volunteer to do the work. Remember to introduce versioning when you do. 

As for efficiency, I would rather have cleaner code since I'm not sure that this will be a huge time suck. I'd rather see place: parsing show up on qfy runs before doing things like this.

>Index: browser/components/places/content/placesPopup.js
>+  /**
>+   * Initializes the popup
>+   */
>+  init: function() {
>+    // Set up the query for the menulist
>+    var query = PlacesController.history.getNewQuery();
>+    query.setFolders([PlacesController.bookmarks.placesRoot], 1);
>+    var options = PlacesController.history.getNewQueryOptions();
>+    options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1);
>+    options.excludeItems = true;
>+    this._menuResult = PlacesController.history.executeQuery(query, options);

Use the place="" attribute as I suggested.

>+    // Populate the menulist with the results from the query
>+    this._menuResult.root.containerOpen = true;
>+    var cc = this._menuResult.root.childCount;
>+    for (var i = 0; i < cc; i++) {
>+      var child = this._menuResult.root.getChild(i);
>+      var menuitem = document.createElement("menuitem");
>+      menuitem.setAttribute("label", child.title);
>+      menuitem.setAttribute("uri", child.uri);
>+      this._popup.appendChild(menuitem);
>+    }
>+    this._menuResult.root.containerOpen = false;

This should be automatic for menupopup type="places", no? You might need to call _rebuild manually then set the selected item after that.

Comment 9

[Annie Sullivan]

Attachment: Patch addressing Ben's comments

This patch addresses Ben's comments about using the menu binding.  Had to add a nosubmenus attribute to the menu binding to get it to work.

Also adds Ctrl+H -> Places popup and makes double-clicking a link close the popup.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 218212 [details] [diff] [review]
Patch addressing Ben's comments

r=[e-mail]

Comment 11

[Annie Sullivan]

checked in on trunk and branch.

Comment 12

[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