275170 - Change "Go" menu to "History"

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement, P3)

Description

[Jasper]

go -> history and "reorganize as need be

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)
> reorganize as need be

Like bug 271943?

Comment 2

[Jasper]

This is defenitly something we should do for 0.9. It's an easy thing to do and
I'll make the nib changes. I might reorder some stuff aswell.

Comment 3

[Simon Fraser [no longer active]]

Need to decide by 1.0.

Comment 4

[Samuel Sidler (old account; do not CC)]

Clearing 0.9? request flag...

We *need* to revisit this now and decide.

Comment 5

[Simon Fraser [no longer active]]

So where do we put the "Home" and "Search the web" items? And Back/Forward?

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> So where do we put the "Home" and "Search the web" items? And Back/Forward?

Home, Back, Forward can stay where they are (that's where they are in Safari's History menu).  The latter two are certainly history functions.

Search the web either focuses the Search field or calls the Search sheet, so it's not "go"ing anywhere to begin with.  It can go in Edit with Find IMO.  (Safari puts it there, albeit hidden because of the stupid Find submenu idea.)

And Show History should then move to the new History menu (bug 271943).

The "big" issue, IMO, is we periodically get a lot of comments in the forum from new people trying Camino--I don't know what you hear at caminofeedback--that Camino doesn't have a History menu or feature, and simply renaming the menu would fix that easily, even if we don't also move "Show History".

IMO neither "Go" nor "History" creates a perfect metaphor for all the items we'd want in the menu, but "History" is a more clear, recognizable name for the main purpose of that menu.

Comment 7

[Håkan Waara]

Maybe it's time to do this change before 1.0?

So to wrap up comment 6:

* Change name from "Go" to "History"
* Move "Search the Web" to the "Edit" menu along with "Find", like in Safari.
* Move "Show History" (now in View) to the new History menu.

Comment 8

[Simon Fraser [no longer active]]

What about Home, and Local Network Services?

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Home stays put in the new History menu.

Home
Back
Forward
-------
Show History
-------
global history

As for the Local Network Services stuff, I don't have a good answer.  But they're a collection in the Bookmarks Manager, so moving them into the Bookmarks menu might make sense (like how Safari shows the Bookmarks Toolbar collection in its Bookmarks menu--although showing that particular collection makes little sense there, IMO).

Comment 10

[Mike Pinkerton (not reading bugmail)]

we need more time to come to a consensus. not for 1.0. it's really not the end of the world.

Comment 11

[Håkan Waara]

The new UI for Firefox 2 will change "Go" to "History", as posted on dev.apps.firefox.   Anyone against just getting this done at the same time?

Comment 12

[Jasper]

Well if it where up to me I would have changed this years ago. Come on let's just do this and get it over with.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

What actually appears in the "Local Network Services" submenu, besides the "About Local Network Services" and the extra menu separator (bug 321937)?  Knowing what appears there would help us better understand where the submenu should go and thereby come up with a solution that will make Simon happy and let us resolve this bug and bug 271943 :)

Comment 14

[Simon Fraser [no longer active]]

