211538 - Cmd-B should hide bookmarks manager if it's visible (and history too)

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[S Woodside]

Cmd-B hides the bookmarks manager if it's already visible, and Cmd-Y hides the
manager if the History is visible.

Comment 1

[S Woodside]

Attachment: patch

* added - (int) selectedContainer method to BookmarksController
* changed manageBookmarks and manageHistory in BrowserWindowController

Comment 2

[Chris Casciano]

dupe of 203493?

(or the other way around given things like patches)

Comment 3

[Mike Pinkerton (not reading bugmail)]

i think i'm in agreement with this. the problem is i'm not sure how to extend
this as we move to more than 1 bookmark container (which dhaas is close to
having ready. Soon there will be more than just bookmarks and history and i
don't think this solution quite scales to that.

it is something i agree we need to solve. i only didn't do it because again i'm
not sure even if the manager is gonna stay in this window.

Comment 4

[S Woodside]

When you have multiple bookmarks containers, would Cmd-B take you back to the
main one?

Comment 5

[Mike Pinkerton (not reading bugmail)]

that's the question, and the danger. the whole point of this bug is to allow a
cmd-key to quickly hide the bm manager. once you have it press it some unknown
number of times to make it go away (sometimes once, sometimes twice), it becomes
a crap shoot and the user learns this wonky behavior of pressing it repeatedly
and sometimes toggling too many times and cursing camino.

note that safari disables cmd-opt-b entirely when the manager is enabled. i'm
not sure there is a key equiv to toggle it (besides maybe cmd-[ for back)

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 126981 [details] [diff] [review]
patch

We have a new Camino request flag which can be set multiple times for a patch.
Moving old review requests to the new flag. Sorry for the spam.

Comment 7

[Ludovic Hirlimann [:Usul]]

David's bookmarks patch has landed. Should we close this one or is cmd-B still
supose to hide the bmmanager ?(pink)

Comment 8

[louis bennett]

this bug's description, as stated, has been fixed. command-b does not hide the
bookmarks manager if it's visible.

re: comment 5, safari (v85.5) now lets you toggle the manager with command-option-b.

re: comment 7, if command-b should hide the bookmark manager, that'd be bug 203493.

Comment 9

[S Woodside]

Attachment: enter key

Uses the enter key instead of the return key.

Comment 10

[S Woodside]

Comment on attachment 135592 [details] [diff] [review]
enter key

Wrong bug, sorry.

Comment 11

[S Woodside]

fixing title

Comment 12

[S Woodside]

Attachment: updated patch

updated patch, to work with david's new bookmark manager.

Comment 13

[louis bennett]

*** Bug 203493 has been marked as a duplicate of this bug. ***

Comment 14

[Ludovic Hirlimann [:Usul]]

Comment on attachment 135607 [details] [diff] [review]
updated patch

> -(IBAction)manageHistory: (id)aSender
> {
>-  if ( ![mContentView isBookmarkManagerVisible] )
>+  if ( [mContentView isBookmarkManagerVisible] == NO
>+       || [mBookmarkViewController selectedContainer] == kHistoryContainerIndex ) {
>     [self toggleBookmarkManager: self];
>-
>-  [mBookmarkViewController selectContainer:kHistoryContainerIndex];
>+  }
>+  if( [mContentView isBookmarkManagerVisible] )
>+    [mBookmarkViewController selectContainer:kHistoryContainerIndex];
> }

This is unecessary. When viewing History, pressing cmd-B pops up the bookmark
manager and does not Hide the history manager.
cmd-Y does not hide historyn when history is shown.

Also it would be great if the menu title would change to Hide bookmark when
bookmark are enabled so that the shortcut reflects something in the menu.

I do think bookmark and history should be 2 different bugs.

Comment 15

[S Woodside]

> This is unecessary. When viewing History, pressing cmd-B pops up the bookmark
> manager and does not Hide the history manager.
> cmd-Y does not hide historyn when history is shown.

I don't understand.

What I implemented does this:
- when you are in bm manager, viewing history collection, Cmd-Y closes the bm
manager. Cmd-B switches to "Menu Bookmarks" collection.
- when you are in BM Manager, viewing ANY OTHER collection, Cmd-Y switches to
the "History" collection. Cmd-B closes the BM manager.
That's the intended behaviour.

> Also it would be great if the menu title would change to Hide bookmark when
> bookmark are enabled so that the shortcut reflects something in the menu.

File another bug :-)

> I do think bookmark and history should be 2 different bugs.

No, I don't think so. In the code, the "Bookmarks manager" is also the history
manager. That's the way the code is designed. I don't think trying to change
that is either wise or necesary... so for now anyway, "History Manager" is
programmatically, merely a "collection" in the "Bookmarks Manager".

Comment 16

[Ludovic Hirlimann [:Usul]]

Comment on attachment 135607 [details] [diff] [review]
updated patch

Sorry simon, I missed something yesterday When I wrote my review; I've just
cleanly reapplyied the patch and does what you describe and what I think is the
correct behaviour.
From the summary I was dumbly trying to switch from History to normal with
cmd-B.

Comment 17

[David Haas (gone, not reading mail)]

Ludo is right - the menu should change based on what's visible.  It might be
tricky.  Have fun ;)

Also - rather than add a "-(int) selectedContainer" method, use the
already-existing "-(BookmarkFolder *)activeCollection" method & check to see if
what's returned is the history folder.

Comment 18

[Mike Pinkerton (not reading bugmail)]

fixed not using above patch.

Comment 19

[waverider]

>comment #18
Do you forget to add some lines to Locarizable.strings?

"Hide All Bookmarks" = "Hide All Bookmarks";
"Show All Bookmarks" = "Show All Bookmarks";
"Manage Bookmarks" = "Manage Bookmarks";
"Bookmarks" = "Bookmarks";

Comment 20

[Mike Pinkerton (not reading bugmail)]

i was under the impression that a localization could add it themselves. It uses
the key you give it if the string isn't in a l10n string file.

is that incorrect?

Comment 21

[waverider]

That is correct, but these lines are necessary to localize.

for example, French localizer change localizable.strings:
"Hide All Bookmarks" = "Hide All Bookmarks";
to
"Hide All Bookmarks" = "cachez tout signet";
and copy it French.lproj folder.

Comment 22

[Mike Pinkerton (not reading bugmail)]

but i thought the french localization could just add that line to their version
of that file, it doesn't need to be in the english version, or am I mistaken on
how NSLocalizedString works?

Comment 23

[Ludovic Hirlimann [:Usul]]

mike this means the french guy has to read the code in order to find what
strings to localize. It's faster and easier for the localisers to have all the
strings in the strings.plist and translkate them there. hence they do not need
to read all the code.
The pitty for use is that the strings.plist is not diffeable.

Comment 24

[Mike Pinkerton (not reading bugmail)]

makes sense. added:

"Hide All Bookmarks" = "Hide All Bookmarks";
"Show All Bookmarks" = "Show All Bookmarks";

to the localized plist. i think that's all we need for this case. removed the
sidebar strings since they're now unused. thanks for the reminder!

Comment 25

[S Woodside]

check out the genstrings command line tool, it might be useful.

Also "AppleGlot" although I think it's more a tool for using later for
generating localized versions (as opposed to now, when we are internationalizing).