Created attachment 203603 [details] [review]
Patch to place tooltips beside buttons (for the dash)

So I have a very small fix for this... However this will affect all tooltips. So maybe a more generic solution that would allow us to place the tooltip below/left/right/over is better. However I am not sure this is necessary though since the dash is the only thing that uses tooltips AFAICT.
Cheers
Seif

Review of attachment 203603 [details] [review]:

Beside from the comment below, the tooltip code in general is pretty broken (note a whole load of Clutter warnings caused by our tooltips) - I suspect that it'll be much cleaner to move the dash "tooltips" to javascript and kill this class altogether.

::: src/st/st-tooltip.c
@@ +303,2 @@
   /* attempt to place the tooltip */
+  tooltip_x = (int)(tip_area->x) + tip_area->width + 9; // 9px offset to the border

We shouldn't hardcode 9 pixels. More importantly, this position does not work at all for RTL locales (where the dash is located on the right monitor edge)

Created attachment 203625 [details] [review]
Patch to place tooltips beside buttons (for the dash)

I reworked it on the js side. The patch is not ready for merging but I need some help with:
1) The general direction of the patch. Is this what you meant?
2) The math in _showTooltip in dash.js

Yay, thanks for the patch Seif! The placement looks good from a design point of view.

It would be good to be able to set the offset in the theme...

Created attachment 203705 [details] [review]
Created new Label based tooltip inside every DashItemContainer. The tooltip now show beside the DashItemContainer 500ms upon hovering. The tooltip does now  show up over the menu.

Created new Label based tooltip inside every DashItemContainer. The tooltip now show beside the DashItemContainer 500ms upon hovering. The tooltip does now show up over the menu.

@Allan: The offset is now configurable via the css
@Florian: I changed the patch completely from scratch. It is much cleaner now and Everything is packed inside the DashItemContainer except for calling the "tooltips" to be shown is done in the Dash.

Created attachment 203732 [details] [review]
theme patch for labels

Thanks for the updated patch Seif. I've attached some changes for the css - feel free to squash them into what you're doing.

I'm still not seeing the fade that was discussed in bug 654845, or are my eyes deceiving me?

Awesome. I did not do the fade yet. I will finish it tonight. I just need to know the general direction if it is right or wrong

(In reply to comment #7)
> Awesome. I did not do the fade yet. I will finish it tonight. I just need to
> know the general direction if it is right or wrong

Looks good design wise. :)

Created attachment 203742 [details] [review]
tooltip js patch 

Created new Label based tooltip inside every DashItemContainer:
1) The tooltip now show beside the DashItemContainer 500ms upon hovering.
2) The tooltip does not show up over the menu. 
3) Hide tooltips upon mouse click.
4) Hiding and showing are done via fade effects
5) Use Allan's CSS style

(In reply to comment #9)
> Created an attachment (id=203742) [details] [review]
> tooltip js patch 
> 
> Created new Label based tooltip inside every DashItemContainer:
> 1) The tooltip now show beside the DashItemContainer 500ms upon hovering.
> 2) The tooltip does not show up over the menu. 
> 3) Hide tooltips upon mouse click.
> 4) Hiding and showing are done via fade effects
> 5) Use Allan's CSS style

It needs to be faster - display timeout to 300ms and fade time to 150ms

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=203742) [details] [review] [details] [review]
> > tooltip js patch 
> > 
> > Created new Label based tooltip inside every DashItemContainer:
> > 1) The tooltip now show beside the DashItemContainer 500ms upon hovering.
> > 2) The tooltip does not show up over the menu. 
> > 3) Hide tooltips upon mouse click.
> > 4) Hiding and showing are done via fade effects
> > 5) Use Allan's CSS style
> 
> It needs to be faster - display timeout to 300ms and fade time to 150ms

So we have 3 timeouts:
1) timeout for the "animte tooltip to start" = 500ms
2) timeout for the "tooltip to animate(show)" upon hover = 300ms
3) timeout for the "tooltip to animate(hide)" upon hovering out = 300ms

