As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 588807 - Add Unsorted Bookmarks to Bookmarks menu and rename Personal Toolbar to Bookmarks Toolbar
: Add Unsorted Bookmarks to Bookmarks menu and rename Personal Toolbar to Bookm...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.9
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
Depends on: SMPlacesBMarks
Blocks: 588191
  Show dependency treegraph
 
Reported: 2010-08-19 10:06 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2012-01-14 07:01 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.45 KB, patch)
2012-01-11 09:46 PST, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Splinter Review
patch v2 (9.61 KB, patch)
2012-01-12 13:47 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review
patch v2a [Checkin: Comment 12] (10.74 KB, patch)
2012-01-13 15:29 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review

Description User image Jens Hatlak (:InvisibleSmiley) 2010-08-19 10:06:00 PDT
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 User image Robert Kaiser 2010-08-19 11:01:08 PDT
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 User image neil@parkwaycc.co.uk 2010-08-21 15:50:58 PDT
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 User image Marco Bonardo [::mak] 2010-08-23 09:23:46 PDT
(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 User image Jens Hatlak (:InvisibleSmiley) 2012-01-10 14:39:04 PST
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 User image Jens Hatlak (:InvisibleSmiley) 2012-01-11 09:46:58 PST
Created attachment 587735 [details] [diff] [review]
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 User image neil@parkwaycc.co.uk 2012-01-11 14:22:00 PST
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 User image Jens Hatlak (:InvisibleSmiley) 2012-01-12 13:47:43 PST
Created attachment 588175 [details] [diff] [review]
patch v2

(In reply to neil@parkwaycc.co.uk 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 User image neil@parkwaycc.co.uk 2012-01-12 14:26:39 PST
(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 User image neil@parkwaycc.co.uk 2012-01-12 14:34:18 PST
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 User image Jens Hatlak (:InvisibleSmiley) 2012-01-13 02:02:29 PST
(In reply to neil@parkwaycc.co.uk 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 User image Jens Hatlak (:InvisibleSmiley) 2012-01-13 15:29:00 PST
Created attachment 588545 [details] [diff] [review]
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 User image Jens Hatlak (:InvisibleSmiley) 2012-01-14 07:01:06 PST
Comment on attachment 588545 [details] [diff] [review]
patch v2a [Checkin: Comment 12]

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

Note You need to log in before you can comment on or make changes to this bug.