Adds Attach Detach Listener API for Element

[pleku]

Fixes #386

---

This change is

[Legioth]

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks broke.

---

a discussion (no related file):
If applied, this commit will

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 296 [r1] (raw file):
Confusing to have both cases in the same method when there's basically no code shared between them. Distinct method names would remove the need for remembering whether to pass true or false.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 398 [r1] (raw file):
ConcurrentModificationException e.g. if a listener removes itself when it's invoked

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 404 [r1] (raw file):
ConcurrentModificationException e.g. if a listener removes itself when it's invoked

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 59 [r1] (raw file):
Why is this an inner interface when DomEventListener is not?

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1421 [r1] (raw file):
It's the event and not the listener that is fired.

---

Comments from Reviewable

[pleku]

Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks broke.

---

a discussion (no related file):
??

---

Comments from Reviewable

[pleku]

Review status: 2 of 8 files reviewed at latest revision, 13 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 296 [r1] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 398 [r1] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 404 [r1] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 59 [r1] (raw file):
Because there is a ticket to make Component Attach & Detach listeners, which should be quite much more used than these, so having these as internal interfaces will make life easier since there won't be two different items offered by IDE autocomplition and imports...

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1421 [r1] (raw file):
Done.

---

Comments from Reviewable

[Legioth]

Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
Let's try again: If applied, this commit will <your subject line here>

From https://docs.google.com/document/d/1kZ4RUhOVf4sQsfAg5g5cXF4fibMtfM7U6FcnNVLOvV8/edit#heading=h.3133vgpjy6w4

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 316 [r2] (raw file):
This is probably the most inefficient way of doing a depth-first traversal since it means that we have to allocate memory for all nodes before we get to do anything.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 320 [r2] (raw file):
Why not just iterate the list instead of continously modifying it?

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 59 [r1] (raw file):
ElementAttachListener?

Or a generic attach listener that just doesn't give any promises about whether the source of the event is a node, element or component?

It just feels very confusing when some events are inner types and others are not.

---

Comments from Reviewable

[denis-anisimov]

Reviewed 2 of 7 files at r1, 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 163 [r2] (raw file):
Does it really matter in which order to call handleOnAttach ?
Because this method walks through the tree twice and it's not effective to do it using two traversal ways.

It seems it doesn't matter here and especially for onDetach, see below

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 171 [r2] (raw file):

I think id does matter in this order to call handleOnDetach. So only one traversal is enough here which calls two methods.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 294 [r2] (raw file):
I'm not sure but it might make sense to keep old method name which delegates to visitNodeTreeBottomUp().
Usually it doesn't matter which way to traverse a tree. So short method name as API method would be better discoverable.
And if traversal order is important then there will be these two methods.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 171 [r2] (raw file):
Sorry: I think it doesn't matter in which order to ......

---

Comments from Reviewable

[Artur-]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 163 [r2] (raw file):
Attach parent before child, detach child before parent.

In attach, you should be able to find the root (element/component/node), in detach, you should be able to find the old root (element/component/node)

---

Comments from Reviewable

[pleku]

Review status: 4 of 10 files reviewed at latest revision, 7 unresolved discussions.

---

a discussion (no related file):
Done

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 163 [r2] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 171 [r2] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 294 [r2] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 316 [r2] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 320 [r2] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 59 [r1] (raw file):
Done.

---

Comments from Reviewable

[denis-anisimov]

Reviewed 7 of 8 files at r3, 1 of 1 files at r4.
Review status: 8 of 10 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
This recursive call should introduce the issue #252 back. Do tests pass ?
Implementation should still avoid recursion and uses a stack in an appropriate way.

---

Comments from Reviewable

[pleku]

Review status: 8 of 10 files reviewed at latest revision, 3 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
Yes, tests pass. I changed to recursion since @Legioth said that should not collect all nodes in memory before visiting.

---

Comments from Reviewable

[Legioth]

Reviewed 5 of 8 files at r3, 1 of 1 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

a discussion (no related file):
Where?

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
Tests do pass since there are no torture tests for this method, only for the top-down version.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachListener.java, line 19 [r5] (raw file):
"Interface for" is redundant.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachListener.java, line 19 [r5] (raw file):
Listening for events (or listening to events)

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachListener.java, line 19 [r5] (raw file):
Same remarks as for the attach listener.

---

Comments from Reviewable

[pleku]

Review status: 7 of 10 files reviewed at latest revision, 5 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
Tests pass even if this method is triggered. So I could raise the premature optimization card here

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachListener.java, line 19 [r5] (raw file):
It is the same as in Vaadin 7

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachListener.java, line 19 [r5] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachListener.java, line 19 [r5] (raw file):
Done.

---

Comments from Reviewable

[Legioth]

Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[pleku]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
But that seems to be because the tree is built differently (wrong)

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
Right, the tests are written to test setTree() method which sets an owner "recursively" for all children already constructed tree.
So it doesn't matter on which way the tree is built. And setTree() uses stack based tree traversal algorithm which has been implemented as a fi for #252.

Tree has to be constructed in a different way to be able to test Attach/detach events .
Or tree traversal method has to be tested explicitly.
So this patch requires its own tests for big trees as well.

---

Comments from Reviewable

[Legioth]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
Or then we just accept this simple and easily readable implementation, since we don't know of any use cases where the nesting depth could be even close to 3000 since e.g. Firefox doesn't show elements that are nested more than 200 levels deep.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
In the latter case I would suggest:

• create an issue (will be internal improvement perhaps).
• make tree traversal "bottom to up" method private .
• return back short name for stack based pre-order tree traversal and keep it public.

---

Comments from Reviewable

[Artur-]

Reviewed 2 of 9 files at r1, 1 of 8 files at r2, 5 of 8 files at r3, 1 of 1 files at r4, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 305 [r6] (raw file):
"The visitor is recursively applied to the child nodes before it is applied to this node" ?

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 375 [r6] (raw file):
Should null the field again if it was the last listener

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 394 [r6] (raw file):
Should null the field again if it was the last listener

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1404 [r6] (raw file):
a UI

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1411 [r6] (raw file):
an event registration handle

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1433 [r6] (raw file):
an event registration handle

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachEvent.java, line 21 [r6] (raw file):
after ... has been

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachEvent.java, line 43 [r6] (raw file):
Why does this not override getSource instead?

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachEvent.java, line 21 [r6] (raw file):
Can you still find the UI from the element when this event is fired or not? Sounds like the element is first detached and then the event is fired?

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachEvent.java, line 43 [r6] (raw file):
Why does this not override getSource instead?

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/StateNodeTest.java, line 241 [r6] (raw file):
Would be better to use AtomicInteger and assert it has been triggered once

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/StateNodeTest.java, line 275 [r6] (raw file):
Would be better to use AtomicInteger and assert it has been triggered once

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/dom/ElementTest.java, line 1625 [r6] (raw file):
or incrementAndGet()

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/dom/ElementTest.java, line 1629 [r6] (raw file):
Use separate trackers to ensure both have been triggered instead of either one triggered twice

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/dom/ElementTest.java, line 1777 [r6] (raw file):
Add tests to verify that you can find the UI in both attach and detach listeners

---

Comments from Reviewable

[pleku]

Review status: 8 of 10 files reviewed at latest revision, 15 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 311 [r4] (raw file):
This version works with any sensible and not sensible amount of data. I did however reduce the depth used in the tests since IMO it makes no sense to test (or even support) something that is never used just for the sake of it.

---

Comments from Reviewable

[Legioth]

Reviewed 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[denis-anisimov]

Reviewed 3 of 3 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[pleku]

Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks broke.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 305 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 375 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 394 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1404 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1411 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/Element.java, line 1433 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachEvent.java, line 21 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementAttachEvent.java, line 43 [r6] (raw file):
Done.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachEvent.java, line 21 [r6] (raw file):
For what purpose?

The UI thread local should be enough unless there is a specific use case for that

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachEvent.java, line 43 [r6] (raw file):
Done.

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/dom/ElementTest.java, line 1629 [r6] (raw file):
Done.

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/dom/ElementTest.java, line 1777 [r6] (raw file):
Added tests for finding StateTree, which should be enough for now. Denis adds patch for getting UI for StateTree.

---

Comments from Reviewable

[pleku]

Review status: 5 of 10 files reviewed at latest revision, 13 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachEvent.java, line 21 [r6] (raw file):
For now the StateTree is accessible (there is tests), and soon(ish) the UI will be accesible from the StateTree.

---

Comments from Reviewable

[Legioth]

Reviewed 2 of 8 files at r3, 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[Artur-]

Reviewed 2 of 8 files at r3, 1 of 2 files at r7, 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 320 [r7] (raw file):
Could rewrite to do
this.forEachChild
and
previousParent=this before the loop and avoid this "problem"

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/dom/ElementDetachEvent.java, line 21 [r6] (raw file):
StateTree or even parent is good enough, just wanted to make sure that the element has context

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/dom/ElementTest.java, line 1629 [r6] (raw file):
Can't see what checks that both attach listeners have been triggered and not e.g. child listener twice and grandChild listener never

---

Comments from Reviewable

[pleku]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

hummingbird-server/src/main/java/com/vaadin/hummingbird/StateNode.java, line 320 [r7] (raw file):
You're right, good insight, but

Lots of unnecessary code added with that only because sonarcube can't understand the contracts of APIs. No thanks I won't.

---

hummingbird-server/src/test/java/com/vaadin/hummingbird/dom/ElementTest.java, line 1629 [r6] (raw file):
It was indirectly tested last when the other listener was removed and tested that only one was triggered but now it is done each step

---

Comments from Reviewable

[Artur-]

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

Comments from Reviewable

[Artur-]

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable