105485 - personal toolbar bookmark tooltips: show me the title, not the url

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

personal toolbar bookmark tooltips:  show me the title, not the url

on my personal toolbar, I've got bookmarks that are cropped because the title of
the bookmarked page is too long.

when I mouse over, instead of getting the full title in the tooltip, I get the
url to the bookmark, which doesn't do me much good.

example:  I've got "Yahoo! TV - S...", the title of the page is "Yahoo! TV -
Survivor Pick'em"

the url is
"http://tvgames.yahoo.com/survivor/show?page=privategroup&gid=2110&tid=14355"

I'd rather see the full title on mouseover, than the url.  more and more (I
claim) the actual url is meaningless.

a suggestion, that is probably way too fancy:   if the title is cropped, show
the title in the tooltip.  if the title is not cropped, show the url.

Comment 1

[Blake Ross]

We could do what 4.x does, show the url in the statusbar and the title in the
tooltip.

Comment 2

[Doron Rosenberg (IBM)]

sounds good, especially since we have some problems with overlong tooltip texts.

Comment 3

[timeless]

... or we could do multiline tooltips, i'm assuming that's a nono in our ui, am
i right?

Comment 4

[(not reading, please use [e-mail] instead)]

> We could do what 4.x does, show the url in the statusbar and the title in the 
tooltip.

best of both worlds!  and, it's a similar link behavior to what happens when 
you mouse over a link a web page.  (you get the url in the status area)

Comment 5

[Chris Lyon]

*** Bug 93139 has been marked as a duplicate of this bug. ***

Comment 6

[Hixie (not reading bugmail)]

Attachment: Patch: fixes this bug as described above (name in tooltip, URI in status bar)

This patch won't actually cause anything to appear in the status bar until bug
31620 is fixed. However, that's a separate issue.

Comment 7

[Hixie (not reading bugmail)]

Attachment: Alternative fix: two line tooltips

Here's an alternative fix. It uses two line tooltips, first line is the title
and second line is the URI. Once the statustext bug is fixed the URI will also
appear in the status bar.

Comment 8

[Dan Price]

One annoyance I've had with the tooltips is that I use "google buttons"
(see http://www.google.com/options/buttons.html) and various other
javascripted buttons in my toolbar.  When I mouse over them, I get a
tooltip as follows:
--------------------------------------------------------------------------
|javascript:q=document.getSelection();if(!q){void(q=prompt('Enter text to|
--------------------------------------------------------------------------

The script is truncated, as it is too long for the tooltip.  Over the
weekend I hacked up a small change which looks to see if the URL for the
tooltip begins with the string 'javascript:'.  If so, it substitutes
some appropriate text like "Runs a Script" for the tooltip text-- but
this fix changes the same codepath so I'll wait to file an RFE to see
what happens here.  Perhaps you could roll an awareness of the
"javascript:" substring into this fix?

Even if not, here is my $0.002 on this fix...

The first fix proposed, which would cause the long URL to show up in
the status bar, would be OK, but I'd vote for the second (the two-line
version), because it localizes all of the relevant information in
the same place.  In the future this could even be extended to include
other information (like "last visited", or the favicon, or whatever).

Comment 9

[Hixie (not reading bugmail)]

I'd be wary of replacing javascript: urls in the tooltip. Feel free to file a
bug on the User Interface Design component suggesting it though. Similarly with
the last visited time -- an interesting idea, please file a bug! :-)

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 63200 [details] [diff] [review]
Alternative fix: two line tooltips

sr=[e-mail]

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 63200 [details] [diff] [review]
Alternative fix: two line tooltips

>+//Fill in tooltip for personal toolbar bookmark items
>+function FillInPTBITooltip(tipElement)
>+{
>+  var nodeLabel = tipElement.label;
>+  var url = tipElement.statusText;
>+
>+  if (nodeLabel || url) {
>+    var tooltipTitle = document.getElementById("ptbiTitleText");
>+    var tooltipUrl = document.getElementById("ptbiUrlText"); 
>+    if ((nodeLabel) && (nodeLabel != url)) {
>+      if (tooltipTitle.getAttribute("hidden") == "true")
>+        tooltipTitle.removeAttribute("hidden");
>+      tooltipTitle.setAttribute("value", nodeLabel);
>+    } 
>+    else  {
>+      tooltipTitle.setAttribute("hidden", "true");
>+    }
>+    if (url) {
>+      if (tooltipUrl.getAttribute("hidden") == "true")
>+        tooltipUrl.removeAttribute("hidden");
>+      tooltipUrl.setAttribute("value", url);
>+    }
>+    else {
>+      tooltipUrl.setAttribute("hidden", "true");
>+    }
>+    return true;
>+  } else {
>+    return false;
>+  }
>+}

Wow, the normal case code is overindented just so we can have a vestigial else
return false at the end.  How ugly, as well as illogical!

It is better to cast abnormal cases out with early return, so use DeMorgan's
theorem.

(Blake wanted me to lambaste, so here I am ;-).

/be

Comment 12

[Hixie (not reading bugmail)]

> Wow, the normal case code is overindented just so we can have a vestigial 
> else return false at the end.  How ugly, as well as illogical!

I disagree, on all counts.

It's not illogical. There are two possible alternatives, either there's
something to show, or there isn't. Why should we look for the unusual case
first? And why look for the case which requires a negation first?

It's aesthetic qualities can be debated until the 1.0 comes out; personally I
find the added confusion of |not|s outweighs any benefit of losing the 2 columns
of extra whitespace.

The |return false| is not vestigial, even if you read the code linearly (as
opposed to structurally, like a collapsable outline view) it appears right next
to its counterpart |return true|, separated by an |else| that makes the meaning
of the code obvious.


> It is better to cast abnormal cases out with early return

IMHO it is better to get to the important cases first, since that makes it
easier to see what the code is about. The unimportant edge cases can then be
dealt with afterwards.

Additionally, using the

   if (foo) {
     bar;
     return blah;
   } else {
     return glah;
   }

...structure conveys the meaning of the code better than the

   if (!foo) {
     return glah;
   }
   bar;
   return blah;

...case. The first very clearly shows that the two codepaths are mutually
exclusive, whereas the second case requires that the reader examine the actual
contents of the blocks to determine what the possible codepaths are.

Furthermore, the first case is easier to maintain should the "glah" block grow
to more than just a single return.


> so use DeMorgan's theorem.

DeMorgan's theorem is not relevant here, if the code were to be switched it
would be a simple case of negation.

Comment 13

[Brendan Eich [:brendan]]

>It's not illogical.

Yes, it is -- else after return is a control flow non-sequitur.

>There are two possible alternatives, either there's
>something to show, or there isn't.

Which is normal, or more common, or more interesting, or more likely to have
bugs in its implementation?  Hint: the big then-clause might be it.  Why in the
name of anything should it be indented more?  This Pascal-ized style infects
code and leads to opaque blocks of overindented code (ask waterson for Gecko
pointers).

>Why should we look for the unusual case first?

Because that case's then-clause should be the indented one that returns false
(false is another hint that the case is abnormal).

>And why look for the case which requires a negation first?

The condition may or may not require a negation.  Negation is not a big deal
compared to overindentation in my book, and we're talking about my book here.

>It's aesthetic qualities can be debated until the 1.0 comes out; personally I
>find the added confusion of |not|s outweighs any benefit of losing the 2
>columns of extra whitespace.

I'm not debating, I'm giving you a page out of Mozilla house style that I and
lots of others give during review.  If ben and blake are happy with the else
after return, I won't worry you further.  It sounded like blake was not, but
that he wanted me to be bad cop, so here I am.

>The |return false| is not vestigial, even if you read the code linearly (as
>opposed to structurally, like a collapsable outline view)

I used vestigial anatomically.  Code of the sort you wrote tends to grow in ripe
middle age to resemble an obese man whose belly is spilling over a tight belt
arond his hips.  What protrudes vestigially below that, I shant say :-).  Yes, I
was making an aesthetic criticism.  I'm not aware of linear (algebra?) vestiges.

Your outline view analogy is fatally flawed when considering control flow, which
code readers *must* consider (unless one is forced to eschew sugared gotos as in
Pascal :-).

>it appears right next to its counterpart |return true|, separated by an
>|else| that makes the meaning of the code obvious.

The meaning of the code is not unobvious in either case, but the control flow
analysis that careful reviewers make is hobbled by the return-else, and the main
or normal clause is overindented.  Again, I don't mean to break a butterfly upon
a wheel here.  This particular instance is a so-so example, but the
else-after-return pattern is an illogical meme that has caused, and will
continue to cause, unsightly multi-level logic cases where the normal code is
overindented by the maximum number of logic levels.

>> It is better to cast abnormal cases out with early return
>
>IMHO it is better to get to the important cases first, since that makes it
>easier to see what the code is about.

Not if the normal case is indented too much, or conditioned by if-if-if chains
that are less clear when juxtaposed than when treated as early exceptions, with
comments as needed, and with early returns (for if-then-return sequences).  The
word "early" is important here for the reader, who can then assume more
invariant when reading the normal case code in a non-trivial example.

>The unimportant edge cases can then be dealt with afterwards.

Besides flagrant control flow non-sequiturs, code over-indentation, and opaque
nested if conditions, another symptom of the bad meme emerges over time, with
many hands maintaining and extending the code.  That symptom is

    normal code here;
    if (some normal condition) {
      more normal code here;
    } else {
      return false;
    }
    yet more normal code here;

This example contains no else after return, but it shows a hazard in your "test
for normal case first" principle.  Inverting the test then begs the question of
"why put an else after the return false in the then clause".

>Additionally, using the
>   if (foo) {
>     bar;
>     return blah;
>   } else {
>     return glah;
>   }
>
>...structure conveys the meaning of the code better than the
>
>   if (!foo) {
>     return glah;
>   }
>   bar;
>   return blah;
>
>...case.

I disagree of course, but want to point out right here that you are not thinking
of the programming-in-the-large problem.  Again, this patch, with only one logic
level, is not the worst case spot that the slippery slope you defend can and
does land us on.

>The first very clearly shows that the two codepaths are mutually
>exclusive, whereas the second case requires that the reader examine the actual
>contents of the blocks to determine what the possible codepaths are.

That's right -- the reader has to look for return, break, continue, and goto in
C and C++.  Any reader who does not, and who uses equal indentation of then and
else clauses, will miss bugs and do a poorer job reviewing.  The hard case is
not this simple one.  It is much more readable to cast out abnormal cases (in a
function body, a loop body, or other block) before overindenting the normal
case, when the code complexity is high, or even when the function or method body
is merely long, and there are a few or more logic levels in the "Pascal" version.

The early return version is also clearer to anyone analyzing the control flow. 
Overindented code is less easily analyzed above a certain complexity/size
threshold, not only because there are no collapsing outliner views in common
code editors, but also because anyone doing a diligent review *has* to look for
return, goto, and possibly break and continue.

>Furthermore, the first case is easier to maintain should the "glah" block grow
>to more than just a single return.

I'm sorry, no.  We have lots of code (kipp tended to write such code in Gecko)
that grows glah until the normal case is indented ten levels, and each else has
to deal with restoring invariants.  Your theory does not match our experience.

>> so use DeMorgan's theorem.
>
>DeMorgan's theorem is not relevant here, if the code were to be switched it
>would be a simple case of negation.

No, I meant what I wrote: rather than

+  if (nodeLabel || url) {
+    ...;
+    return true;
+  } else {
+    return false;
+  }

I did *not* mean this:

+  if (!(nodeLabel || url))
+    return false;
+  ...;
+  return true;

but this:

+  if (!nodeLabel && !url)
+    return false;
+  ...;
+  return true;

Over to blake, the good cop.

/be

Comment 14

[Brendan Eich [:brendan]]

>The unimportant edge cases can then be dealt with afterwards.

This bears repeating, especially as I typo'd "invariant", in "[the reader] can
then assume more invariantS when reading the normal case code in a non-trivial
example."  The edge cases in non-trivial code (which again, this patch is not
under either proposal) should be dealt with *first*.  Robust, complex code is
all about edge case isolation, early treatment, and invariant reduction so the
reader can safely "ignore" those cases.

Philosophically, I think "dealt with afterwards" sums up a problem, not a solution.

/be

Comment 15

[Brendan Eich [:brendan]]

Argh, time for sleep: I meant "variant reduction".  Or "invariant increase".  Or
something.  Time to go...

/be

Comment 16

[Hixie (not reading bugmail)]

Your three references to a Pascal-style of programming are probably more
relevant than you imagined: I was raised on Pascal, and can therefore read code
written in the Pascal style with more ease than code written in what you call
the Mozilla house style.


>> It's not illogical.
> Yes, it is -- else after return is a control flow non-sequitur

I have no idea what this means. The phrase "control flow non-sequitur" gets no
hits on google, and taking the phrase literally, "control flow does not follow",
also doesn't help me, as the control flow follows more directly in the Pascal
style than the no-else style.


>> There are two possible alternatives, either there's something to show, or
>> there isn't.
> Which is normal, or more common, or more interesting, or more likely to have
> bugs in its implementation?

The first one. That's why I put it first.


> Hint: the big then-clause might be it. Why in the name of anything should it
> be indented more?

It isn't. It's indented the same amount.


> This Pascal-ized style infects code and leads to opaque blocks of overindented
> code (ask waterson for Gecko pointers).

I've seen lots of places in Gecko code that are massively over-indented -- and
you would never catch me writing such code (except in cases where I am following
instructions from the module peers and don't understand the code well enough
myself to make it more abstract). It's unreadable.


>> Why should we look for the unusual case first?
> Because that case's then-clause should be the indented one that returns false
> (false is another hint that the case is abnormal).

Something else you wouldn't catch me doing is returning from the middle of a
function, as that makes it hard to read the code. In code I write you could
remove all the |return foo| statements, replace them with |rv = value|
statements, add a |return value| at the end, and get the same result. Breaking
this convention results in code that, for me at least, is harder to read.


>> The |return false| is not vestigial, even if you read the code linearly (as
>> opposed to structurally, like a collapsable outline view)
> [...] Code of the sort you wrote tends to grow in ripe middle age to resemble
> an obese man whose belly is spilling over a tight belt around his hips. [...]
The function in my patch is at about the limit of size that I would be happy
about. If I were to add any more code, I would split the function into two.


> Your outline view analogy is fatally flawed when considering control flow,
> which code readers *must* consider (unless one is forced to eschew sugared
> gotos as in Pascal :-).

Eschewing sugared gotos is, IMHO, a good thing. :-)

That doesn't mean I am against |break|, |continue| or |return|, just that I
don't think they should be used in the middle of code where, if they were
removed, more code in the block would end up being executed.


> The meaning of the code is not unobvious in either case, but the control flow
> analysis that careful reviewers make is hobbled by the return-else [...]

I really don't see why the if-return-else-return case causes any more troubles
to control flow analysis than the if-return;return case. They are exactly
equivalent (so much so that any self-respecting compiler would generate the same
code for both).


> This particular instance is a so-so example, but the else-after-return pattern
> is an illogical meme that has caused, and will continue to cause, unsightly
> multi-level logic cases where the normal code is overindented by the maximum
> number of logic levels.

That is akin to banning drinking altogether simply because some people are
violent when drunk.

Sure, the return-else pattern can be illogical, and can be used in code that is,
through the use of that pattern, made a lot worse than it need be. However, this
is not such a case. In this particular case, the if-else-return-else pattern is
used to make the code _easier_ to read, much like a single margarita can be
healthy. :-)


>> IMHO it is better to get to the important cases first, since that makes it
>> easier to see what the code is about.
> Not if the normal case is indented too much, or conditioned by if-if-if chains
> [...]

Not to belabour a point, but while I agree in general, this is not such a case.


> [...] That symptom is
>
>    normal code here;
>    if (some normal condition) {
>      more normal code here;
>    } else {
>      return false;
>    }
>    yet more normal code here;

I fully agree that this is a horrible practice (not to mention buggy: the normal
return value is undefined). I would write that as:

   normal code here;
   if (some normal condition) {
     more normal code here;
     return yet more normal code here;
   } else {
     return false;
   }

In my opinion, it is the responsability of the code maintainers and reviewers to
make sure the code doesn't reach that state. It should not be the responsability
of the initial programmer to make sure that future maintainers cannot shoot
themselves in the foot. That would be a losing battle.


> This example contains no else after return, but it shows a hazard in your
> "test for normal case first" principle. Inverting the test then begs the
> question of "why put an else after the return false in the then clause".

