246431 - Add "New Bookmark" to the bookmarks context menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[:deanis74]

I thought I'd seen a bug about this before, but I can't find it now.  I don't
see any sense in having a New Folder context menu item in the Bookmarks Sidebar
and Manage Bookmarks and not having New Bookmark.  I create many more bookmarks
than bookmark folders, as I'm sure does the average user.  Creating a bookmark
is also very context-sensitive as the position of the right-click dictates where
to create the bookmark, either in relation to the currently-selected bookmark or
in the selected folder.

Comment 1

[Blake Ross]

Would be nice to have, but not blocking 1.0. Dean, I bet you could easily make a
patch for this and drive it in ;)

Comment 2

[:deanis74]

Attachment: cvs diff -u mozilla/browser/components/bookmarks

This patch:

1. Adds "New Bookmark" and "New Separator" items to the bookmarks context menu
2. Adds an ellipsis to "New Separator" (don't ask me why I needed to do that in
two places)
3. Fixes bug 204406 by changing the access key for New Folder from 'F' to 'L'.

Comment 3

[:deanis74]

Comment on attachment 152082 [details] [diff] [review]
cvs diff -u mozilla/browser/components/bookmarks

Here it is, Blake.  How about a review?

Comment 4

[:deanis74]

Requesting blocking-aviary1.0 again now that there's a patch.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 152082 [details] [diff] [review]
cvs diff -u mozilla/browser/components/bookmarks

The one thing I'm not sure I like about this patch is exposing the New Separtor
(and its dialog with the label) even more.

Unless separator labels are going to work in the BTF and bookmarks menus, I'm
thinking we should hide/disable the UI to add the labels.

Otherwise this looks fine.  r=me, for now, and I'll have to talk to ben about
the separator label stuff.

Comment 6

[:deanis74]

Is it the New Separator menu item or just the actual dialog that you don't like?
 For now, while the label UI is in there, at least this patch indicates that an
intermediate dialog is going to appear, via the added ellipsis.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 152082 [details] [diff] [review]
cvs diff -u mozilla/browser/components/bookmarks

go ahead and land this as-is, I'll kill the ellipsis when I fix bug 230370
(once someone fixes bug 236696, we can revisit that)

Comment 8

[:deanis74]

Mike, I won't be around my build machine for the next few days.  Any chance you
could check this in for me?

Comment 9

[Mike Connor [:mconnor]]

sure, no problem.

I think I'll drop the ellipsis stuff before I check in though.

Comment 10

[:deanis74]

Only if you drop the dialog at the same time.  Otherwise the ellipsis should be
there to indicate to the user that there's an additional step before the
separator is created.

Comment 11

[Mike Connor [:mconnor]]

that's the plan, of course :)

Comment 12

[:deanis74]

Attachment: patch without ellipsis

Fixed on branch alongside bug 230370.  This is what was checked in.  Still need
to fix this on the trunk when it opens.

Comment 13

[Robert Parenton]

Adding fixed-aviary1.0 keyword per comment #12

Comment 14

[:deanis74]

I missed one case.  Mike, can you give me the go-ahead to check in this change
tonight?

-    case "BookmarkSeparator":
-      commands = ["bm_newfolder", "bm_separator", 
+      commands = ["bm_newbookmark", "bm_newfolder", "bm_newseparator",
"bm_separator",

Comment 15

[Mike Connor [:mconnor]]

r/a=me on that added bit.

Comment 16

[:deanis74]

Fixed on trunk and branch.  (The trunk was open when I checked in - honest!)

Comment 17

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change