588807 - Add Unsorted Bookmarks to Bookmarks menu and rename Personal Toolbar to Bookmarks Toolbar

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Since the landing of Places-based bookmarks, Unsorted Bookmarks is available from the Bookmarks Manager and the Bookmarks sidebar. However it is missing from the Bookmarks menu. That's consistent with Firefox, but not consistent in itself. It should be added to the menu.

Comment 1

[Robert Kaiser]

I'm not 100% sure how useful it is to have it around on either the bookmarks toolbar button or the bookmarks menu.

Marco, I probably copied the difference from the Firefox bookmarks toolbar button and menu, what specific reason is there for including it in one and not the other?

Comment 2

[[e-mail]]

So, in bug 544817, the BMB_unsortedBookmarksFolderMenu was only ever added to the bookmarks button, but KaiRo must have copied the gnomestripe CSS file which referred to a bogus unsortedBookmarksFolderMenu.

Comment 3

[Marco Bonardo [:mak]]

(In reply to comment #1)
> Marco, I probably copied the difference from the Firefox bookmarks toolbar
> button and menu, what specific reason is there for including it in one and not
> the other?

That stuff is still being examinated to make it coherent based on scope, no obvious reasons.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

I'll try to get this done, and rename remaining "Personal Toolbar" occurrences to "Bookmarks Toolbar" while I'm at it. Then, in yet another bug, I shall (try to) change the default File Bookmark(s) location to Unsorted Bookmarks. This madness needs to end.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

The patch will also fix bug 588191.

Note: I intenionally left out Help since that's an extra-huge task, given that we have no documentation that takes Places-based bookmarks into account yet at all.

Comment 6

[[e-mail]]

Comment on attachment 587735 [details] [diff] [review]
patch

This doesn't look as if it's the right way to set the label for the menu. Presumably we have to adapt BookmarksMenuButton from browser-places.js?

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(In reply to [e-mail] from comment #6)
> This doesn't look as if it's the right way to set the label for the menu.

I don't really see the point of lazily setting the label. Is it because the entry can be removed (didn't check whether you can)?

> Presumably we have to adapt BookmarksMenuButton from browser-places.js?

Done.

Comment 8

[[e-mail]]

(In reply to Jens Hatlak from comment #7)
> (In reply to comment #6)
> > This doesn't look as if it's the right way to set the label for the menu.
> I don't really see the point of lazily setting the label. Is it because the
> entry can be removed (didn't check whether you can)?
Probably it's to avoid duplicating the property entry (which is needed for the Bookmark Tree in the manager and sidebar).

Comment 9

[[e-mail]]

Comment on attachment 588175 [details] [diff] [review]
patch v2

>+                 onpopupshowing="BookmarksMenu.onPopupShowing(event, '');
This gave me a shock until I realised that our existing references to BookmarksMenu are stale. Leaving them in will confuse people though ;-)

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #9)
> >+                 onpopupshowing="BookmarksMenu.onPopupShowing(event, '');
> This gave me a shock until I realised that our existing references to
> BookmarksMenu are stale. Leaving them in will confuse people though ;-)

I'll address this and upload a new patch then.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2a [Checkin: Comment 12]

The removal of bookmarksPopupset, which was missed in bug 580660 (it even was in the context of its patch!), is the only change to v2.

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 588545 [details] [diff] [review]
patch v2a [Checkin: Comment 12]

http://hg.mozilla.org/comm-central/rev/d29573dcbadb