Show/Hide Bookmarks toolbar button does not indicate state

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
--
enhancement
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Tristan O'Tierney, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

(Whiteboard: [good first bug] [STRING CHANGES in comment 8] [needs icon], URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
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

Comment 3

11 years ago
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

11 years ago
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]
Assignee: nobody → d.elliott
Desmond is looking into this, and Jasper's working on an icon.
Status: NEW → ASSIGNED

Updated

11 years ago
Status: ASSIGNED → NEW

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 7

11 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

11 years ago
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";
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #230767 - Flags: review?(stuart.morgan)

Comment 9

11 years ago
This needs to wait for bug 346782, or things could get very confusing.
Depends on: 346782
(Assignee)

Updated

11 years ago
Attachment #230767 - Flags: review?(stuart.morgan)
(Assignee)

Comment 10

11 years ago
Created attachment 234889 [details] [diff] [review]
Patch
Attachment #230767 - Attachment is obsolete: true
Attachment #234889 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Whiteboard: [good first bug] → [good first bug] [STRING CHANGES in comment 8]
(Assignee)

Updated

11 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

11 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

11 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

11 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)
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

11 years ago
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.
Attachment #234889 - Attachment is obsolete: true
Attachment #235804 - Flags: superreview?(mikepinkerton)
Attachment #234889 - Flags: superreview?(mikepinkerton)

Comment 17

11 years ago
I'm working on a hidemanager.tif icon - is that the only one required?
(Assignee)

Comment 18

11 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 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

11 years ago
Created attachment 237911 [details] [diff] [review]
This is the bitrot police (checked in to branch and trunk)
Attachment #235804 - Attachment is obsolete: true
Created attachment 237914 [details] [diff] [review]
trunk project patch (checked in to trunk)
Created attachment 237915 [details] [diff] [review]
branch project patch (checked in to branch)
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

11 years ago
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)

Comment 25

11 years ago
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 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+
(Assignee)

Comment 28

11 years ago
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.
Attachment #238223 - Flags: superreview?(mikepinkerton)

Comment 29

11 years ago
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+
(Assignee)

Comment 31

11 years ago
Fixed on trunk and 1.8branch.  Thanks for icon Jon! :D
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.