Last Comment Bug 235863 - Show/Hide Bookmarks toolbar button does not indicate state
: Show/Hide Bookmarks toolbar button does not indicate state
Status: RESOLVED FIXED
[good first bug] [STRING CHANGES in c...
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
http://otierney.net/
Depends on: 346782
Blocks:
  Show dependency treegraph
 
Reported: 2004-02-27 12:27 PST by Tristan O'Tierney
Modified: 2006-09-14 10:29 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (2.92 KB, patch)
2006-07-26 11:21 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (2.91 KB, patch)
2006-08-21 18:39 PDT, froodian (Ian Leue)
hwaara: review+
Details | Diff | Splinter Review
Removes second param from localized strings (15.56 KB, patch)
2006-08-28 15:10 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review
This is the bitrot police (checked in to branch and trunk) (15.41 KB, patch)
2006-09-11 22:33 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
trunk project patch (checked in to trunk) (2.68 KB, patch)
2006-09-11 22:48 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details | Diff | Splinter Review
branch project patch (checked in to branch) (2.69 KB, patch)
2006-09-11 22:48 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details | Diff | Splinter Review
First draft of 'hidemanager' icon (2.62 KB, image/tiff)
2006-09-13 04:32 PDT, Jon Hicks
samuel.sidler+old: review+
mikepinkerton: superreview+
Details
Changes toolbar item's label too (2.43 KB, patch)
2006-09-13 09:41 PDT, froodian (Ian Leue)
bugzilla-graveyard: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Tristan O'Tierney 2004-02-27 12:27:09 PST
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
Comment 1 User image Greg K. 2004-02-28 03:10:33 PST
Confirming as an RFE, since it's pretty obvious which state the browser is in.
Comment 2 User image Samuel Sidler (old account; do not CC) 2005-07-30 00:23:28 PDT
Not 1.0 material. Moving to 1.2.
Comment 3 User image Mark E. 2006-02-24 07:43:55 PST
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 User image Stuart Morgan 2006-02-24 08:42:33 PST
That's bug 328259.
Comment 5 User image Samuel Sidler (old account; do not CC) 2006-04-29 23:28:34 PDT
This would probably be a good first bug for anyone interested.
Comment 6 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-30 02:23:53 PDT
Desmond is looking into this, and Jasper's working on an icon.
Comment 7 User image Desmond Elliott 2006-07-21 06:42:35 PDT
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
Comment 8 User image froodian (Ian Leue) 2006-07-26 11:21:51 PDT
Created attachment 230767 [details] [diff] [review]
Patch

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 User image Stuart Morgan 2006-07-31 20:07:45 PDT
This needs to wait for bug 346782, or things could get very confusing.
Comment 10 User image froodian (Ian Leue) 2006-08-21 18:39:23 PDT
Created attachment 234889 [details] [diff] [review]
Patch
Comment 11 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-28 10:19:58 PDT
Jasper, any progress with this icon?
Comment 12 User image Håkan Waara 2006-08-28 11:58:49 PDT
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.
Comment 13 User image Nick Kreeger 2006-08-28 12:34:32 PDT
(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 14 User image froodian (Ian Leue) 2006-08-28 13:31:07 PDT
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.
Comment 15 User image Mike Pinkerton (not reading bugmail) 2006-08-28 14:32:46 PDT
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?
Comment 16 User image froodian (Ian Leue) 2006-08-28 15:10:32 PDT
Created attachment 235804 [details] [diff] [review]
Removes second param from localized strings

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.
Comment 17 User image Jon Hicks 2006-09-11 02:01:12 PDT
I'm working on a hidemanager.tif icon - is that the only one required?
Comment 18 User image froodian (Ian Leue) 2006-09-11 05:48:53 PDT
Assuming it works visually as a toggle with our existing manager icon, yes (otherwise we'd need a new manager icon too). :)
Comment 19 User image Mike Pinkerton (not reading bugmail) 2006-09-11 06:42:00 PDT
Comment on attachment 235804 [details] [diff] [review]
Removes second param from localized strings

sr=pink. i'm curious to see the new icon.
Comment 20 User image froodian (Ian Leue) 2006-09-11 22:33:17 PDT
Created attachment 237911 [details] [diff] [review]
This is the bitrot police (checked in to branch and trunk)
Comment 21 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-11 22:48:02 PDT
Created attachment 237914 [details] [diff] [review]
trunk project patch (checked in to trunk)
Comment 22 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-11 22:48:32 PDT
Created attachment 237915 [details] [diff] [review]
branch project patch (checked in to branch)
Comment 23 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-11 23:14:02 PDT
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).
Comment 24 User image froodian (Ian Leue) 2006-09-12 11:59:27 PDT
Checked in with duplicate icon on trunk and 1.8branch.
Comment 25 User image Jon Hicks 2006-09-13 04:32:59 PDT
Created attachment 238193 [details]
First draft of 'hidemanager' icon

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 User image Samuel Sidler (old account; do not CC) 2006-09-13 08:12:48 PDT
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?
Comment 27 User image Mike Pinkerton (not reading bugmail) 2006-09-13 09:24:24 PDT
Comment on attachment 238193 [details]
First draft of 'hidemanager' icon

cool! sr=pink
Comment 28 User image froodian (Ian Leue) 2006-09-13 09:41:43 PDT
Created attachment 238223 [details] [diff] [review]
Changes toolbar item's label too

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.
Comment 29 User image Chris Lawson (gone) 2006-09-13 20:06:07 PDT
Comment on attachment 238223 [details] [diff] [review]
Changes toolbar item's label too

r=me
Comment 30 User image Mike Pinkerton (not reading bugmail) 2006-09-14 10:12:00 PDT
Comment on attachment 238223 [details] [diff] [review]
Changes toolbar item's label too

rs=pink
Comment 31 User image froodian (Ian Leue) 2006-09-14 10:29:09 PDT
Fixed on trunk and 1.8branch.  Thanks for icon Jon! :D

Note You need to log in before you can comment on or make changes to this bug.