Separator sometimes is sometimes displayed as generic bookmark in bookmarks bar

RESOLVED FIXED

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: froodian (Ian Leue), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1.1})

Details

Attachments

(1 attachment, 2 obsolete attachments)

1.02 KB, patch
Stuart Morgan
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
STR:

1. Cmd-B
2. Select Bookmarks bar collection, make sure no bookmark items are selected
3. + menu, Add Separator
4. Click on a bookmark item other than the separator
5. Click on the separator

What happens: Separator is now displayed as a generic bookmark favicon in the bm bar.
What should happen: Should still look like a separator
(Assignee)

Comment 1

11 years ago
Created attachment 248857 [details] [diff] [review]
Patch

The problem here was that we were setting the icon and title in |bookmarkChanged:|.  This solution could be considered a little excessive (a simple "if ([mItem isSeparator]) return NO;" at the beginning of the method would suffice), but I like it because it encapsulates the separator display logic.
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #248857 - Flags: review?(bugzilla)
(Assignee)

Comment 2

11 years ago
Created attachment 248858 [details] [diff] [review]
Patch

Oops, forgot the declare.
Attachment #248857 - Attachment is obsolete: true
Attachment #248858 - Flags: review?(bugzilla)
Attachment #248857 - Flags: review?(bugzilla)

Comment 3

11 years ago
Comment on attachment 248858 [details] [diff] [review]
Patch

Yeah, that looks good to me.
Attachment #248858 - Flags: superreview?(stuart.morgan)
Attachment #248858 - Flags: review?(bugzilla)
Attachment #248858 - Flags: review+

Comment 4

11 years ago
Comment on attachment 248858 [details] [diff] [review]
Patch

I don't like displayAsSeparator being a hybrid of something you call to make something look like a separator and something that tells you if a bookmark is a separator. The solution you mention in comment 1 sounds cleaner.
Attachment #248858 - Flags: superreview?(stuart.morgan) → superreview-
(Assignee)

Comment 5

11 years ago
Created attachment 248870 [details] [diff] [review]
Patch

A slightly more reasonable solution. ;)
Attachment #248858 - Attachment is obsolete: true
Attachment #248870 - Flags: superreview?(stuart.morgan)

Comment 6

11 years ago
Comment on attachment 248870 [details] [diff] [review]
Patch

sr=smorgan
Attachment #248870 - Flags: superreview?(stuart.morgan) → superreview+
(Assignee)

Comment 7

11 years ago
Checked in on 1.8branch and trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.