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.

Sign up for GitHub

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

Revert #4397 #4468

Closed
wants to merge 1 commit into from
Closed

Revert #4397 #4468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 20 additions & 27 deletions flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,25 @@ public void collectChanges(Consumer<NodeChange> collector) {
return;
}
if (isInactive()) {
if (!isInactiveSelf) {
/*
* We are here if: the node itself is not inactive but it has
* some ascendant which is inactive.
*
* In this case we send only some subset of changes (not from
* all the features). But we should send changes for all
* remaining features. Normally it automatically happens if the
* node becomes "visible". But if it was visible with some
* invisible parent then only the parent becomes dirty (when
* it's set visible) and this child will never participate in
* collection of changes since it's not marked as dirty.
*
* So here such node (which is active itself but its ascendant
* is inactive) we mark as dirty again to be able to collect its
* changes later on when its ascendant becomes active.
*/
getOwner().markAsDirty(this);
}
if (isInitialChanges) {
// send only required (reported) features updates
Stream<NodeFeature> initialFeatures = Stream
Expand Down Expand Up @@ -693,33 +712,7 @@ private Stream<NodeFeature> getDisalowFeatures() {
}

private void setInactive(boolean inactive) {
if (isInactiveSelf != inactive) {
isInactiveSelf = inactive;

visitNodeTree(child -> {
if (!this.equals(child) && !child.isInactiveSelf) {
/*
* We are here if: the child node itself is not inactive but
* it has some ascendant which is inactive.
*
* In this case we send only some subset of changes (not
* from all the features). But we should send changes for
* all remaining features. Normally it automatically happens
* if the node becomes "visible". But if it was visible with
* some invisible parent then only the parent becomes dirty
* (when it's set visible) and this child will never
* participate in collection of changes since it's not
* marked as dirty.
*
* So here such node (which is active itself but its
* ascendant is inactive) we mark as dirty again to be able
* to collect its changes later on when its ascendant
* becomes active.
*/
child.markAsDirty();
}
});
}
isInactiveSelf = inactive;
}

/**
Expand Down
39 changes: 18 additions & 21 deletions flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,32 +659,31 @@ public void collectChanges_initiallyInactiveViaParentElement_sendOnlyDisalowAndR

private void assertCollectChanges_initiallyInactive(StateNode stateNode,
ElementPropertyMap properties, Consumer<Boolean> activityUpdater) {

properties.setProperty("foo", "bar");

TestStateTree tree = (TestStateTree) stateNode.getOwner();
tree.dirtyNodes.clear();

ElementData visibility = stateNode.getFeature(ElementData.class);

activityUpdater.accept(false);

// activity updater may modify visibility of the node itself or its
// ancestor. The number of changes will depend on whether the subject
// node is visible or not
boolean visibilityChanged = !visibility.isVisible();

properties.setProperty("foo", "bar");

TestStateTree tree = (TestStateTree) stateNode.getOwner();

tree.dirtyNodes.clear();

List<NodeChange> changes = new ArrayList<>();
stateNode.collectChanges(changes::add);

if (visibilityChanged) {
Assert.assertEquals(1, tree.dirtyNodes.size());
Assert.assertThat(tree.dirtyNodes, CoreMatchers.hasItem(stateNode));
Assert.assertEquals(0, tree.dirtyNodes.size());
} else {
// the target node should be marked as dirty because it's visible
// but its parent is inactive
Assert.assertEquals(2, tree.dirtyNodes.size());
stateNode.visitNodeTree(node -> Assert.assertThat(tree.dirtyNodes,
CoreMatchers.hasItem(node)));
Assert.assertEquals(1, tree.dirtyNodes.size());
tree.dirtyNodes.contains(stateNode);
}

Assert.assertEquals(visibilityChanged ? 3 : 2, changes.size());
Expand All @@ -700,6 +699,7 @@ private void assertCollectChanges_initiallyInactive(StateNode stateNode,
Assert.assertTrue("No tag change found", tagFound.isPresent());
MapPutChange tagChange = tagFound.get();


MapPutChange change = (MapPutChange) changes.get(1);
if (visibilityChanged) {
Assert.assertThat(changes.get(2),
Expand Down Expand Up @@ -784,9 +784,6 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,

changes.clear();

TestStateTree tree = (TestStateTree) stateNode.getOwner();
tree.dirtyNodes.clear();

// now make the node inactive via the VisibiltyData

activityUpdater.accept(false);
Expand All @@ -796,6 +793,9 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,

properties.setProperty("foo", "baz");

TestStateTree tree = (TestStateTree) stateNode.getOwner();
tree.dirtyNodes.clear();

stateNode.collectChanges(changes::add);

// activity updater may modify visibility of the node itself or its
Expand All @@ -808,18 +808,16 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,

MapPutChange change;
if (visibilityChanged) {
Assert.assertEquals(1, tree.dirtyNodes.size());
Assert.assertThat(tree.dirtyNodes, CoreMatchers.hasItem(stateNode));
Assert.assertEquals(0, tree.dirtyNodes.size());
Assert.assertThat(changes.get(0),
CoreMatchers.instanceOf(MapPutChange.class));
change = (MapPutChange) changes.get(0);
Assert.assertEquals(ElementData.class, change.getFeature());
} else {
// the target node should be marked as dirty because it's visible
// but its parent is inactive
Assert.assertEquals(2, tree.dirtyNodes.size());
stateNode.visitNodeTree(node -> Assert.assertThat(tree.dirtyNodes,
CoreMatchers.hasItem(node)));
Assert.assertEquals(1, tree.dirtyNodes.size());
tree.dirtyNodes.contains(stateNode);
}

changes.clear();
Expand All @@ -843,8 +841,7 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode,
.equals(change.getFeature()) ? change
: (MapPutChange) changes.get(1);
propertyChange = change.equals(visibilityChange)
? (MapPutChange) changes.get(1)
: change;
? (MapPutChange) changes.get(1) : change;
} else {
propertyChange = change;
}
Expand Down