How do you want it exactly?

(In reply to comment #11)
...
> So we have 3 timeouts:
> 1) timeout for the "animte tooltip to start" = 500ms
> 2) timeout for the "tooltip to animate(show)" upon hover = 300ms
> 3) timeout for the "tooltip to animate(hide)" upon hovering out = 300ms
> 
> How do you want it exactly?

1) 300ms
2) 150ms
3) 100ms

:)

Created attachment 203796 [details] [review]
Patch to place tooltips beside buttons (for the dash)

ok fixed timeouts

Review of attachment 203796 [details] [review]:

Slowly getting there; as mentioned on IRC, I wouldn't call the item label "tooltip" - it's particularly confusing as we have a widget called "Tooltip" (not to mention that the behavior *is* (subtly) different from a generic tooltip).

On the nitpick front, the commit message is messed up, and there are whitespace changes Linus dislikes (which means that git is anal about them) ...

::: js/ui/appDisplay.js
@@ +552,3 @@
         }
 
+        this.actor.set_hover(false);

Uh? I don't think we want a change from 'true' to 'false' here - note the unintended style change you are introducing with this patch!

::: js/ui/dash.js
@@ +34,3 @@
         this.actor._delegate = this;
 
+        this.tooltip = null;

Why is this public?

@@ +35,3 @@
 
+        this.tooltip = null;
+        this._tooltip_text = null;

camelCase in JS.

@@ +89,3 @@
     },
 
+    removeTooltip: function() {

Unused function.

@@ +96,3 @@
+
+    showTooltip: function() {
+        if (this.tooltip != null) {

Use an early return to save one level of indentation:

if (this.tooltip == null)
    return;

// Rest of the function

@@ +103,3 @@
+            this.tooltip.show();
+            let height = this.actor.allocation.y2 - this.actor.allocation.y1;
+            let offset = Main.overview._dash.actor.allocation.y1 + height/2;

Beeeep - don't access private properties of another object!

@@ +111,3 @@
+            
+            let y = this.actor.allocation.y1 + offset - tooltipHeight/2;
+            let x = this.actor.allocation.x2;

Again: completely broken for RTL locales.

@@ +115,3 @@
+            Tweener.addTween(this.tooltip,
+                         { scale_center_x: 1,
+                           scale_center_y: 1,

This is not what you want (apart from not having any visible effect at the moment).

My guess of what you are trying to do:
 - set the scale gravity when creating the "tooltip"
 - animate the scale_x/scale_y properties

@@ +117,3 @@
+                           scale_center_y: 1,
+                           opacity: 255,
+                           time: 0.15,

Please define values like this as a constant at the top of the file (after imports), for instance ITEM_LABEL_ANIMATION_TIME

@@ +127,3 @@
+            this.tooltip = new St.BoxLayout({style_class: 'tooltip-container',
+                                         vertical: false,
+                                         clip_to_allocation: true });

So you are using a container for the gap between dash and label? That's ugly, don't do that - instead, define a custom property (-dash-label-offset or something) and use its value when positioning the label.

@@ +144,3 @@
+                       scale_center_y: 0,
+                       opacity: 0,
+                       time: 0.1,

Should be another constant

@@ +455,3 @@
+    _onHover: function (item, display) {
+        if (display.actor.get_hover()) {
+            Mainloop.timeout_add(300,

... and another contant

@@ +457,3 @@
+            Mainloop.timeout_add(300,
+                Lang.bind(this, function() { 
+                    if (display.actor.get_hover()) {

The double check for hover is ugly! Better save the id returned by timeout_add and cancel the call as necessary.

@@ +459,3 @@
+                    if (display.actor.get_hover()) {
+                        item.showTooltip();
+                    }

Callbacks for timeout_add() are expected to return a boolean - please return an explicit value rather than relying on some obscure js behavior.

@@ +462,3 @@
+                }));
+        }
+        else if (!display.actor.get_hover()) {

Wrong indentation style (should be } else { ) - and

if (foo)
  // something
else if (not foo)

 - seriously?

Review of attachment 203796 [details] [review]:

::: js/ui/appDisplay.js
@@ +552,3 @@
         }
 
+        this.actor.set_hover(false);

I prefer to remove it completly since setting it to true forces the "tooltip" to appear below the menu if clicked

::: js/ui/dash.js
@@ +103,3 @@
+            this.tooltip.show();
+            let height = this.actor.allocation.y2 - this.actor.allocation.y1;
+    showTooltip: function() {

How else can i access the _dash actor allocation. The other solution is to call the parent of the dashitemcontainer?

@@ +111,3 @@
+            
+            let y = this.actor.allocation.y1 + offset - tooltipHeight/2;
+            this.tooltip.scale_center_y = 0;

I think I will need help with that then

@@ +462,3 @@
+                }));
+        }
+                    if (display.actor.get_hover()) {

Must have missed that! Stupid me -.-

(In reply to comment #15)
> +        this.actor.set_hover(false);
> 
> I prefer to remove it completly since setting it to true forces the "tooltip"
> to appear below the menu if clicked

Without setting the tooltip text (which your patch rightfully removes) it does not do anything like that. What it does is forcing the :hover style on the item while its menu is up - your patch must not change this.


> ::: js/ui/dash.js
> @@ +103,3 @@
> +            this.tooltip.show();
> +            let height = this.actor.allocation.y2 - this.actor.allocation.y1;
> +    showTooltip: function() {
> 
> How else can i access the _dash actor allocation. The other solution is to call
> the parent of the dashitemcontainer?

Or use get_transformed_position(), or set the position from the dash


> @@ +111,3 @@
> +            
> +            let y = this.actor.allocation.y1 + offset - tooltipHeight/2;
> +            this.tooltip.scale_center_y = 0;
> 
> I think I will need help with that then

Not sure what with?

(In reply to comment #16)
> (In reply to comment #15)
> > +        this.actor.set_hover(false);
> > 
> > I prefer to remove it completly since setting it to true forces the "tooltip"
> > to appear below the menu if clicked
> 
> Without setting the tooltip text (which your patch rightfully removes) it does
> not do anything like that. What it does is forcing the :hover style on the item
> while its menu is up - your patch must not change this.
> 
> 
> > ::: js/ui/dash.js
> > @@ +103,3 @@
> > +            this.tooltip.show();
> > +            let height = this.actor.allocation.y2 - this.actor.allocation.y1;
> > +    showTooltip: function() {
> > 
> > How else can i access the _dash actor allocation. The other solution is to call
> > the parent of the dashitemcontainer?
> 
> Or use get_transformed_position(), or set the position from the dash
> 
> 
> > @@ +111,3 @@
> > +            
> > +            let y = this.actor.allocation.y1 + offset - tooltipHeight/2;
> > +            this.tooltip.scale_center_y = 0;
> > 
> > I think I will need help with that then
> 
> Not sure what with?
With RTL

Review of attachment 203796 [details] [review]:

::: js/ui/dash.js
@@ +457,3 @@
+            Mainloop.timeout_add(300,
+                Lang.bind(this, function() { 
+                    if (display.actor.get_hover()) {

I really don't understand how I should do this

(In reply to comment #17)
> > Not sure what with?
> With RTL

You can test for the text direction with

  if (this.actor.get_text_direction() == St.TextDirection.RTL)

and then position the "tooltips" accordingly (currently they show up over the workspace switcher ;-)


(In reply to comment #18)
> Review of attachment 203796 [details] [review]:
> 
> ::: js/ui/dash.js
> @@ +457,3 @@
> +            Mainloop.timeout_add(300,
> +                Lang.bind(this, function() { 
> +                    if (display.actor.get_hover()) {
> 
> I really don't understand how I should do this

There are a lot of examples in the existing code, but the general approach would be:

if (display.actor.get_hover()) {
  if (this._labelTimeoutId == 0)
    this._labelTimeoutId = Mainloop.timeout_add(...);
} else {
  if (this._labelTimeoutId > 0)
    Mainloop.source_remove(this._labelTimeoutId);
  this._hideLabel();
}

Created attachment 203810 [details] [review]
Patch to place tooltips beside buttons (for the dash)

OK fixed most issues you asked for... The only 2 remaining issues are highlighted via "TODO" and "FIXME"

(In reply to comment #20)
> Created an attachment (id=203810) [details] [review]
> Patch to place tooltips beside buttons (for the dash)
> 
> OK fixed most issues you asked for... The only 2 remaining issues are
> highlighted via "TODO" and "FIXME"

What's with the crazy new transition effect? This has way too much movement; we just need it to quickly fade in and out.

Created attachment 203888 [details] [review]
Added a Dashboard for new tabs and zeitgeist powered searching

OK added RTL and dropped the animation (only fade is used)

Created attachment 203891 [details] [review]
Patch to place tooltips beside buttons (for the dash)

Sorry wrong description

Review of attachment 203888 [details] [review]:

The positioning code needs cleanup, other than that only style comments left (also note the whitespace warnings and commit message mentioned earlier, those will need fixing as well).

::: js/ui/dash.js
@@ +98,3 @@
+        this._label.show();
+        let height = this.actor.allocation.y2 - this.actor.allocation.y1;
+        let offset = this.actor.get_parent().get_parent().allocation.y1 + height/2;

Style nit: missing spaces around operator

@@ +102,3 @@
+        // Make sure that label height is an even number so we can divide
+        let labelHeight = this._label.get_height();
+        if (labelHeight%2 == 1)

Dto.

@@ +105,3 @@
+            labelHeight = labelHeight + 1;
+
+        let y = this.actor.allocation.y1 + offset - labelHeight/2;

Dto.

@@ +108,3 @@
+        
+        let node = this._label.get_theme_node();
+        let spacing = node.get_length('x-offset');

Nit: custom properties should be prefixed with '-'

@@ +114,3 @@
+        // Check for RTL
+        if (St.Widget.get_default_direction () == St.TextDirection.RTL) {
+            let mainWidth = this.actor.get_parent().get_parent().get_parent().allocation.x2

Ugh, no - this is massively breaking encapsulation (more than your use of actor.allocation, which is not entirely correct either, as it relies on the dash's position at the screen edge).

Again: to get the position on the stage, use get_transformed_position ()

@@ +129,3 @@
+        if (this._label == null) {
+            this._label = new St.Label({ style_class: 'dash-label'});
+        }

No parentheses

@@ +138,3 @@
+        this._label.opacity = 255;
+        Tweener.addTween(this._label,
+                     { opacity: 0,

Weird indentation

@@ +143,3 @@
+                       onComplete: Lang.bind(this, function() {
+                               this._label.hide();
+                           })

wrong indent

@@ +456,3 @@
+                        item.showLabel();
+                        return false;
+                        }

indentation

Created attachment 203902 [details] [review]
Patch to place tooltips beside buttons (for the dash)

used get_transformed_postion where needed thus making code looking better... Also fixed issues raised by Florian.

Review of attachment 203902 [details] [review]:

::: js/ui/dash.js
@@ +98,3 @@
+        this._label.show();
+
+        var actorPosition = this.actor.get_transformed_position();

Don't use 'var'.

    let [actorX, actorY] = this.actor.get_transformed_position();

@@ +121,3 @@
+        this._label.set_position(x, y);
+        Tweener.addTween(this._label,
+                     { opacity: 255,

Indentation is still funny.

Created attachment 203906 [details] [review]
Patch to place tooltips beside buttons (for the dash)

fixed issues raised by Jasper

Comment on attachment 203906 [details] [review]
Patch to place tooltips beside buttons (for the dash)

(In reply to comment #27)
> fixed issues raised by Jasper

No you didn't

Created attachment 203908 [details] [review]
Patch to place tooltips beside buttons (for the dash)

Ok now its fixed

Review of attachment 203908 [details] [review]:

OK, getting there - remaining comments are mostly style nits. (Commit message is still messed up btw)

::: js/ui/appDisplay.js
@@ +536,3 @@
 
+    getMenuUp: function () {
+        return this._isMenuUp;

Just make the property public

@@ +557,3 @@
         }
 
+        this._isMenuUp = true;

Any reason for not setting the property in the 'open-state-changed' handler?

this._isMenuUp = isPoppedUp;

should cover both cases ...

::: js/ui/dash.js
@@ +98,3 @@
+        this._label.show();
+
+        let [actorX, actorY] = this.actor.get_transformed_position();

Maybe stageX, stageY (to make it clear that it's the position on the stage)?

@@ +100,3 @@
+        let [actorX, actorY] = this.actor.get_transformed_position();
+
+        let offset = (this.actor.allocation.y2 - this.actor.allocation.y1) / 2;

Hmm, doesn't make too much sense - the actual offset is "offset - labelHeight / 2". Maybe just use itemHeight here (e.g. don't divide) and use

let y = stageY + (itemHeight - labelHeight) / 2;

later. (or rather Math.floor((itemHeight - labelHeight) / 2), see below)

@@ +105,3 @@
+        let labelHeight = this._label.get_height();
+        if (labelHeight % 2 == 1)
+            labelHeight = labelHeight + 1;

What about "offset"? It's arguably cleaner to just use Math.floor(), see comment above.

@@ +110,3 @@
+
+        let node = this._label.get_theme_node();
+        let spacing = node.get_length('-x-offset');

nit: foo = get('bar') reads weird - use xOffset

@@ +112,3 @@
+        let spacing = node.get_length('-x-offset');
+
+        let x = actorX + this.actor.allocation.x2 + spacing;

this.actor.allocation.x2 should be the item's width - the two match, but we should not rely on that

@@ +116,3 @@
+        // Check for RTL
+        if (St.Widget.get_default_direction () == St.TextDirection.RTL) {
+            x = actorX - this._label.get_width() - spacing;

You shouldn't assign x twice - do

let x;
if (rtl)
  x = ...
else
  x = ...

(also: why not this._label.get_text_direction()? the comment is superfluous)

Created attachment 203909 [details] [review]
Patch to place tooltips beside buttons (for the dash)

@Florian: this._label.get_text_direction() always gives me LTR, also setting the isMenuUp property in the 'open-state-changed' handler doesn't work for me since the Menu does not disappear...

Review of attachment 203909 [details] [review]:

(In reply to comment #31)
> @Florian: setting the isMenuUp property in the 'open-state-changed' handler 
> doesn't work for me since the Menu does not disappear...

Hmm, it should work (you might need to move the calls to sync_hover()/set_hover(), as you rely on isMenuUp being set before onHover is called). Anyway, the patch looks ok as-is, provided you write a proper commit message. See those[0] examples[1].

[0] https://github.com/torvalds/subsurface/blob/master/README#L161
[1] http://lists.cairographics.org/archives/cairo/2008-September/015092.html

Created attachment 204065 [details] [review]
Patch to place tooltips beside buttons (for the dash)

Updated commit message

Pushed the patch, thanks! Feel free to close the other two bugs as obsolete/dups ...

Probably should remove the double semi-colon in stylesheet.css

+.dash-label {
+ border-radius: 7px;
+ padding: 4px 12px;
+ background-color: rgba(0,0,0,0.5);
+ color: #ffffff;
+ font-size: 0.9em;
+ font-weight: bold;;
+ text-align: center;
+ -x-offset: 8px;
+}
+