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)

PowerPC
macOS
enhancement
Not set
normal

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)

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
Target Milestone: --- → Camino1.0
Not 1.0 material. Moving to 1.2.
Target Milestone: Camino1.0 → Camino1.2
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+)
That's bug 328259.
This would probably be a good first bug for anyone interested.
Assignee: mikepinkerton → nobody
QA Contact: toolbars
Whiteboard: [good first bug]
Desmond is looking into this, and Jasper's working on an icon.
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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
Attached patch Patch (obsolete) — Splinter Review
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";
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #230767 - Flags: review?(stuart.morgan)
This needs to wait for bug 346782, or things could get very confusing.
Depends on: 346782
Attachment #230767 - Flags: review?(stuart.morgan)
Attached patch Patch (obsolete) — Splinter Review
Attachment #230767 - Attachment is obsolete: true
Attachment #234889 - Flags: review?(stuart.morgan)
Whiteboard: [good first bug] → [good first bug] [STRING CHANGES in comment 8]
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 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+
(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.
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)
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?
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)
I'm working on a hidemanager.tif icon - is that the only one required?
Assuming it works visually as a toggle with our existing manager icon, yes (otherwise we'd need a new manager icon too). :)
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+
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]
Checked in with duplicate icon on trunk and 1.8branch.
Attachment #237911 - Attachment description: This is the bitrot police → This is the bitrot police (checked in to branch and trunk)
Attachment #237914 - Attachment description: trunk project patch → trunk project patch (checked in to branch and trunk)
Attachment #237915 - Attachment description: branch project patch → branch project patch (checked in to branch)
Attachment #237914 - Attachment description: trunk project patch (checked in to branch and trunk) → trunk project patch (checked in to trunk)
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 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+
Target Milestone: Camino1.2 → Camino1.1
Comment on attachment 238193 [details]
First draft of 'hidemanager' icon

cool! sr=pink
Attachment #238193 - Flags: superreview?(mikepinkerton) → superreview+
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 on attachment 238223 [details] [diff] [review]
Changes toolbar item's label too

r=me
Attachment #238223 - Flags: review+
Comment on attachment 238223 [details] [diff] [review]
Changes toolbar item's label too

rs=pink
Attachment #238223 - Flags: superreview?(mikepinkerton) → superreview+
Fixed on trunk and 1.8branch.  Thanks for icon Jon! :D
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: