223337 - Cannot make menu separator anywhere but in bookmark menu

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Avi Drissman]

Build ID: 2003102211 with the huge new bookmark system

One of the new features of the new bookmark system (bug 212630) is menu
separators ("<Menu Spacer>"). Pressing the "S" button creates one, but even if
you're looking at a different place (e.g. Toolbar Bookmarks), it gets created in
the Bookmark Menu.

1. Bookmarks > Manage Bookmarks
2. Click Toolbar Bookmarks
3. Create a folder inside the Toolbar Bookmarks
4. Click the "S" button

What should happen is that the separator is created in the toolbar bookmarks.
However, it's created in the Bookmark Menu.

Comment 1

[Avi Drissman]

Oh, and once it's created in the Bookmark Menu, you cannot drag it to the
Toolbar Bookmarks in the list on the left, nor can you drag it to the folder
that's visible on the toolbar.

Comment 2

[Jasper]

The separator creation should resect the users current selection to create it
there. And the users should be able to create separators not only in the
bokmarks menu but also in the toolbar and other custom folders.

People not only use separators for menu's such as the bookmarks menu but also
use it when arranging their bookmarks in the bookmarks manager. So also think we
should rename it from "<Menu Spacer>" to something more abstract like "--------"

Comment 3

[Jasper]

I can remember that David has it enabled for both folders but for some reason
took it out.

Comment 4

[Josh Aas]

over to Haas

Comment 5

[Josh Aas]

I cannot reproduce this bug. I think its just really old. It isn't possible to
create separators outside of the bookmark menu item.

Can you still reproduce this Jasper? If you can, could you write better
reproduction instructions?

Comment 6

[Jon Hicks]

Is this not referring to the fact that you can't add a menu separator in a folder in the Bookmark Bar ? 
This is still not working in the latest nightly.

Comment 7

[Jasper]

Exactly. The point is that users will want to use separartor not only in the
Bookmarks menu but also in all other bookmark places as it then serves as a tool
to structure your bookmarks.

In my view we should allow users to add a separator in:
- bookmarks menu
- bookmarks toolbar (not in the actual bar but that could be filtered)
- dock menu
- and custom added folders
- in imported folders we should display seperators

But when we do fix this bug we also need to make sure separators look like
separators in the bookmark manager. See FF for an example.

Comment 8

[ender.mb]

FWIW, you can have separators in bookmarks toolbar folders if you move them to
the menu, add a separator, and move them back. However, if you try to open the
folder in tabs, it will open a blank tab in the place of each separator.

Comment 9

[David Haas (gone, not reading mail)]

Attachment: a patch

maybe it works.   maybe it doesn't.  that's what makes it so exciting!

Comment 10

[Josh Aas]

1. There are a few offsets when applying the patch. I don't think it matters,
but you might want to look into it.
-------------------------------
2.
+  if ( [mItemPane numberOfSelectedRows ] == 1 )
+  {

should be

+  if ([mItemPane numberOfSelectedRows ] == 1) {

moz style is to put opening brace on the same line as the if statement. The
exception is the opening brace of a method. Also there shouldn't be spaces on
the inside of the if statement. Happens in a few places. Same thing with for
statements. Also there should not be a space between the colon and an argumet in
calls to obj-c methods.
--------------------------------------
3. Adding separator to dock menu folder results in a blank line where the
separator is in the dock menu. Is that what we want? Seems fine to me since
having actual menu separators might confuse groupings.
--------------------------------------
4. Any chance this patch could include a fix for respecting a user's selection
when adding a separator?

Comment 11

[Jasper]

David itt would be fantastic if you could make sure we respect a user's selection
when adding a separator.

Next big step for separators will be to make sure they look like separators in
the bookmark manager -> Bug 236674

Comment 12

[David Haas (gone, not reading mail)]

(In reply to comment #10)
> 3. Adding separator to dock menu folder results in a blank line where the
> separator is in the dock menu. Is that what we want? Seems fine to me since
> having actual menu separators might confuse groupings.

I think it is adding a separator to the dock menu, it's just that the dock menu displays it as a blank line.  
But I'll look at it again to be sure.

> --------------------------------------
> 4. Any chance this patch could include a fix for respecting a user's selection
> when adding a separator?

Somebody's gonna have to explain this one to me.

Comment 13

[Josh Aas]

I mean, if a user has a bookmark selected and hits the add separator button, the
separator is added to the bottom of whatever folder the selection is in. It
would be nice if it was added where the selection is.

Comment 14

[Geoff "temporarily off bugmail" Beier]

Although this applies with nothing more than warnings about offsets,
BookmarkButton.mm and BookmarkToolbar.mm are both now newer than the versions
used in the patch.

Comment 15

[Geoff "temporarily off bugmail" Beier]

This looks fine and works as advertised, bar the issues Josh raised. If Josh's comments are addressed 
r=[e-mail].

Comment 16

[Josh Aas]

dave - the email address you use doesn't appear to work any more. I tried to
send you something but the address was rejected. Could you update it?

Comment 17

[David Haas (gone, not reading mail)]

Attachment: updated patch

I tweaked some of the style things I found (ie, location of brackets, spaces in
parenthesis, etc).

Also, the separator now gets added in front of the currently selected object. 
I'm not sure if I'd prefer it to end up after the current object - opinions
welcome.  Oh, if nothing is selected, it gets added to the end of the current
active container.

Comment 18

[Josh Aas]

Comment on attachment 169162 [details] [diff] [review]
updated patch

Looks fine to me. I don't have an opinion on whether or not the separator
should be created before or after the selection.

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #18)
> I don't have an opinion on whether or not the separator
> should be created before or after the selection.

FWIW, my preference would be for after the selection; it seems more "logical" to
me but I can't offer any solid logic.  

However, some actual data: among the browsers I have on my Mac, IE and iCab
place the separator after the selection, Firefox places it before the selection,
and, amazingly, Safari doesn't appear to have this feature at all!

Comment 20

[ender.mb]

For completeness' sake, OmniWeb also places the separator after the selection,
which seems more logical to me as well.

Comment 21

[Mike Pinkerton (not reading bugmail)]

i'd prefer it to go *after* the selected element. 

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 169162 [details] [diff] [review]
updated patch

sr=pink with the caveat that it's changed to add the separator after the
selected item.

Comment 23

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 169162 [details] [diff] [review]
updated patch

r+ with the same caveat pink makes.

Comment 24

[Mike Pinkerton (not reading bugmail)]

what's needed to move the separator after? the math wasn't immediately obvious
and i didn't want to screw anything up with a hackjob

Comment 25

[Josh Aas]

Attachment: updated to current trunk

This patch is updated to the current trunk code. The previous patch no longer
applies correctly. Nothing significant has changed, and still need insertion
logic corrected before it can land.

Comment 26

[Josh Aas]

There is a typo in the patch I just posted that still compiles but prevents the
opening of browser windows... oops. New patch coming up, with separator
insertion fixed, which I will land.

Comment 27

[Josh Aas]

Attachment: landed patch

this is the patch I landed

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 301153 has been marked as a duplicate of this bug. ***