381245 - Toolbar icon to toggle bookmarks sidebar is missing

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dan Mills [:thunder]]

With places, the icon that toggles the bookmarks sidebar has disappeared.  We should add it back.

Comment 1

[Dan Mills [:thunder]]

Attachment: Add back the bookmarks toolbar icon

Not tested on mac due to bug 358446, but tested on vista and works for me as expected.

Comment 2

[Dan Mills [:thunder]]

Attachment: Add back the bookmarks toolbar icon v2

Ugh, try this one instead.

Comment 3

[Dan Mills [:thunder]]

Attachment: Add back the bookmarks toolbar icon v3

Sorry for the spam and the crappy patches.  This version should work for real now.  Incl. dnd links from the page onto the icon.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 265602 [details] [diff] [review]
Add back the bookmarks toolbar icon v3

Bug 381245: Add back the toolbar icon to toggle the bookmarks sidebar.
>
>diff -r d1451cc668d4 browser/base/content/browser.js
>--- a/browser/base/content/browser.js	Sat May 19 01:00:47 2007 -0400
>+++ b/browser/base/content/browser.js	Mon May 21 12:24:16 2007 -0700
>@@ -2786,29 +2786,34 @@ function openHomeDialog(aURL)
>       var homeButton = document.getElementById("home-button");
>       homeButton.setAttribute("tooltiptext", aURL);
>     } catch (ex) {
>       dump("Failed to set the home page.\n"+ex+"\n");
>     }
>   }
> }
> 
>-#ifndef MOZ_PLACES_BOOKMARKS
> var bookmarksButtonObserver = {
>   onDrop: function (aEvent, aXferData, aDragSession)
>   {
>     var split = aXferData.data.split("\n");
>     var url = split[0];
>     if (url != aXferData.data) {  //do nothing if it's not a valid URL
>+#ifndef MOZ_PLACES_BOOKMARKS
>       var dialogArgs = {
>         name: split[1],
>         url: url
>       }
>       openDialog("chrome://browser/content/bookmarks/addBookmark2.xul", "",
>                  BROWSER_ADD_BM_FEATURES, dialogArgs);
>+#else
>+    var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
>+    uri = ios.newURI(uri, null, null);

s/uri/url/

>+    PlacesUtils.showAddBookmarkUI(uri, split[1]);
>+#endif
>     }
>   },

fix indent here please

r-minusing b/c of the uri/url issue

Comment 5

[Dan Mills [:thunder]]

Attachment: Add back the bookmarks toolbar icon v4

Fixed uri/url issue, properly declared uri.
Fixed indentation.

Comment 6

[Dan Mills [:thunder]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.791; previous revision: 1.790
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.342; previous revision: 1.341
done

Comment 7

[Asaf Romano (gone)]

Shouldn't you use showMinimalAddBookmarkUI* in this case?

Comment 8

[Asaf Romano (gone)]

Also, in browser.js you should use makeURI to create uris.

Comment 9

[Dan Mills [:thunder]]

Quite likely so, I didn't know about either call.
I'll submit a patch to fix that, should I reopen this bug or file a followup?

Comment 10

[Asaf Romano (gone)]

Either way works for me.

Comment 11

[Asaf Romano (gone)]

and no, that should have been uri, url was already used as a string.

Comment 12

[Dan Mills [:thunder]]

He meant the argument to newURI.  The uri declaration was also missing the "var", which I fixed.

I'll just reopen this bug, it seems easier.

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: follup w/ mano's changes

Comment 14

[Dietrich Ayala (:dietrich)]

(In reply to comment #13)
> Created an attachment (id=266129) [details]
> follup w/ mano's changes
> 

note that i haven't tested this yet, due to bug 358446.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 266129 [details] [diff] [review]
follup w/ mano's changes

r=mano.

Comment 16

[Dietrich Ayala (:dietrich)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.794; previous revision: 1.793
done

Comment 17

[Tracy Walker [:tracy]]

verified with Mac and Windows trunk builds of 06/07

Comment 18

[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