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 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 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 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 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 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 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 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 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 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 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 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 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 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.