188679 - Enhanced tooltips with URL for some Toolbar icons

Status: VERIFIED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[Stephane Moureau]

Add the (Name and/or) URL of the corresponding page in the tooltip of the
following toolbar icons:
Go Back, Go Forward, Go Home, Search, Send Link.
eg: Go back to the page: Apple - http://www.apple.com/

Comment 1

[S Woodside]

Good idea.

Comment 2

[Kathleen :Brade]

-->pinkerton

Comment 3

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

I don't think we should do this. Any thoughts?

Comment 4

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

It might be a neat bit of polish for back and forward; I can't see the need for
it on home (who doesn't know their homepage) or send link (that's the current page).

I'd future/1.2 it for back and forward only.

Comment 5

[Mike Pinkerton (not reading bugmail)]

worth considering.

Comment 6

[Wevah (Nate Weaver)]

I like this idea, and it shouldn't be too hard. Might be worth putting the URL
on the "Home" button, too, if we do this.

Comment 7

[Wevah (Nate Weaver)]

Oh; already said. Dur.

Comment 8

[froodian (Ian Leue)]

-> Me

Comment 9

[froodian (Ian Leue)]

Attachment: Provides Enhanced tooltips for back and forward

This patch:

-Takes the logic used to find the previous URL when bookmarking from the bookmarks manager, and moves it to two separate methods, |getPreviousTitle| and |getNextTitle|.  Note that since most of the code in these methods is the same, it plays merry hell with the diff file.  Hopefully bugzilla's diff view makes it look a bit nicer.  Basically, |getPreviousTitle| is what was in |bookmarkableTitle| verbatim, and |getNextTitle| just finds the number of elements in in the session history, and makes sure that the current URL isn't the last one.

-Sets the toolbar items to the previous/next title & url (using the same tooltip format as for bookmark buttons), or if there is no previous/next title, keeps them the way they are currently.

-moves the validation for |manageBookmarks| to after the straight "navigation" toolbar buttons

-makes the following STRING CHANGES: 
"BackToolTipFormat" = "%1$@\n—\n%2$@";  /* 1=title, 2=url */
"ForwardToolTipFormat" = "%1$@\n—\n%2$@";  /* 1=title, 2=url */

We're not validating Home per IRC, since most people know what their home page is, we'd have to have it be URL only (we don't necessarily know the title), and  it wouldn't really make sense once we get multiple home page support.

Comment 10

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

Do we really need 3 different tooltip strings that will always be the same format?  Can't we make back and forward just use the existing string?

Comment 11

[Stuart Morgan]

Comment on attachment 227639 [details] [diff] [review]
Provides Enhanced tooltips for back and forward

>+      if (curEntryIndex < count)
>+      {
>+        nsCOMPtr<nsIHistoryEntry> entry;
>+        sessionHistory->GetEntryAtIndex(curEntryIndex + 1, PR_FALSE, getter_AddRefs(entry));

If curEntryIndex is |count-1|, then this will attempt to get the entry at index |count|, which is out of bounds for a zero-indexed array of |count| elements.


Also, instead of duplicating all the code, implement them both in terms of a method that gets the URL and title for a given index, calling it with currentIndex-1 and currentIndex+1 as appropriate.

Comment 12

[froodian (Ian Leue)]

Attachment: Consolidates Code

This patch:
-Implements a single method to return the title and url of items at a given relative index in session history.
-Piggybacks on the existing tooltip format, but in order to do so, makes the following STRING CHANGE (instead of those listed in comment 9):
renames "BookmarkButtonToolTipFormat" to "ToolbarItemToolTipFormat"

Note that the diff is still ugly as all getout, and for that I'm sorry.

Comment 13

[Stuart Morgan]

Comment on attachment 227862 [details] [diff] [review]
Consolidates Code

>+- (void)sessionHistItemAtRelativeOffset:(int)indexOffset forWrapper:(BrowserWrapper*)inWrapper title:(NSString**)outTitle URL:(NSString**)outURLString;

Abbreviating in method names is discouraged, and dropping three letters from History isn't really saving much here.

>+      NSString* tooltipString = [NSString stringWithFormat:NSLocalizedString(@"ToolbarItemToolTipFormat", @""), backTitle, backURL];
...
>+      NSString* tooltipString = [NSString stringWithFormat:NSLocalizedString(@"ToolbarItemToolTipFormat", @""), forwardTitle, forwardURL];

In order to continue describing actions, these should use format strings that prepend "Go back to" and "Go forward to" to the tooltip.

>+    BOOL enable = [[mBrowserView getBrowserView] canGoBack];
>+    if (!enable && ![self bookmarkManagerIsVisible])
>+      enable = true;

As long as you are touching this, s/true/YES/

>+  if (webBrowser)
>+  {
...

And since you're touching all the bracing that's ending up in this method, perhaps a bit of fighting for the side of righteousness in the battle for control of BWC.

Comment 14

[froodian (Ian Leue)]

Attachment: Uses action language

-Addresses stuart's comments
-Adds the following NEW STRINGS

"BackToolTipFormat" = "Go back to “%1$@”\n—\n%2$@";  /* 1=title, 2=url */
"ForwardToolTipFormat" = "Go forward to “%1$@”\n—\n%2$@";  /* 1=title, 2=url */

This is a little bit ugly on pages where the title is the url, but then again, so is having the title displayed as the window title... just above the location bar.  I think it's one of those necessary evils of not naming pages.

Comment 15

[Stuart Morgan]

Comment on attachment 227884 [details] [diff] [review]
Uses action language

>+    // set the tooltip to the previous URL and title
>+    if ([theItem isEnabled]) { // if there is a previous URL
>+      NSString* backTitle;
>+      NSString* backURL;
>+      [self sessionHistoryItemAtRelativeOffset:-1 forWrapper:[self getBrowserWrapper] title:&backTitle URL:&backURL];
>+
>+      NSString* tooltipString = [NSString stringWithFormat:NSLocalizedString(@"BackToolTipFormat", @""), backTitle, backURL];
>+      // using "\n\n" as a tooltip string causes Cocoa to hang when displaying the tooltip,
>+      // so be paranoid about not doing that
>+      if (![tooltipString isEqualToString:@"\n\n"])
>+        [theItem setToolTip:tooltipString];
>+    }
>+    else // there's no previous URL, so give it a stock tooltip
>+      [theItem setToolTip:NSLocalizedString(@"BackToolTip", @"")];
>+

I don't like that there's a path here where no tooltip is set (tooltip == "\n\n") rather than setting the generic one.  Related to that, sessionHistoryItemAt... can return nil title and URL as error conditions, but this uses them without checking, which would give weird tooltips.  There needs to be fallback on the stock tooltip in all error cases. (You may want to see if there's a reasonable way to offload the logic to a helper method so you aren't doing it all twice.)

Comment 16

[froodian (Ian Leue)]

Attachment: Checks for null states better

Done.  You get to decide whether it qualifies as "a reasonable way", but it's certainly a way. ;)

Comment 17

[Stuart Morgan]

Comment on attachment 228910 [details] [diff] [review]
Checks for null states better

>+// Helper method for setting formatted tooltips with a title and url (like the back/forward tooltips)
>+//
>+- (NSString *)generateFormattedToolTip:(NSString *)format title:(NSString *)backTitle URL:(NSString *)backURL

This needs a less generic name, especially since it's in BWC. Mabye locationTooltipWithFormat:title:url:

>+    NSString* toolTipString = @"";

This should init to nil, since it will never be used in this form...

>+      toolTipString = [self generateFormattedToolTip:@"BackToolTipFormat" title:backTitle URL:backURL];

...and because this returns nil on error (as it should), not @""...

>...
>+    if (toolTipString != @"")

...meaning that this check isn't reliable as is--it can just be a nil check with init changed.

Comment 18

[Wevah (Nate Weaver)]

(Not to mention that |toolTipString != @""| is a pointer comparison and not an empty string check.)

Comment 19

[froodian (Ian Leue)]

Attachment: Checks for nil string properly

Comment 20

[Stuart Morgan]

Comment on attachment 228962 [details] [diff] [review]
Checks for nil string properly

The patch has bitrotted because of the reload change, but with that modified by hand it works fine.

>+    if (toolTipString != nil)

if (toolTipString)

>+
>+      // send it off to a helper method to set up the toolTipString and make sure that nothing's nil

remove this

>+    if (toolTipString != nil)

if (toolTipString)

>+    BOOL enable = [[mBrowserView getBrowserView] canGoBack];
>+    if (!enable && ![self bookmarkManagerIsVisible])
>+      enable = YES;
>+    return enable;

I know this isn't your code (again), but it would be great if while you are respinning you could replace that whole block with:

    return [[mBrowserView getBrowserView] canGoBack] || (![self bookmarkManagerIsVisible]);

>   if ([[curURL lowercaseString] isEqualToString:@"about:bookmarks"] ||
>-      [[curURL lowercaseString] isEqualToString:@"about:history"])
>-  {
>+      [[curURL lowercaseString] isEqualToString:@"about:history"]) {

The way it was is correct for multi-line if conditions; please put it back.


r=me with those changes.

Comment 21

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 229895 [details] [diff] [review]
r=smorgan patch

+      NSString* backTitle;
+      NSString* backURL;

init these to nil (just to be extra safe) and sr=pink.

Comment 23

[froodian (Ian Leue)]

Attachment: sr patch

Comment 24

[Nick Kreeger]

Fixed trunk and branch.

Comment 25

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

Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.