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

Fixes PopupDemo regression #238

Closed

Conversation

JordanMartinez
Copy link
Contributor

Popup wouldn't reposition itself when user typed/deleted text

@TomasMikula
Copy link
Member

Here's my understanding of the problem: When positionPopup (indirectly) asks for getCellIfVisible, the cell is not yet visible, because the virtualFlow has not been laid out yet. This is a simplified view of how the layout proceeds in JavaFX (which I only found out by reading the JavaFX sources):

void layout() {
    layoutChildren(); // 1
    for(Node node: getChildren()) {
        if(node instanceof Parent) {
            ((Parent) node).layout(); // 2
        }
    }
}

virtualFlow.getCellIfVisible is called from layoutChildren (1), i.e. before virtualFlow.layout() (2).

To confirm my hypothesis that this is the cause of the problem, instead of your fix, you can try to call virtualFlow.layout() before positionPopup() and see if that fixes the problem as well.

As a general fix, though, I think methods of VirtualFlow whose result depends on the layout, should themselves call layout() as the first thing. Such methods would include getCellIfVisible, visibleCells and hit. The fact that they first call layout() should be documented in their Javadoc.

You should not be too concerned about layout() being called multiple times, because it is a no-op if the layout is not dirty (the layout() code above is simplified, in fact it also checks whether the layout is dirty).

@TomasMikula
Copy link
Member

Also, this means that the refactoring uncovered a deficiency in Flowless, rather than there being a problem with your code.

@JordanMartinez
Copy link
Contributor Author

You are correct in your hypothesis.

// changing this code
Platform.runLater(() -> positionPopup(popup, alignment, adjustment));
// to this code will correct the problem
virtualFlow.layout();
positionPopup(popup, alignment, adjustment);

@JordanMartinez
Copy link
Contributor Author

I've opened up an issue for this in Flowless. We'll fix it there.

@TomasMikula
Copy link
Member

👍

@JordanMartinez JordanMartinez deleted the popupRegressionFix branch January 11, 2016 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants