If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

6.27 KB, patch
Ben Goodger (use ben at mozilla dot org for email)
: review+
Brian Ryner (not reading)
: superreview+
Details | Diff | Splinter Review
Bring Personal Toolbar functionality back, plus add a small "Places" button at the start for easy access.
Depends on: 317631
Assignee: nobody → annie.sullivan
(Assignee)

Comment 1

12 years ago
Created attachment 204569 [details] [diff] [review]
Fixes open in tabs, fixes open commands in context menu grayed out, work on pasting

Added mousedown to the command updater, so that when the activeView is set on personal toolbar mousedown, the enabled/disabled states of the commands are updated.  This fixes a bug where "Open in..." links were sometimes disabled when they should have been enabled.

Added a check in openLinksInTabs() to make sure that only urls are opened, not folders.

Worked on fixing pasting of folders.  Added the flavour for folders to the nsITransferable object in the paste function, changed the call to _getFolderCopyTransaction() from PlacesController to PlacesControllerDragHelper (where it is declared), and sent data instead of data.folderId as the first parameter.  But the call to bms.getFolderTitle(folderId) throws an NS_ERROR_ILLEGAL_VALUE exception, which means that the database query for the folderId returned no results.  So I'm not sure if these changes help.
Attachment #204569 - Flags: review?(bugs)
Comment on attachment 204569 [details] [diff] [review]
Fixes open in tabs, fixes open commands in context menu grayed out, work on pasting

>   paste: function() {
>     var xferable = 
>         Cc["@mozilla.org/widget/transferable;1"].
>         createInstance(Ci.nsITransferable);
>     xferable.addDataFlavor(TYPE_X_MOZ_PLACE);
>     xferable.addDataFlavor(TYPE_X_MOZ_URL);
>     xferable.addDataFlavor(TYPE_UNICODE);
>+    xferable.addDataFlavor(TYPE_X_MOZ_PLACE_CONTAINER);

Is this the right place to be adding the container type? The order that you add things to the transferable is very important. The item added first generally has precedence. Other places in code add the container first, perhaps it should be added that way here too. 

Otherwise, r=ben@mozilla.org
Attachment #204569 - Flags: superreview?(bryner)
Attachment #204569 - Flags: review?(bugs)
Attachment #204569 - Flags: review+
Comment on attachment 204569 [details] [diff] [review]
Fixes open in tabs, fixes open commands in context menu grayed out, work on pasting

>--- browser/components/places/content/controller.js	29 Nov 2005 17:12:02 -0000	1.24
>+++ browser/components/places/content/controller.js	30 Nov 2005 18:11:27 -0000
>@@ -448,19 +448,23 @@ var PlacesController = {
>    */
>   openLinksInTabs: function PC_openLinksInTabs() {
>     var node = this._activeView.selectedNode;
>     if (this._activeView.hasSingleSelection && this.nodeIsFolder(node)) {
>       var queries = node.getQueries({});
>       var kids = this._hist.executeQueries(queries, queries.length,
>                                            node.queryOptions);
>       var cc = kids.childCount;
>-      for (var i = 0; i < cc; ++i)
>-        this._activeView.browserWindow.openNewTabWith(kids.getChild(i).url, 
>+      for (var i = 0; i < cc; ++i) {
>+        var node = kids.getChild(i);
>+        if (!this.nodeIsURL(node))
>+          continue;
>+        this._activeView.browserWindow.openNewTabWith(node.url, 
>                                                       null, null);
>+      }

I think this would be a little clearer with the |if| inverted:

if (this.nodeIsURL(node)) {
  this._activeView.browserWindow.openNewTabWith(node.url, null, null);
}

Looks good otherwise.
Attachment #204569 - Flags: superreview?(bryner) → superreview+

Updated

12 years ago
Blocks: 317881
Priority: -- → P1
Annie, I'm going to close this one out. Can you file separate bugs on remaining features that you know need to be implemented? This will make tracking our progress much easier. 
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.