Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Component.isAttached() #7911

Closed
mvysny opened this issue Mar 27, 2020 · 10 comments · Fixed by #8810
Closed

Add Component.isAttached() #7911

mvysny opened this issue Mar 27, 2020 · 10 comments · Fixed by #8810

Comments

@mvysny
Copy link
Member

mvysny commented Mar 27, 2020

Currently in Vaadin 14.1.19 it's only possible to check whether a component is attached, by running the following method:

fun Component.isAttached(): Boolean {
    val ui: UI = ui.orElse(null) ?: return false
    return !ui.isClosing
}

It would be good to have Component.isAttached() directly provided by Vaadin.

@mvysny
Copy link
Member Author

mvysny commented Mar 27, 2020

I wonder if a component may be perceived as attached even if the UI itself is detached from the session?

@denis-anisimov
Copy link
Contributor

Component::getElement::getNode::isAttached.

I agree it's not obvious but I would avoid put everything into Component as well: it litters API and makes it's hard to find required methods.

I would close this ticket but it's up to you.
Considering the fact that this functionality already exists I'm pretty sure this will never be fixed.
So it's just a question of keeping this ticket open (since the ticket has some sense) and never done or just close it.

@mvysny
Copy link
Member Author

mvysny commented Aug 3, 2020

There are the following arguments for the benefit of having such API:

  1. The Component class itself implements both AttachNotifier and DetachNotifier interfaces and offers onAttach() and onDetach() functions to be overridden, but the class doesn't offer any means of checking whether the component is actually attached. The current API therefore can be perceived as incomplete; adding isAttached() function would logically complete the API.
  2. The Element.getNode() API is marked "This method is meant for internal use only." in javadoc, and so I (as a Vaadin programmer) am not even supposed to call this API.
  3. The StateNode.isAttached() is very low-level: I, as a Vaadin programmer, have no idea what the state tree is and what it means to be attached to one. I think in terms of Components, not StateNodes.
  4. The state of being attached could mean one thing for the StateNode, and something completely else for a Component. Having explicit Component.isAttached() method would clear up things.

Additional remarks:

  • the 'being attached' state is not well defined: e.g. is the component attached when the UI is closed? This is something that would be good to document in AttachNotifier and DetachNotifier.

I therefore insist on keeping the bug open as a minimum; moreover I think the API would be beneficial and should be implemented.

@denis-anisimov
Copy link
Contributor

Alright. The ticket makes sense.
Even though I disagree with it, so let's keep it open.

@mvysny
Copy link
Member Author

mvysny commented Aug 4, 2020

Thank you.

The Component::getElement::getNode::isAttached returns true during the onDetach() function / detach listener call. Is there a way to return false in such place please?

Hmm, it sounds like the component is not yet detached at all during the call to onDetach() - for example the getParent().isPresent() still returns true. I'd expect the node/element/component to be detached when onDetach() is called. However, this is a different issue since it affects getParent() and other calls as well. I'll open a separate bug report for that.

@denis-anisimov
Copy link
Contributor

I'm not sure.......
This is how the code is written:

onDetach();
this.parent = parent;

Technically you are right: the parent is assigned only after the detach event is fired but should be assigned before.
On the other hand there can be a reason for that and changing the behavior may cause serious regressions.

Also you don't need to check isAttached inside on detach : you know that the semantic of the method is : detached.
But I understand that in practice you have an extracted method which relies normally only on one state based on isAttached instead of several info sources so it's inconvenient.
Anyway, this is another issue . Please create another ticket.

@mvysny
Copy link
Member Author

mvysny commented Aug 4, 2020

But I understand that in practice you have an extracted method which relies normally only on one state based on isAttached instead of several info sources so it's inconvenient.

I agree, exactly my thoughts.

Anyway, this is another issue . Please create another ticket.

I agree, I've opened #8809 and tried to summarize the discussion above there.

@mvysny
Copy link
Member Author

mvysny commented Aug 18, 2021

Will this be backported to Vaadin 14? (it's marked as released but I can't see the function in Vaadin 14.6.8).

@pleku
Copy link
Contributor

pleku commented Aug 19, 2021

Hasn't been backported because nobody has just done it. It should have been though, so I'm even willing to make another Flow 2.7 beta for it.

@mvysny
Copy link
Member Author

mvysny commented Aug 19, 2021

Thank you @pleku , really appreciated 🥇

caalador pushed a commit that referenced this issue Aug 19, 2021
Closes #7911
# Conflicts:
#	flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants