391125 - cannot hide open in tabs and friends menuitem(s) from bookmark containers

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[logan]

Attachment: patch v1

The .openintabs-menuitem and .openintabs-menuseparator classes on the elements related to Open All in Tabs bookmark menuitems were never carried over from the old bookmarks stuff. I rather liked being able to hide these..

a userChrome.js alternative (should this go unfixed) would be to kill the onpopupshowing attribute from #bookmarksMenuPopup or override the BookmarksEventHandler.onPopupShowing function.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

could make sense for themes

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 387634 [details] [diff] [review]
patch v1.0

> openintabs-menuseparator
> openlivemarksite-menuitem
> openintabs-menuitem

This seems like an odd setup, because the separator is named as if it separated only the "open all in tabs" menu item from the other items, although it separates the "open site" menu item as well.

Also, use .className instead of .setAttribute?

Comment 3

[Marco Bonardo [:mak]]

(In reply to comment #2)
> (From update of attachment 387634 [details] [diff] [review])
> > openintabs-menuseparator
> > openlivemarksite-menuitem
> > openintabs-menuitem
> 
> This seems like an odd setup, because the separator is named as if it separated
> only the "open all in tabs" menu item from the other items, although it
> separates the "open site" menu item as well.

While this is true, it's only to underline those names come from previous FX versions, so it could be wise to maintain them for compatibility with themes.
The separator includes also open site for livemarks, while it's separating only open in tabs for most common folders... btw, if we want to discard old naming could be containerOptions-menuseparator maybe.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.1

fixed comments

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 391556 [details] [diff] [review]
patch v1.1

>+      target._endOptSeparator.className = "containeroptions-menuseparator";

What's a "container option"? :(

Comment 6

[Dão Gottwald [:dao]]

Maybe bookmark-items-separator?

Comment 7

[Marco Bonardo [:mak]]

it's not separating items, would be even worst imo. it's separating items from actions available for that container... containeractions-menuseparator?

Comment 8

[Dão Gottwald [:dao]]

> it's not separating items, would be even worst imo. it's separating items from [...]

Well, it doesn't separate items from each other, but it does separate items from /something/, whatever that something is.

It took me a while to figure out why you chose "container"... It seems to be very places-specific terminology that's used mostly internally. It doesn't make a lot of sense as a generic class name, I think.

Comment 9

[Marco Bonardo [:mak]]

container is a generic enough term: an array is a container, a box is a container, a folder is a container, a bag is a container... not really Places specific, just this is separating items from action applicable to what contains them. i see you point, but i'm not sure i can find a better term, maybe bookmarks-actions-separator? but this can be applied also to queries reporting only history entries not bookmarked...

i don't like bookmark-items-separator because you can have other separators between items, as many as you will, and they won't obey this class, so it's confusing having a class that does not apply to what you expect.

Comment 10

[Dão Gottwald [:dao]]

By "generic" I meant a class name that can be clearly understood without specific knowledge about places. Without looking at places code, "container" can mean anything as you point out, so it's not particularly clear what it means in this case.

"actions" isn't great either, but I guess it's the best option so far. I'd be content with bookmarks-actions-menuseparator.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/3b33f5df449e

Comment 14

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv