Closed
Bug 105485
Opened 23 years ago
Closed 23 years ago
personal toolbar bookmark tooltips: show me the title, not the url
Categories
(SeaMonkey :: Bookmarks & History, defect)
SeaMonkey
Bookmarks & History
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla0.9.9
People
(Reporter: sspitzer, Assigned: ian)
References
Details
Attachments
(2 files, 1 obsolete file)
918 bytes,
patch
|
Details | Diff | Splinter Review | |
2.67 KB,
patch
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
We could do what 4.x does, show the url in the statusbar and the title in the tooltip.
Comment 2•23 years ago
|
||
sounds good, especially since we have some problems with overlong tooltip texts.
... or we could do multiline tooltips, i'm assuming that's a nono in our ui, am i right?
Component: Browser-General → Bookmarks
QA Contact: doronr → claudius
Reporter | ||
Comment 4•23 years ago
|
||
> 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)
Updated•23 years ago
|
Target Milestone: --- → mozilla1.0
Updated•23 years ago
|
Status: NEW → ASSIGNED
Updated•23 years ago
|
Target Milestone: mozilla1.0 → mozilla0.9.9
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.1
Assignee | ||
Comment 6•23 years ago
|
||
This patch won't actually cause anything to appear in the status bar until bug 31620 is fixed. However, that's a separate issue.
Assignee | ||
Comment 7•23 years ago
|
||
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.
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).
Assignee | ||
Comment 9•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 63200 [details] [diff] [review] Alternative fix: two line tooltips sr=ben@netscape.com
Attachment #63200 -
Flags: superreview+
Comment 11•23 years ago
|
||
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
Attachment #63200 -
Flags: needs-work+
Assignee | ||
Comment 12•23 years ago
|
||
> 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.
Assignee: blaker → ian
Status: ASSIGNED → NEW
Keywords: patch
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: mozilla1.1 → mozilla0.9.9
Comment 13•23 years ago
|
||
>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•23 years ago
|
||
>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•23 years ago
|
||
Argh, time for sleep: I meant "variant reduction". Or "invariant increase". Or something. Time to go... /be
Assignee | ||
Comment 16•23 years ago
|
||
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•23 years ago
|
||
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
Assignee | ||
Comment 18•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
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
Assignee | ||
Comment 21•23 years ago
|
||
In the interests of world peace, here is my second attempt.
Attachment #63200 -
Attachment is obsolete: true
Assignee | ||
Comment 22•23 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 23•23 years ago
|
||
(Way to play good cop, blake! Falling asleep during the interrogation -- I'm talking to the Lieutenant!) /be
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•