Enhanced tooltips with URL for some Toolbar icons

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
--
enhancement
VERIFIED FIXED
15 years ago
11 years ago

People

(Reporter: Stephane Moureau, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
Good idea.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

14 years ago
-->pinkerton
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.
Status: NEW → ASSIGNED
Target Milestone: Camino1.0 → Camino1.2

Comment 6

12 years ago
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

12 years ago
Oh; already said. Dur.
(Assignee)

Comment 8

11 years ago
-> Me
Assignee: mikepinkerton → stridey
Status: ASSIGNED → NEW
(Assignee)

Comment 9

11 years ago
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.
Attachment #227639 - Flags: review?(nick.kreeger)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
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 11

11 years ago
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-
(Assignee)

Comment 12

11 years ago
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.
Attachment #227639 - Attachment is obsolete: true
Attachment #227862 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Whiteboard: [New strings in comment 9] → [New strings in comment 12]

Comment 13

11 years ago
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-
(Assignee)

Comment 14

11 years ago
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.
Attachment #227862 - Attachment is obsolete: true
Attachment #227884 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Whiteboard: [New strings in comment 12] → [New strings in comment 14]

Comment 15

11 years ago
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-
(Assignee)

Comment 16

11 years ago
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. ;)
Attachment #227884 - Attachment is obsolete: true
Attachment #228910 - Flags: review?(stuart.morgan)

Comment 17

11 years ago
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-

Comment 18

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

Comment 19

11 years ago
Created attachment 228962 [details] [diff] [review]
Checks for nil string properly
Attachment #228910 - Attachment is obsolete: true
Attachment #228962 - Flags: review?(stuart.morgan)

Comment 20

11 years ago
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+
(Assignee)

Comment 21

11 years ago
Created attachment 229895 [details] [diff] [review]
r=smorgan patch
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+
(Assignee)

Comment 23

11 years ago
Created attachment 230061 [details] [diff] [review]
sr patch
Attachment #229895 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Whiteboard: [New strings in comment 14] → [New strings in comment 14] [needs checkin]

Comment 24

11 years ago
Fixed trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [New strings in comment 14] [needs checkin]

Updated

11 years ago
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Status: RESOLVED → VERIFIED
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.