Last Comment Bug 188679 - Enhanced tooltips with URL for some Toolbar icons
: Enhanced tooltips with URL for some Toolbar icons
Status: VERIFIED FIXED
: 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)
: sairuh (rarely reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-01-11 08:06 PST by Stephane Moureau
Modified: 2007-03-31 18:03 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Provides Enhanced tooltips for back and forward (11.36 KB, patch)
2006-06-29 18:06 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Consolidates Code (11.82 KB, patch)
2006-07-01 22:29 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Uses action language (10.55 KB, patch)
2006-07-02 08:39 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Checks for null states better (11.60 KB, patch)
2006-07-11 22:57 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Checks for nil string properly (11.60 KB, patch)
2006-07-12 08:18 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
r=smorgan patch (11.12 KB, patch)
2006-07-19 14:55 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review
sr patch (11.15 KB, patch)
2006-07-20 18:35 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review

Description User image Stephane Moureau 2003-01-11 08:06:05 PST
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 User image S Woodside 2003-01-17 19:15:25 PST
Good idea.
Comment 2 User image Kathleen Brade 2003-05-23 08:40:43 PDT
-->pinkerton
Comment 3 User image Samuel Sidler (old account; do not CC) 2005-07-28 23:05:31 PDT
I don't think we should do this. Any thoughts?
Comment 4 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-07-29 00:31:02 PDT
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 User image Mike Pinkerton (not reading bugmail) 2005-07-29 12:24:32 PDT
worth considering.
Comment 6 User image Wevah 2005-07-29 15:26:50 PDT
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 User image Wevah 2005-07-29 15:30:14 PDT
Oh; already said. Dur.
Comment 8 User image froodian (Ian Leue) 2006-05-19 00:22:51 PDT
-> Me
Comment 9 User image froodian (Ian Leue) 2006-06-29 18:06:01 PDT
Created attachment 227639 [details] [diff] [review]
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-01 00:33:11 PDT
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 User image Stuart Morgan 2006-07-01 09:41:23 PDT
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 User image froodian (Ian Leue) 2006-07-01 22:29:07 PDT
Created attachment 227862 [details] [diff] [review]
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 User image Stuart Morgan 2006-07-02 00:33:45 PDT
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 User image froodian (Ian Leue) 2006-07-02 08:39:57 PDT
Created attachment 227884 [details] [diff] [review]
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 User image Stuart Morgan 2006-07-11 21:51:02 PDT
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 User image froodian (Ian Leue) 2006-07-11 22:57:24 PDT
Created attachment 228910 [details] [diff] [review]
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 User image Stuart Morgan 2006-07-11 23:08:57 PDT
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 User image Wevah 2006-07-12 02:38:23 PDT
(Not to mention that |toolTipString != @""| is a pointer comparison and not an empty string check.)
Comment 19 User image froodian (Ian Leue) 2006-07-12 08:18:40 PDT
Created attachment 228962 [details] [diff] [review]
Checks for nil string properly
Comment 20 User image Stuart Morgan 2006-07-19 08:31:34 PDT
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 User image froodian (Ian Leue) 2006-07-19 14:55:18 PDT
Created attachment 229895 [details] [diff] [review]
r=smorgan patch
Comment 22 User image Mike Pinkerton (not reading bugmail) 2006-07-20 06:38:19 PDT
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 User image froodian (Ian Leue) 2006-07-20 18:35:05 PDT
Created attachment 230061 [details] [diff] [review]
sr patch
Comment 24 User image Nick Kreeger 2006-07-20 20:46:29 PDT
Fixed trunk and branch.
Comment 25 User image Samuel Sidler (old account; do not CC) 2007-03-31 18:03:37 PDT
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.

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