ROTranslatingShape mouse hotspot mis-alignment

[seandenigris]

Originally reported on Google Code with ID 842

Fix attached related to the following mail-posts
Also attached are before and after snapshots from ROExample>>nestingTranslate.

Ben Coman wrote:
>
> I was trying to apply ROTranslatingShape to produce a label sitting above the 
> children nodes.  To understand it better how to get what I
> need, I made ROExample>>nestingTranslate to show a permutation of the order 
> the shapes can be applied.  I have attached a screen snapshot and fileout for 
> this.  The case I am particularly interested in is el3.
> The 'el0' case is the reference without any ROTranslatingShape.
> When running the example hover near and over the inner elements to view the 
> misalignment of the mouse hotspot. I have marked the approximate offset in 
> green in the snapshot.
>
> I notice that the existing call chain goes...
> ROView>>localElementAt:
> ROElement>>elementAt:
> ROElement>>contains:
> ROElemenet>>bounds
> ROShape>>extentFor:   (union of extent of each shape)
>
> It seems to me the hotspot misalignment is related to the translation being 
> applied at the shape draw level whereas the #contains: checking is done at the 
> element level.  This relies on the union of the shape extents to set the 
> element bounds, but I can't see how that extent union could be modified to 
> account for the translation - particular in the case of a negative translation 
> since extents throw away information
> about any negative offset.
> So I thought perhaps that the element #contains: checking needs might be 
> better chained through the shaped, so that the translation
> could be applied similar to ROTranslatingShape>>chainedDrawOn:for: and that 
> this would not cause any/much additional computation since all the shapes were 
> being cycled through anyway for #extentFor:
>
> Something like...
> ROView>>localElementAt:
> ROElement>>elementAt:
> ROElement>>contains:
> ROShape>>chainedContains:For:    (for each shape)

You can see in all case el1 to el8, the child elements have shifted in relation 
to el0.
However the hotspot remains aligned with the location in el0.

I think part of the problem is that ROChildrenShape is a ROAbstractEndingShape 
and is always at the end of the line - so the translation is applied to the 
children elements.
Whereas it might work better if the position of the child elements was not 
affected by ROTranslatingShape, such that it becomes the reference for the 
position of any other translated shapes.
This would require ROChildrenShape be always the first shape of an element, 
rather than always be the last shape.
--------------------

The following code snippet...
    el := ROElement on: 1.
    (el + ROBorder + ROTranslatingShape + ROLabel ) inspect.
produces shapes chained in order of: 
ROLabel-->ROTranslatingShape-->ROBorder-->ROChildrenShape.
which seems reversed from what I would intuitively expect reading the code left 
to right.
I think ideally the shape chain would be: 
ROChildrenShape-->ROBorder-->ROTranslatingShape-->ROLabel-->RONullShape

Otherwise as in el1 you can see the position of the label is the same as el0, 
since the label has actually been drawn first prior to the translation, and 
instead the translation is applied to the border and child elements, the latter 
of which affects the mouse hotspot.

Reported by benjamin.t.coman on 2012-09-22 16:38:10

---

- _Attachment: [Roassal-BenComan.310.mcz](https://storage.googleapis.com/google-code-attachments/moose-technology/issue-842/comment-0/Roassal-BenComan.310.mcz)_ - _Attachment: ROTranslatingShape-permutations-mouse-hotspot-broken.png
_ - _Attachment: ROTranslatingShape-permutations-mouse-hotspot-fixed.png
_

[seandenigris]

Note however that while the mouse hotspot is fixed for the most inner nodes, hovering
exactly over the "el1" text does not highlight this text and its element.  I think
it should, and that there is value in further consideration of ROShape>>chainedContains:For:.


As another example of where the chained shape contains might be considered a benefit
is the following script...
| rawView view el |
rawView := ROView new.
el := ROElement new + ROBorder + (ROTranslatingShape new offset: 20 @ 20) + ROBox .

el extent: 50 @ 50.
el @ RODraggable @ ROLightlyHighlightable.
rawView add: el.
rawView open

Referring to attached image, currently the hotspot is only the bordered square and
not the filled in square.  I think ideally the hotspot should extend to the filled
square, but not the areas that I have marked with the red circles.

Reported by benjamin.t.coman on 2012-09-22 17:05:52

---

- _Attachment: ROTranslatingShape-chained-shape-contains-example.png
_

[seandenigris]

Partial fix to extend the mouse hotspot to include translated shapes with new 
ROShape>>chainedContains:for: and friends.   Check against script in Comment-1 and
ROExample>>nestingTranslate. 

Need to review hovering just above the bottom border of el7 & el8 from ROExample>>nestingTranslate.
 Note there are several debugging Transcript outputs scattered through until this is
resolved.  (Actually if anyone was interested this provides an interesting display
of how the Roassal internals work.

Reported by benjamin.t.coman on 2012-09-22 17:17:53

---

- _Attachment: [Roassal-BenComan.311.mcz](https://storage.googleapis.com/google-code-attachments/moose-technology/issue-842/comment-2/Roassal-BenComan.311.mcz)_

[seandenigris]

Just a couple peripheral things...

1. I notice when ROExample>>nestingTranslate is run by executing the script in its
comment, for half-a-second elements el2 & el3 are slightly below el0, then they shift
up to be in line with el0. 

2. ROExample>>nestingTranslate shows that the offset shapes affect the growing of the
parent element - which is really good.  However if ROHorizontalLineLayout is changed
to ROVerticalLineLayout, then it can be seen (for example with el1) that the offset
shape (the label) is not included in the default spacing between elements.  It would
be really good if that can be done.

Reported by benjamin.t.coman on 2012-09-22 17:32:30

---

- _Attachment: ROTranslatingShape-include-offset-shapes-in-spacing.png
_

[seandenigris]

Perhaps needing more consideration, previously the children were always drawn last and
ended up on top of any shapes, whereas now they get drawn first and _might_ be overwritten
by a later shape of the parent. 

Perhaps we need to be able to arrange the ROChildrenShape into the middle of the shape-chain.


Or perhaps drawing children is special and done differently to the rest of the shapes,
so that it is always drawn last but not affected by ROTranslateShape.  
Perhaps something roughly like....

ROAbstractElement>>drawOn: aCanvas
    shape chainedDrawOn: aCanvas for: self
    ROChildrenShape new drawOn: aCanvas for: self.

Reported by benjamin.t.coman on 2012-09-23 05:17:55

[seandenigris]

> Perhaps we need to be able to arrange the ROChildrenShape into the middle of the shape-chain.

Following on from this, thought I would float the idea of considering ROChildrenShape
as reference layer 0, then the current "plus sign" could be considered to be adding
shapes layered above ROChildrenShape, while the a "minus sign" might be used to add
shapes layered below ROChildrenShape (or the opposite orientation).

So then you might have a script like...
------------------
| rawView outer |
rawView := ROView new.
outer := (ROElement spriteOn: 'outer') - (ROCircle new color: (Color lightGreen alpha:
0.95)) + ROTranslateShape + ROLabel.
1 to: 4 do: [ :n| outer add: (ROElement spriteOn: n)].
ROGridLayout on: outer elements.
rawView add: outer.
rawView open.
-----------------
where the circle is drawn under the children elements, then the child elements are
drawn, then the translation is applied to the label drawn on top.

btw, what are the issues with each shape having its own offset initialized to 0@0,
or  using bounds rather than extent, where the origin could go negative?  

Reported by benjamin.t.coman on 2012-09-23 08:41:12

[seandenigris]

HOLD THE PRESS!!!  After working through all of the above getting the label translation
working at the shape level, I've come at it another way using translation at the element
level instead.  This gets me what I need to progress at the moment, allowing for some
extended time to consider the above ideas - which are fairly major changes to the architecture
but I think useful in other ways.

Please review the attached mcz containing ROExample>>nestingTranslate2, and hopefully
this is considered lower enough impact to push through quickly.

Reported by benjamin.t.coman on 2012-09-23 12:48:03

---

- _Attachment: [Roassal-BenComan.312.mcz](https://storage.googleapis.com/google-code-attachments/moose-technology/issue-842/comment-7/Roassal-BenComan.312.mcz)_ - _Attachment: ROLayout#offsetBy.png
_

[seandenigris]

I started to work on this. This is a tough one. 

Reported by alexandre.bergel on 2012-09-24 22:29:27

[seandenigris]

I know :).  Thanks for the update. I've thrown a whole lot of ideas out there. Just
a couple of minor new thoughts (not necessarily good ones) this morning...

Depending on which way to go, if you are go the way of minus/plus...
outer := (ROElement spriteOn: 'outer') - (ROCircle new color: (Color lightGreen alpha:
0.95)) + ROTranslateShape + ROLabel.

then I was not completely clear what the intuitive result would be for multiple minuses
chained together.  Does a minus always push to the front? Or does it push immediately
in front of ROChildren?  Perhaps a more intuitive way would be to use only pluses with
ROElement representing the position in the chain where the children are drawn...
outer := (ROCircle new color: (Color lightGreen alpha: 0.95)) + (ROElement spriteOn:
'outer') + ROTranslateShape + ROLabel.

Reported by benjamin.t.coman on 2012-09-24 23:55:08

[seandenigris]

btw, on review of above it looks like I didn't mention some of my musings about part
of the problem being the use of extents throwing away information about negative position
of shapes.  I thought perhaps an offset stored per shapes, perhaps considered as a
"negative extent", or shapes using bounds rather than extents might be required to
make it work. 

This consideration of "negative" translation of shapes may intertwined with "negative"
translation of elements in http://code.google.com/p/moose-technology/issues/detail?id=831

Reported by benjamin.t.coman on 2012-09-25 00:02:05

[seandenigris]

A little bit to go. With the following example, the node cannot be dragged by the blue
square. Also any part of the red square outside the black square cannot be used for
dragging.
--------------
    | el inner view |
    el := ROElement on: 1.
    el + ROBorder blue + (ROTranslatingShape offset: 20 @ 20) + ROBorder.
    el extent: 100 @ 100.
    el @ RODraggable.
    el @ ROPopup.

    inner := ROElement on: 2.
    inner + ROBorder red.
    inner extent: 50 @ 50.
    inner @ RODraggable.
    inner @ ROPopup.

    el add: inner.

    view := ROView new.
    view add: el.
    view open
---------------

Just another thought.  I wonder if some of the difficulty arises from the shape being
 drawn by one code execution path, and then the #extents and #contains being calculated
by another code execution path - and whether this could be consolidated into one place...
since by the end of the #drawOn:for: method it should know the final position and extent
that it has drawn of itself, it could be required to set a a new 'offset' and existing
;extents' ivars in ROShape using local co-ordinates.  Then all the #extent and #contains
need to do is consider those stored values rather than doing a different calculation
which currently seems not quite aligned.

Also I notice in ROElement>>elementAt: you write "that in local co-ordinates aPoint
= 0@0 refers to the top left of the [element] bounds."  I presume you mean element
bounds.  Now my thinking might be naive, but I can't help feeling that this definition
will cause difficulties.  Since a shape with negative offset will move the bounds,
so will move 0@0 and so what is the meaning of the negative offset? My thought is that
conceptually, the local co-ordinate 0@0 aligns with the "element position" where that
is not necessarily "element bounds topLeft" ...or are the issues with doing that way
that I don't see.

Reported by benjamin.t.coman on 2012-09-26 18:46:17