Because otherwise removing the return (or replacing it with rv=value and
returning rv at the end of the function) would change the meaning of the code, a
good sign (in my opinion) that the code is going to be hard to read.


> That's right -- the reader has to look for return, break, continue, and goto
> in C and C++.

(this is actually JS)

The fact that the reader does have to look for this is no excuse for making the
reader actually _find_ any.

The reader should also not have to find any leaks, but he still has to look for
them.


> Any reader who does not, and who uses equal indentation of then and else
> clauses, will miss bugs and do a poorer job reviewing.

Ideally, the reader would look through the code, find no sugared gotos in the
middle of any blocks of code, and be able to rely on the indentation from that
point on, resulting in fewer bugs missed and a better job reviewing. IMHO of
course.


> The early return version is also clearer to anyone analyzing the control flow.
> Overindented code is less easily analyzed above a certain complexity/size
> threshold, not only because there are no collapsing outliner views in common
> code editors, but also because anyone doing a diligent review *has* to look
> for return, goto, and possibly break and continue.

Any function that ends up in that hole is too long anyway.


>> Furthermore, the first case is easier to maintain should the "glah" block
>> grow to more than just a single return.
> I'm sorry, no. We have lots of code (kipp tended to write such code in Gecko)
> that grows glah [...]

glah, in my example, was the |return false| case, which isn't what I think you
are referring to.


> No, I meant what I wrote: rather than
> 
> +  if (nodeLabel || url) {
> +    ...;
> +    return true;
> +  } else {
> +    return false;
> +  }
> 
> I did *not* mean this:
> 
> +  if (!(nodeLabel || url))
> +    return false;
> +  ...;
> +  return true;
> 
> but this:
> 
> +  if (!nodeLabel && !url)
> +    return false;
> +  ...;
> +  return true;

That's even worse. Every negation gives me a headache. :-)

Comment 17

[Brendan Eich [:brendan]]

The "glah" block doesn't tend to grow, the normal (return true) one does.  But I
have put down the truncheon.  Blake is eager to soothe a confession from you
with good-cop love-talk, I'm sure!

/be

Comment 18

[Hixie (not reading bugmail)]

> control flow non-sequitur

Come to think of it, the if-return-else-return case is a _lot_ easier to analyse
than the if-return;return case.

Here's the flow-chart for my version of the code you mentioned:

      normal code
           |
           |
           | 
      some normal
       condition
         /  \
       T/    \F
       /      \
     more    false
    normal
     code
   yet more
    normal
     code

I can't see how to draw a flow-chart for either the version you quoted or the
version I believe you would have me write.


Regardless of the outcome of this discussion with respect to this patch, I would
be interested in continuing this discussion (especially your reply to my
comments above), as I have already learnt quite a bit just through your forcing
 me to think about this. :-)

I don't mind if we do that through e-mail or in this bug.

Comment 19

[Brendan Eich [:brendan]]

Hixie, here're two clues: 1) no one draws flow charts; 2) no one uses Pascal for
real-world apps any longer.  Yeah, I'm exagerrating -- but only slightly.

Email me a reduction of your last comment, I'm pressed for time but I will reply
point-by-point when I have time, and spare the cc: list of this bug further bad
cop behavior :-).

/be

Comment 20

[Blake Ross]

Oh...dear... here is a flowchart of my boredom with this discussion:

     Z
    / \
   ZZZZ
  /    \
 ZZZZZZZZ

If I were writing this, I would much prefer an |if (!(nodeLabel || url)) return 
false;| at the beginning and nice, non-indented code.  And I would remove the 
extra parens from |if ((nodeLabel) && (nodeLabel != url)) {|.

But I am the mere reviewer.  Ben, in all his super-reviewing mightiness, should 
have demanded a fix for those problems!

Please remove the <popupset>, though, and the getAttribute("hidden") checks.

r=blake

Comment 21

[Hixie (not reading bugmail)]

Attachment: Alternative fix: two line tooltips, v2

In the interests of world peace, here is my second attempt.

Comment 22

[Hixie (not reading bugmail)]

checked in

Comment 23

[Brendan Eich [:brendan]]

(Way to play good cop, blake!  Falling asleep during the interrogation -- I'm
talking to the Lieutenant!)

/be