GNOME Bugzilla – Bug 666170
Improve timing of tooltips (like in gtk+)
Last modified: 2012-01-20 19:21:53 UTC
I noticed that the recent gtk+ tooltips have an initial delay before showing up, but once the first tooltip for a button in a toolbar is visible (meaning the user is very likely exploring things) and you move the pointer along the toolbar, the tooltip will follow immediately. It would be nice if the tooltips for apps in the dash would behave like that, too: the first tooltip would take some time to appear, but after that, moving the pointer over the apps in the dash would make the tooltip follow instantly. As far as i can tell the unity launcher tooltips behave like that too. Alternatively, the timeout for the dash tooltips could be made smaller.
This is a good idea. I'd prefer the initial time out to be lower than what we have now. It's frustrating waiting for the label to appear.
(Comment #1 already qualifies as ui-review, removing keyword)
(In reply to comment #2) > (Comment #1 already qualifies as ui-review, removing keyword) OK, I thought the tag was more used as a way to find bugs involving designers, even when they already commented on the issue.
In bug 6661066 we are implementing the alternative solution.
In bug 666166 we are implementing the alternative solution.
(In reply to comment #5) > In bug 666166 we are implementing the alternative solution. I'd like to see both solutions in combination - faster initial appearance of the label and instant display of other labels thereafter.
Created attachment 204081 [details] [review] Patch visibility timeout for labels to behave more like gtk+ I finished up a tiny fix... Allan is this the behavior you seek?
Created attachment 204082 [details] [review] Patch visibility timeout for labels to behave more like gtk+ Quick fix
Created attachment 204111 [details] [review] Patch visibility timeout for labels to behave more like gtk+ Updated patch --> Please review
(In reply to comment #9) > Created an attachment (id=204111) [details] [review] > Patch visibility timeout for labels to behave more like gtk+ > > Updated patch --> Please review The design looks good. Would it be possible to use a very quick fade for showing and hiding the label once they are being displayed? Mixing fade transitions with non-fading feels awkward.
Review of attachment 204111 [details] [review]: Works as advertised, but there are a couple of style nits / clarity issues. Regarding the commit message, we agreed to not call the dash labels "tooltips", and it is unclear what "toolbar" refers to. ::: js/ui/dash.js @@ +302,3 @@ + this._hideLabelTimeoutId = 0; + this._hoverTimeout = DASH_ITEM_HOVER_TIMEOUT; + this._isShowingLabel = false; Sounds a bit odd to me - something like _labelIsShowing or _labelShowing reads more natural imho (with a preference for the latter, as capital i + l can be hard to distinguish) @@ +459,3 @@ return false; })); + Mainloop.source_remove(this._hideLabelTimeoutId); You should (1) check that _hideLabelTimeoutId is valid (2) reset _hideLabelTimeoutId after calling source_remove @@ +465,3 @@ + Mainloop.source_remove(this._showLabelTimeoutId); + if (this._isShowingLabel) { + this._hoverTimeout = 0; Any reason for not updating _hoverTimeout in the "showLabelTimeout" function? @@ +466,3 @@ + if (this._isShowingLabel) { + this._hoverTimeout = 0; + this._hideLabelTimeoutId = Mainloop.timeout_add(DASH_ITEM_HOVER_TIMEOUT, Either _hideLabelTimeoutId is a misnomer (as the function sets two variables without hiding anything), or the call to hideLabel() should be moved into the anonymous function (probably nicer for symmetry with the if{} part) @@ +469,3 @@ + Lang.bind(this, function() { + this._hoverTimeout = DASH_ITEM_HOVER_TIMEOUT; + this._isShowingLabel = false; Missing return value. @@ +472,3 @@ + })); + } + this._showLabelTimeoutId = 0; The line above really belongs to the Mainloop.source_remove() call above.
Review of attachment 204111 [details] [review]: ::: js/ui/dash.js @@ +465,3 @@ + Mainloop.source_remove(this._showLabelTimeoutId); + if (this._isShowingLabel) { + this._hoverTimeout = 0; which showLabelTimeout function? @@ +466,3 @@ + if (this._isShowingLabel) { + this._hoverTimeout = 0; + this._hideLabelTimeoutId = Mainloop.timeout_add(DASH_ITEM_HOVER_TIMEOUT, I don't get the problem sorry? how should it look like then?
Created attachment 205494 [details] [review] Patch visibility timeout for labels to behave more like gtk+ Went through most of Florians fixes
(In reply to comment #12) > which showLabelTimeout function? The anonymous function passed as timeout source whose source id is _showLabelTimeoutId. > @@ +466,3 @@ > + if (this._isShowingLabel) { > + this._hoverTimeout = 0; > + this._hideLabelTimeoutId = > Mainloop.timeout_add(DASH_ITEM_HOVER_TIMEOUT, > > I don't get the problem sorry? how should it look like then? You have showLabelTimeoutId = function() { /* stuff */ item.show(); } but hideLabelTimeoutId = function() { /* stuff */ }; item.hide(); The name 'hideLabelTimeoutId' suggests that the label is hidden in the anonymous function, just like it's shown in the 'showLabelTimeoutId' function. So it should either do that to match the expectation or use another name.
Created attachment 205527 [details] [review] Patch visibility timeout for labels to behave more like gtk+ Updated patch according to Florains suggestions
Review of attachment 205527 [details] [review]: See comments below, plus the commit message still uses "tooltip" and "toolbar"(???). ::: js/ui/dash.js @@ +453,3 @@ if (display.actor.get_hover() && !display.isMenuUp) { + if (this._showLabelTimeoutId == 0) { + this._showLabelTimeoutId = Mainloop.timeout_add(this._hoverTimeout, No biggie, but you could get rid of this._hoverTimeout by using this._labelShowing ? 0 : DASH_ITEM_HOVER_TIMEOUT here. @@ +463,3 @@ + Mainloop.source_remove(this._hideLabelTimeoutId); + this._hideLabelTimeoutId = 0; + item.hideLabel(); Why are you hiding the label here? I'd just expect something like if (this._hideLabelTimeoutId > 0) Mainloop.source_remove(this._hideLabelTimeoutId); this._hideLabelTimeoutId = 0; @@ +470,3 @@ + Mainloop.source_remove(this._showLabelTimeoutId); + this._showLabelTimeoutId = 0; + item.hideLabel(); ... and another instance of hideLabel(); apparently this one is necessary, so you should not touch the original code (with the exception of renaming labelTimeoutId => showLabelTimeoutId) @@ +477,3 @@ + this._hoverTimeout = DASH_ITEM_HOVER_TIMEOUT; + this._labelShowing = false; + item.hideLabel(); OK, so when I said that the timeout ID should match the functionality in the anonymous function, I didn't mean to add calls to hideLabel() all over the place - we should only call it once, and as we already need to hide it above, the call here is redundant. In that case, _hideLabelTimeoutId is misleading, so I'd suggest _resetHoverTimeoutId instead (I'm also not entirely sold on reusing DASH_ITEM_HOVER_TIMEOUT here for "the time between hiding the label and considering it hidden", but I can live with it)
Created attachment 205702 [details] [review] Added a Dashboard for new tabs and zeitgeist powered searching
Created attachment 205703 [details] [review] Patch to place tooltips beside buttons (for the dash)
Review of attachment 205703 [details] [review]: Mostly good; apart from the comments below, you shouldn't indent the commit message ::: js/ui/dash.js @@ +301,3 @@ + this._showLabelTimeoutId = 0; + this._resetHoverTimeoutId = 0; + this._hoverTimeout = DASH_ITEM_HOVER_TIMEOUT; Never read => remove @@ +454,3 @@ + if (this._showLabelTimeoutId == 0) { + this._showLabelTimeoutId = Mainloop.timeout_add( + this._labelShowing ? 0 : DASH_ITEM_HOVER_TIMEOUT, "Weird" indentation style; if you want to avoid an overly long line (which I appreciate btw, but we don't have a hard limit), you can use a local variable: let timeout = this._labelShowing ? 0 : HOVER_TIMEOUT; this._showLableTimeoutId = Mainloop.timeout_add(timeout, ... @@ +470,3 @@ + Mainloop.source_remove(this._showLabelTimeoutId); + this._showLabelTimeoutId = 0; + item.hideLabel(); Please move that out of the block as mentioned in the last review: if (id > 0) Mainloop.source_remove(id); id = 0; item.hideLabel();
Created attachment 205722 [details] [review] Patch visibility timeout for labels to behave more like gtk+
Review of attachment 205722 [details] [review]: Almost ... ::: js/ui/dash.js @@ +466,3 @@ } else { + if (this._showLabelTimeoutId > 0) { + Mainloop.source_remove(this._showLabelTimeoutId); You changed the indent, but not the scope (note the brackets); OK with this fix.
Created attachment 205724 [details] [review] Patch visibility timeout for labels to behave more like gtk+
Review of attachment 205724 [details] [review]: Go for it!
I dont have my git gnome stuff set up here... Can you push it?
(In reply to comment #24) > I dont have my git gnome stuff set up here... Can you push it? Sure.