(In reply to comment #13)
> What actually appears in the "Local Network Services" submenu, besides the
> "About Local Network Services" and the extra menu separator (bug 321937)?

http, https and ftp Bonjour items. Turn on FTP on your machine, and you'll see one.

Comment 15

[Wevah (Nate Weaver)]

Should that menu item's title be changed to "Bonjour" to match the BM manager?

Comment 16

[froodian (Ian Leue)]

Attachment: Patch

This patch makes the following changes (nib-only changes will be enumerated with the forthcoming nib):

- Changes text of the "Get Info" menu item to "Page Info" or "Bookmark Info", depending on whether the bm manager is showing.
- Load view source in fore/background based on a shift alternate
- Puts in the wiring for Cmd-option-K ("Add Current Tabs as Tab Group…"), basically by bringing up the dialog as normal and flipping the "tab group" checkbox
- Adds support for dialogless bookmarking (for individual bookmarks and tab groups)
- Keeps Cmd-D as a legacy keyboard shortcut for Add Bookmark
- Adds link to "support lite"

It requires the following STRING CHANGES (in pseudo-diff).  BlankBookmarkInfoTitle was unused, iirc.:
- "Get Info" = "Get Info";
+ "Page Info" = "Page Info";
- "BlankBookmarkInfoTitle" = "Bookmark Info";
+ "Bookmark Info" = "Bookmark Info";

And the following change to WebsiteDefaults.strings:
+ SupportLitePageDefault = "http://www.mozilla.org/projects/camino/support.html";

Comment 17

[froodian (Ian Leue)]

Attachment: new MainMenu.nib

This nib makes the following changes:

- Moves Camino Feedback to Camino menu
- Moves Search The Web to File menu
- Moves Page Info/Bookmark Info to View Menu
- Adds the alternate menu item for View Source (to load in background)
- Changes Go menu to History
- Moves Show History to History menu
- Adds alternate for "Add Page to Bookmarks" (dialogless)
- Adds menu and matching alternate for "Add Current Tabs as Tab Group…" and "Add Current Tabs as Tab Group"
- Moves "Local Network Services" to Bookmarks menu, and renames it to "Bonjour"
- Renames "About Local Network Services" to "About Bonjour"
- Creates new Support Lite menu item, named "Camino Support"

Comment 18

[Stuart Morgan]

Comment on attachment 240761 [details]
new MainMenu.nib

Conceptual nib issues:
-"Search the Web" should be "Search the Web…"
-I don't like the lack of parity between "Add Page to Bookmarks" and "Add Current Tabs as Tab Group". Either both should be "Current" or neither (probably neither), and either both should be "to Bookmarks" or neither. Maybe "Add Tab Group to Bookmarks"

Comment 19

[Stuart Morgan]

Comment on attachment 240760 [details] [diff] [review]
Patch

First pass review; I'll actually test with a later version.

>+- (IBAction)forceAddBookmark:(id)aSender;

Nothing is being forced here. Call it addBookmarkWithoutPrompt:

>+  BOOL loadInBackground = NO;
>+  if ([[aSender keyEquivalent] isEqual:@"V"]) // If it's a capital V, shift is down
>+    loadInBackground = YES;

BOOL loadInBackground = ([[aSender keyEquivalent] isEqual:@"V"])

Does this work for mouse+shift as well?

>+// Create a bookmark or tab group without an intermediate dialog
...

If these are two completely different menu items that do different things, they should have two different action methods. You can use a shared implementation method if it's convenient (but since most of the code is different I don't think that will be necessary).

When you break this appart, make sure to get rid of |newItem| and anything else that's unused cruft left over from the code this is based on.

