Status

()

Firefox
Bookmarks & History
P1
normal
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Ben Goodger (use ben at mozilla dot org for email), Assigned: Annie Sullivan)

Tracking

({fixed1.8.1})

Trunk
Firefox 2 alpha2
fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: swag: 1 day left)

Attachments

(1 attachment, 3 obsolete attachments)

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
Priority: -- → P1
Annie, 

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

;-)
Assignee: bugs → annie.sullivan
(Assignee)

Updated

12 years ago
Whiteboard: swag: 1 day left
(Assignee)

Comment 2

12 years ago
Created attachment 217214 [details] [diff] [review]
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.
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.

(Assignee)

Comment 4

12 years ago
Created attachment 217901 [details] [diff] [review]
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.
Attachment #217214 - Attachment is obsolete: true
You should be able to initialize the menupopup by doing:

place="place:&amp;folder=1&amp;group=3&amp;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

12 years ago
(In reply to comment #5)
> You should be able to initialize the menupopup by doing:
> place="place:&amp;folder=1&amp;group=3&amp;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.
(Assignee)

Comment 7

12 years ago
Created attachment 218035 [details] [diff] [review]
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.
Attachment #217901 - Attachment is obsolete: true
Attachment #218035 - Flags: review?(bugs)
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.
Attachment #218035 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

12 years ago
Created attachment 218212 [details] [diff] [review]
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.
Attachment #218035 - Attachment is obsolete: true
Attachment #218212 - Flags: review?(bugs)
Comment on attachment 218212 [details] [diff] [review]
Patch addressing Ben's comments

r=ben@mozilla.org
Attachment #218212 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

12 years ago
checked in on trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 330153
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.