266356 - [patch] "Add Separator" button shoudn't be enabled in history view.

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[ender.mb]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041024 Camino/0.8+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041024 Camino/0.8+

Just stumbled onto this bug... apparently, the "add separator" button in the
bookmarks manager is enabled when the manager is closed, it will remain enabled
when the manager is next opened--even if it opens to something other than the
"bookmarks menu" pane. 

Reproducible: Always
Steps to Reproduce:
1. click Bookmarks > Show All Bookmarks (cmd+b), or the bookmarks toolbar item.
2. click on "Bookmarks Menu" in the "Items" column.
3. click Bookmarks > Hide All Bookmarks (cmd+b), or the bookmarks toolbar item.
4. click History > Show History (cmd+y).

Actual Results:  
"Add Separator" button is enabled. Clicking it causes separators to be added to
the end of the bookmarks menu.

Expected Results:  
"Add Separator" button should be disabled.

Comment 1

[Jasper]

Confirmed, shuoldn't be possible.

Comment 2

[Bruce Davidson]

Happy to take this one, having done two other UI enable/disable bugs over the
weekend.

Comment 3

[Bruce Davidson]

Attachment: Proposed patch

This patch disables the "add separator" button in the bookmarks view if the
history pane is the currently active pane. Seeking r=

Comment 4

[Ludovic Hirlimann [:Usul]]

Comment on attachment 163739 [details] [diff] [review]
Proposed patch

R+ works

Comment 5

[Ludovic Hirlimann [:Usul]]

Targeting for 0.9, thinking about including this one in 0.8.2

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 163739 [details] [diff] [review]
Proposed patch

sr=pink

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 163739 [details] [diff] [review]
Proposed patch

actually i'd like a better fix for this.

if the separator button is only enabled for bookmarks, then just call the
|-setEnabled:| once. pull the 

[mAddSeparatorButton setEnabled:(kBookmarkMenuContainerIndex == inRowIndex)];

out of the if statement and always call it.

Comment 8

[Bruce Davidson]

Attachment: Updated patch addressing Mike's sr comment

Updated patch that addresses Mike's comment. Tested and it works as expected,
which at least solves the query I had about what that line was doing! Again, if
reviewers are happy please transfer r=, and pinkerton submitting for sr= again.

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 164339 [details] [diff] [review]
Updated patch addressing Mike's sr comment

sr=pink, will land myself.

Comment 10

[Mike Pinkerton (not reading bugmail)]

landed.