> - (IBAction)toggleTabGroup:(id)sender
> {
>+  // if we've been called programmatically, flip the checkbox
>+  if (sender == nil)
>+    [mTabGroupCheckbox setState:![mTabGroupCheckbox state]];

This is ugly, since it makes the method sometimes triggered by a checkbox change and sometimes causing a checkbox change (and you don't actually want to toggle the tab group state at the programatic call site anyway; you want to set it to on, which isn't the same thing). Refactor the title-setting logic into a new method (updateTitle:(BOOL)isTabGroup or something) that the IB action is a thin wrapper around based on the checkbox state, then make a programatic method that takes a BOOL, sets the checkbox accordingly, and calls the same helper method.

>+    IBOutlet NSMenuItem*    mForceAddBookmarkMenuItem;
>+    IBOutlet NSMenuItem*    mForceAddTabGroupMenuItem;

Force<foo>MenuItem -> <foo>WithoutPromptMenuItem

> -(IBAction) supportLink:(id)aSender;
...
>+-(IBAction) supportLiteLink:(id)aSender;

This is not going to fly. The actual difference between what these two things are need to be clear in reasonable naming, and if necessary an explanatory comment.  But frankly, I don't understand how we expect users to to pick between "Camino Help" and "Camino Support" either. I think the "Camino Help" page should help users get whatever level of help they need, rather than making people look in two places.

We need to punt this change from the big ball of changes, and have more discussion in bug 347604. It's not really linked to any of the other changes (feedback should move regardless, since it's definitely not supposed to be help) so we lose nothing by doing so (and it's tied in with things like creating the target page).

>+  [mAddBookmarkMenuItem              takeStateFromItem:[mBookmarksHelperMenu itemWithTitle:[mAddBookmarkMenuItem title]]];
>+  [mForceAddBookmarkMenuItem         takeStateFromItem:[mBookmarksHelperMenu itemWithTitle:[mForceAddBookmarkMenuItem title]]];
>+  [mAddTabGroupMenuItem              takeStateFromItem:[mBookmarksHelperMenu itemWithTitle:[mAddTabGroupMenuItem title]]];
>+  [mForceAddTabGroupMenuItem         takeStateFromItem:[mBookmarksHelperMenu itemWithTitle:[mForceAddTabGroupMenuItem title]]];
>+
>+  [mCreateBookmarksFolderMenuItem    takeStateFromItem:[mBookmarksHelperMenu itemWithTarget:[mCreateBookmarksFolderMenuItem target]
>+                                                                                  andAction:[mCreateBookmarksFolderMenuItem action]]];
>+  [mCreateBookmarksSeparatorMenuItem takeStateFromItem:[mBookmarksHelperMenu itemWithTarget:[mCreateBookmarksSeparatorMenuItem target]
>+                                                                                  andAction:[mCreateBookmarksSeparatorMenuItem action]]];
>+  [mShowAllBookmarksMenuItem         takeStateFromItem:[mBookmarksHelperMenu itemWithTarget:[mShowAllBookmarksMenuItem target]
>+                                                                                  andAction:[mShowAllBookmarksMenuItem action]]];

Get rid of the extra spacing here

Comment 20

[froodian (Ian Leue)]

Attachment: Patch

(In reply to comment #19)
> (From update of attachment 240760 [details] [diff] [review] [edit])
> >+  BOOL loadInBackground = NO;
> >+  if ([[aSender keyEquivalent] isEqual:@"V"]) // If it's a capital V, shift is down
> >+    loadInBackground = YES;
> 
> BOOL loadInBackground = ([[aSender keyEquivalent] isEqual:@"V"])
> 
> Does this work for mouse+shift as well?

Yes.

Addresses all other comments.

Comment 21

[froodian (Ian Leue)]

Attachment: new MainMenu.nib

As requested, the "Support Lite" has been spun back off into bug 347604.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #18)
> -I don't like the lack of parity between "Add Page to Bookmarks" and "Add
> Current Tabs as Tab Group". Either both should be "Current" or neither
> (probably neither), and either both should be "to Bookmarks" or neither. Maybe
> "Add Tab Group to Bookmarks" 

We spent *a lot* of time working on the language for these options, and the fundamental problem is that you can't keep absolute parallelism without losing comprehensibility (i.e., you devolve into jargon) or without having obnoxiously long menu item labels.

I don't mind adding "Current" to the singleton, but the existing proposal for the tab group label is very clear and descriptive of the action we're going to perform (and where else would one think one's going to add a tab group using a command in the bookmarks menu, history?), so I'm very reluctant to change that item's label.

Comment 23

[Stuart Morgan]

I thought I commented again when I noticed that the shortcut is changing to Command-K, but I guess not.  Given that, there's not necessarily a need to keep the word "Add", which means we could consider "Bookmark page" and "Bookmark Tabs as Tab Group" (or both with Current). That's about the same length, but with  parallel structure and no extra jargon.

Comment 24

[Stuart Morgan]

Comment on attachment 240899 [details] [diff] [review]
Patch

>+  BOOL loadInBackground = NO;
>+  if ([[aSender keyEquivalent] isEqual:@"V"]) // If it's a capital V, shift is down
>+    loadInBackground = YES;

This still needs to be collapsed, and it should be isEqualToString:, so:

BOOL loadInBackground = ([[aSender keyEquivalent] isEqualToString:@"V"])

>+  if (!parentFolder)
>+    parentFolder = [bookmarkManager toolbarFolder];
...
>+  if (!parentFolder)
>+    parentFolder = [bookmarkManager toolbarFolder];

Rather than ending up with this default in three places (here plus one in existing bookmark code), go ahead and make lastUsedBookmarkFolder return toolbarFolder as a default itself, so we only have to change it in one place if we change our minds later.

>+  [mTitleField setStringValue:[self updateTitle:isTabGroup]];
...
>+  [mTitleField setStringValue:[self updateTitle:isTabGroup]];

>+- (NSString *)updateTitle:(BOOL)isTabGroup
>+{
>   NSString* defaultGroupTitle = [NSString stringWithFormat:NSLocalizedString(@"defaultTabGroupTitle", @"[%d tabs] %@"), [mBookmarkItems count], mDefaultTitle];
> 
>-  // if title is unedited and we're bookmarking a tab group, prepend a prefix to indicate this
>+  // If the title is unedited, update to the default name
>   if ([[mTitleField stringValue] isEqualToString:mDefaultTitle] ||
>-      [[mTitleField stringValue] isEqualToString:defaultGroupTitle])
>-  {
>-    [mTitleField setStringValue:([mTabGroupCheckbox state] == NSOnState) ? defaultGroupTitle : mDefaultTitle];
>+      [[mTitleField stringValue] isEqualToString:defaultGroupTitle]) {
>+    if (isTabGroup)
>+      return defaultGroupTitle;
>+
>+    return mDefaultTitle;
>   }
>+  // If the title has been edited, leave it
>+  return [mTitleField stringValue];
> }

updateTitle: should do what it says, not return a string.  So the call sites should be:
  [self updateTitle:isTabGroup];
and the function should be:
...
>   if ([[mTitleField stringValue] isEqualToString:mDefaultTitle] ||
>       [[mTitleField stringValue] isEqualToString:defaultGroupTitle])
>+    [mTitleField setStringValue:(isTabGroup ? defaultGroupTitle : mDefaultTitle]);
> }

>It requires the following STRING CHANGES (in pseudo-diff). 
>BlankBookmarkInfoTitle was unused, iirc.:
>- "Get Info" = "Get Info";
>+ "Page Info" = "Page Info";
>- "BlankBookmarkInfoTitle" = "Bookmark Info";
>+ "Bookmark Info" = "Bookmark Info";

Both of these are used in existing code (one in BookmarkManager, one in BookmarkInfoController), and I don't see any changes to the code where they are used.

Comment 25

[froodian (Ian Leue)]

Attachment: Patch

(In reply to comment #24)
> (From update of attachment 240899 [details] [diff] [review] [edit])
> >It requires the following STRING CHANGES (in pseudo-diff). 
> >BlankBookmarkInfoTitle was unused, iirc.:
> >- "Get Info" = "Get Info";
> >+ "Page Info" = "Page Info";
> >- "BlankBookmarkInfoTitle" = "Bookmark Info";
> >+ "Bookmark Info" = "Bookmark Info";
> 
> Both of these are used in existing code (one in BookmarkManager, one in
> BookmarkInfoController), and I don't see any changes to the code where they are
> used.
> 

Ah, I'd made the BookmarkInfoController change, but forgotten to diff (hence why it didn't come up in a cmd-shift-F for me).  Forgot Get Info.  Both fixed.

Addresses all comments.

Comment 26

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 240900 [details]
new MainMenu.nib

r=ardissone with "Bookmark Current Page" and "Bookmark
Current Tabs as Tab Group" and the elipsis syncing for Searching that smorgan wanted.

Comment 27

[froodian (Ian Leue)]

Attachment: r=ardissone MainMenu.nib

Adds the ellipsis to "Search the Web" and makes the bookmark items "Bookmark Current Page" and "Bookmark Current Tabs as Tab Group"

Comment 28

[Stuart Morgan]

Comment on attachment 240948 [details] [diff] [review]
Patch

r=me, with two nits (both my fault for spacing when I made suggestions above; sorry about that):

>+  BOOL loadInBackground = ([[aSender keyEquivalent] isEqualToString:@"V"]);

No parens.

>   if ([[mTitleField stringValue] isEqualToString:mDefaultTitle] ||
>       [[mTitleField stringValue] isEqualToString:defaultGroupTitle])
>-  {
>-    [mTitleField setStringValue:([mTabGroupCheckbox state] == NSOnState) ? defaultGroupTitle : mDefaultTitle];
>-  }
>+    [mTitleField setStringValue:(isTabGroup ? defaultGroupTitle : mDefaultTitle)];

The { } should stay (as they were), since it's a multi-line if.


Someone else should put this all through its paces, since a fair number of things are being touched.

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 240948 [details] [diff] [review]
Patch

(In reply to comment #28)
> Someone else should put this all through its paces, since a fair number of
> things are being touched.

I checked all of the changed items when I reviewed the nib.

Comment 30

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #16)

> It requires the following STRING CHANGES (in pseudo-diff). 
> BlankBookmarkInfoTitle was unused, iirc.:

Actually, this bothered me a little bit, so I did more digging; the net effect now is nothing because of your corresponding code change, but the usage of the two strings is not identical, even if their content is (now).  If we ever change the CM item or menu title (say, back to "Get Info" because someone objects vehemently after the fact), then all the Info windows for items we don't really get info for (bug 319516, history items, collections, etc.) will suddenly be named "Get Info" instead of the generic "Bookmark Info".  So you want to keep that string and revert the part of the patch that touches it (BookmarkInfoController.mm only, I think).  

Also, please annotate the new Bookmark Info string so we know it's being used  2 places, like so:

> + /* for toggling between page/bookmark info Edit menu item */
> + "Page Info" = "Page Info";
> - "Get Info" = "Get Info";
> + "Bookmark Info" = "Bookmark Info"; /* also used for the bookmark item context menu */

The following got spun off, right?

> And the following change to WebsiteDefaults.strings:
> + SupportLitePageDefault =
> "http://www.mozilla.org/projects/camino/support.html";

Please list an updated set of string changes and then update the status whiteboard to point to the correct comment number :)

Comment 31

[froodian (Ian Leue)]

Attachment: Patch

This just corrects the string changes.  So (pseudo-diff):

- "Get Info" = "Get Info";
+ /* for toggling between page/bookmark info Edit menu item */
+ "Page Info" = "Page Info";
+ "Bookmark Info" = "Bookmark Info"; /* also used for the bookmark item context menu */

Comment 32

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 241804 [details] [diff] [review]
Patch

kreeger, were you going to look at this, too?  We'd like to get this in a1....

Comment 33

[froodian (Ian Leue)]

So, comprehensive list of functionality changes being provided by this patch/nib combo:

- Moves Camino Feedback to Camino menu
- Moves Search The Web to File menu, and renames to "Search the Web…"
- Moves Get info to View Menu and renames to "Page Info" or "Bookmark Info", based on whether the BM manager is open
- Adds a shift alternate menu item for View Source (to load in background)
- Changes Go menu to History
- Moves Show History to History menu
- Renames "Add Page to Bookmarks…" to "Bookmark Current Page…"
- Adds alternate for "Bookmark Current Page" (dialogless, note absence of ellipsis) with the shortcut Cmd-Shift-K
- Keeps Cmd-D as a legacy keyboard shortcut for Add Bookmark
- Adds menu item for "Bookmark Current Tabs as Tab Group…" with the shortcut of Cmd-Option-K which utilizes the existing Add Bookmark sheet and checks the tab group checkbox.
- Adds alternate for "Bookmark Current Tabs as Tab Group" (dialogless) with the keyboard shortcut Cmd-Option-Shift-K.
- Moves "Local Network Services" to Bookmarks menu, and renames it to "Bonjour"
- Renames "About Local Network Services" to "About Bonjour"

Comment 34

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 241804 [details] [diff] [review]
Patch

rs=pink

Comment 35

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk.