Closed
Bug 235863
Opened 20 years ago
Closed 18 years ago
Show/Hide Bookmarks toolbar button does not indicate state
Categories
(Camino Graveyard :: Toolbars & Menus, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.5
People
(Reporter: mozilla, Assigned: froodian)
References
()
Details
(Keywords: fixed1.8.1, Whiteboard: [good first bug] [STRING CHANGES in comment 8] [needs icon])
Attachments
(5 files, 3 obsolete files)
15.41 KB,
patch
|
Details | Diff | Splinter Review | |
2.68 KB,
patch
|
Details | Diff | Splinter Review | |
2.69 KB,
patch
|
Details | Diff | Splinter Review | |
2.62 KB,
image/tiff
|
samuel.sidler+old
:
review+
mikepinkerton
:
superreview+
|
Details |
2.43 KB,
patch
|
bugzilla-graveyard
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040227 Camino/0.7+ when in bookmark editing mode, the show/hide bookmarks manager button does not indicate visually whether or not the user is in the bookmarks editor or in the webpage. for example, see how safari handles the bookmarks manager. Reproducible: Always Steps to Reproduce: 1. open a window 2. click show/hide bookmarks Actual Results: button does not change appearance Expected Results: button shows a depressed state, or perhaps open book, to denote editing bookmarks mode
Confirming as an RFE, since it's pretty obvious which state the browser is in.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: Bookmarks → Toolbars & Menus
Ever confirmed: true
Summary: show/hide bookmarks button in toolbar does not indicate state → Show/Hide Bookmarks toolbar button does not indicate state
Updated•19 years ago
|
Target Milestone: --- → Camino1.0
Icon has further problem: click to the right of the bookmark icon to the edge of the window, or to the left of the icon over to the google search input... I kept wondering why the heck I kept activating the bookmarks list when just trying to drag the window...Camino perceives the size of the icon much much larger than it is... didn't know whether to file this seperately or if it could be handled as part of this problem too. Version 2006022300 (1.2+)
Comment 4•18 years ago
|
||
That's bug 328259.
Comment 5•18 years ago
|
||
This would probably be a good first bug for anyone interested.
Assignee: mikepinkerton → nobody
QA Contact: toolbars
Whiteboard: [good first bug]
Assignee: nobody → d.elliott
Desmond is looking into this, and Jasper's working on an icon.
Status: NEW → ASSIGNED
Updated•18 years ago
|
Status: ASSIGNED → NEW
Updated•18 years ago
|
Status: NEW → ASSIGNED
Comment 7•18 years ago
|
||
Reassigned so other new coders can have a go. I lost my patch for this and have no time to find it again. Fix should be trivial manipulation of a member variable representing the state of the url about:bookmarks
Assignee: d.elliott → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 8•18 years ago
|
||
Does what it says on the package. This needs a hidemanager.tif from jasper. This has the following STRING CHANGES: - rename the existing string BookmarkMgrToolTip to ShowBookmarkMgrToolTip - under |ShowBookmarkMgrToolTip|, "HideBookmarkMgrToolTip" = "Hide all bookmarks"; - under |Manage Bookmarks|, "Hide Bookmarks" = "Hide Bookmarks";
Comment 9•18 years ago
|
||
This needs to wait for bug 346782, or things could get very confusing.
Depends on: 346782
Assignee | ||
Updated•18 years ago
|
Attachment #230767 -
Flags: review?(stuart.morgan)
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #230767 -
Attachment is obsolete: true
Attachment #234889 -
Flags: review?(stuart.morgan)
Assignee | ||
Updated•18 years ago
|
Whiteboard: [good first bug] → [good first bug] [STRING CHANGES in comment 8]
Assignee | ||
Updated•18 years ago
|
Attachment #234889 -
Flags: review?(stuart.morgan) → review?(hwaara)
Jasper, any progress with this icon?
Whiteboard: [good first bug] [STRING CHANGES in comment 8] → [good first bug] [STRING CHANGES in comment 8] [needs icon japser]
Comment 12•18 years ago
|
||
Comment on attachment 234889 [details] [diff] [review] Patch I guess code's ok, if it works with the referenced images ("manager" and "hidemanager") as QA thinks it should.
Attachment #234889 -
Flags: review?(hwaara) → review+
Comment 13•18 years ago
|
||
(In reply to comment #12) > (From update of attachment 234889 [details] [diff] [review] [edit]) > I guess code's ok, if it works with the referenced images ("manager" and > "hidemanager") as QA thinks it should. > + if ([self bookmarkManagerIsVisible]) { + [theItem setPaletteLabel:NSLocalizedString(@"Hide Bookmarks", @"Hide Bookmarks palette")]; + [theItem setToolTip:NSLocalizedString(@"HideBookmarkMgrToolTip", @"Hide all bookmarks")]; + [theItem setImage:[NSImage imageNamed:@"hidemanager"]]; + } + else { + [theItem setPaletteLabel:NSLocalizedString(@"Manage Bookmarks", @"Manage Bookmarks palette")]; + [theItem setToolTip:NSLocalizedString(@"ShowBookmarkMgrToolTip", @"Show all bookmarks")]; + [theItem setImage:[NSImage imageNamed:@"manager"]]; Shouldn't we be passing in nil as the second param of the NSLocalizedString calls? I only mention this because pink has requested it on a few other bugs.
Assignee | ||
Comment 14•18 years ago
|
||
Comment on attachment 234889 [details] [diff] [review] Patch I only put in a second argument since the code I was altering had second arguments. I'm happy to do nil if that's what you'd prefer, pink.
Attachment #234889 -
Flags: superreview?(mikepinkerton)
Comment 15•18 years ago
|
||
yeah, make the 2nd param nil. it's useless for the way we use things. are there new icons associated with this? Can someone take a screenshot?
Assignee | ||
Comment 16•18 years ago
|
||
If we're making one nil, we should make them all nil. This does just that. There need to be new icons associated with this, but there aren't. I was planning on getting a review/superreview for the code changes, then review icons separately, if and when we get them.
Attachment #234889 -
Attachment is obsolete: true
Attachment #235804 -
Flags: superreview?(mikepinkerton)
Attachment #234889 -
Flags: superreview?(mikepinkerton)
Comment 17•18 years ago
|
||
I'm working on a hidemanager.tif icon - is that the only one required?
Assignee | ||
Comment 18•18 years ago
|
||
Assuming it works visually as a toggle with our existing manager icon, yes (otherwise we'd need a new manager icon too). :)
Comment 19•18 years ago
|
||
Comment on attachment 235804 [details] [diff] [review] Removes second param from localized strings sr=pink. i'm curious to see the new icon.
Attachment #235804 -
Flags: superreview?(mikepinkerton) → superreview+
Assignee | ||
Comment 20•18 years ago
|
||
Attachment #235804 -
Attachment is obsolete: true
pink, we're going to have froodian land this (tues am, prolly) with a duplicate of the existing manager icon acting as the new one in order to ease testing new close icon. That way people only have to sub in an icon in their build/nightly rather than build with patches and strings to see things in action (and no one else will notice any difference, icon-wise, in the meantime).
Whiteboard: [good first bug] [STRING CHANGES in comment 8] [needs icon japser] → [good first bug] [STRING CHANGES in comment 8] [needs icon]
Assignee | ||
Comment 24•18 years ago
|
||
Checked in with duplicate icon on trunk and 1.8branch.
Updated•18 years ago
|
Attachment #237911 -
Attachment description: This is the bitrot police → This is the bitrot police (checked in to branch and trunk)
Updated•18 years ago
|
Attachment #237914 -
Attachment description: trunk project patch → trunk project patch (checked in to branch and trunk)
Updated•18 years ago
|
Attachment #237915 -
Attachment description: branch project patch → branch project patch (checked in to branch)
Updated•18 years ago
|
Attachment #237914 -
Attachment description: trunk project patch (checked in to branch and trunk) → trunk project patch (checked in to trunk)
Comment 25•18 years ago
|
||
Here's my first proposal for the hidemanager icon. This is my first time using Bugzilla for anything other than a few brief comments, so please let me know if there's anything else I need to do.
Comment 26•18 years ago
|
||
Comment on attachment 238193 [details]
First draft of 'hidemanager' icon
Testing this new image is easy in the latest nightly. Simply download the latest nightly, open the application bundle, and replace hidemanager.tif with this new icon.
r=me.
Mike, what do you think?
Attachment #238193 -
Flags: superreview?(mikepinkerton)
Attachment #238193 -
Flags: review+
Updated•18 years ago
|
Target Milestone: Camino1.2 → Camino1.1
Comment 27•18 years ago
|
||
Comment on attachment 238193 [details]
First draft of 'hidemanager' icon
cool! sr=pink
Attachment #238193 -
Flags: superreview?(mikepinkerton) → superreview+
Assignee | ||
Comment 28•18 years ago
|
||
Looks like I (and the reviewers) forgot the actual toolbar item label (which is what icon+text or text-only people will see most). This (with the appropriate string changes) will make it so the text will be "Manage Bookmarks" and "Hide Bookmarks". Alternately, it could be "Show Bookmarks" and "Hide Bookmarks" if people think that's more appropriate. Once this gets reviewed I'll check both it and the icon in.
Attachment #238223 -
Flags: superreview?(mikepinkerton)
Comment 29•18 years ago
|
||
Comment on attachment 238223 [details] [diff] [review] Changes toolbar item's label too r=me
Attachment #238223 -
Flags: review+
Comment 30•18 years ago
|
||
Comment on attachment 238223 [details] [diff] [review] Changes toolbar item's label too rs=pink
Attachment #238223 -
Flags: superreview?(mikepinkerton) → superreview+
Assignee | ||
Comment 31•18 years ago
|
||
Fixed on trunk and 1.8branch. Thanks for icon Jon! :D
You need to log in
before you can comment on or make changes to this bug.
Description
•