Closed Bug 188679 Opened 22 years ago Closed 18 years ago

Enhanced tooltips with URL for some Toolbar icons


(Camino Graveyard :: Toolbars & Menus, enhancement)

Not set


(Not tracked)



(Reporter: stf, Assigned: froodian)


(Keywords: fixed1.8.1)


(1 file, 6 obsolete files)

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 -
Good idea.
Ever confirmed: true
Assignee: brade → pinkerton
Target Milestone: --- → Camino1.0
I don't think we should do this. Any thoughts?
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.
worth considering.
Target Milestone: Camino1.0 → Camino1.2
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.
Oh; already said. Dur.
-> Me
Assignee: mikepinkerton → stridey
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.
Attachment #227639 - Flags: review?(nick.kreeger)
Whiteboard: [New strings in comment 9]
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 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.
Attachment #227639 - Flags: review?(nick.kreeger) → review-
Attached patch Consolidates Code (obsolete) — Splinter Review
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.
Attachment #227639 - Attachment is obsolete: true
Attachment #227862 - Flags: review?(stuart.morgan)
Whiteboard: [New strings in comment 9] → [New strings in comment 12]
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.
Attachment #227862 - Flags: review?(stuart.morgan) → review-
Attached patch Uses action language (obsolete) — Splinter Review
-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.
Attachment #227862 - Attachment is obsolete: true
Attachment #227884 - Flags: review?(stuart.morgan)
Whiteboard: [New strings in comment 12] → [New strings in comment 14]
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.)
Attachment #227884 - Flags: review?(stuart.morgan) → review-
Attached patch Checks for null states better (obsolete) — Splinter Review
Done.  You get to decide whether it qualifies as "a reasonable way", but it's certainly a way. ;)
Attachment #227884 - Attachment is obsolete: true
Attachment #228910 - Flags: review?(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.
Attachment #228910 - Flags: review?(stuart.morgan) → review-
(Not to mention that |toolTipString != @""| is a pointer comparison and not an empty string check.)
Attached patch Checks for nil string properly (obsolete) — Splinter Review
Attachment #228910 - Attachment is obsolete: true
Attachment #228962 - Flags: review?(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.
Attachment #228962 - Flags: review?(stuart.morgan) → review+
Attached patch r=smorgan patch (obsolete) — Splinter Review
Attachment #228962 - Attachment is obsolete: true
Attachment #229895 - Flags: superreview?(mikepinkerton)
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.
Attachment #229895 - Flags: superreview?(mikepinkerton) → superreview+
Attached patch sr patchSplinter Review
Attachment #229895 - Attachment is obsolete: true
Whiteboard: [New strings in comment 14] → [New strings in comment 14] [needs checkin]
Fixed trunk and branch.
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [New strings in comment 14] [needs checkin]
Keywords: fixed1.8.1
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.
Target Milestone: Camino1.2 → Camino1.1
You need to log in before you can comment on or make changes to this bug.