From 7ab4e3ecf7ae25417f2b784f8afa5c18e330a428 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Tue, 13 Mar 2012 17:54:03 +0200 Subject: [PATCH] 8500 Let the framework handle unregistration of Connectors --- .../gwt/client/ApplicationConnection.java | 62 +++++++++--- .../terminal/gwt/client/ConnectorMap.java | 97 +++++++------------ .../terminal/gwt/client/LayoutManager.java | 4 +- .../client/ui/AbstractComponentConnector.java | 8 +- .../gwt/client/ui/PanelConnector.java | 26 +---- 5 files changed, 96 insertions(+), 101 deletions(-) diff --git a/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java b/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java index 24b0bca664..a6ef93854e 100644 --- a/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java +++ b/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java @@ -1030,6 +1030,7 @@ public class ApplicationConnection { json.getValueMap("dd")); } + unregisterRemovedConnectors(); VConsole.log("updateFromUidl: " + updateDuration.elapsedMillis() + " ms"); @@ -1077,16 +1078,6 @@ public class ApplicationConnection { } } - if (repaintAll) { - /* - * idToPaintableDetail is already cleanded at the start of - * the changeset handling, bypass cleanup. - */ - connectorMap.purgeUnregistryBag(false); - } else { - connectorMap.purgeUnregistryBag(true); - } - // TODO build profiling for widget impl loading time final long prosessingTime = (new Date().getTime()) @@ -1100,6 +1091,29 @@ public class ApplicationConnection { } + private void unregisterRemovedConnectors() { + List currentConnectors = new ArrayList( + connectorMap.getConnectors()); + for (ServerConnector c : currentConnectors) { + if (c instanceof ComponentConnector) { + ComponentConnector cc = (ComponentConnector) c; + if (cc.getParent() == null + && !(cc instanceof RootConnector)) { + // The connector has been detached from the + // hierarchy, unregister it and any possible + // children. The RootConnector should never be + // unregistered even though it has no parent. + connectorMap.unregisterConnector(cc); + } else if (cc.getParent() != null + && !cc.getParent().getChildren().contains(cc)) { + VConsole.error("ERROR: Connector is connected to a parent but the parent does not contain the connector"); + } + } + + } + + } + private void createConnectorsIfNeeded(ValueMap json) { VConsole.log(" * Creating connectors (if needed)"); @@ -1244,6 +1258,16 @@ public class ApplicationConnection { .get(connectorIndex); ComponentConnector childConnector = (ComponentConnector) connectorMap .getConnector(childConnectorId); + if (childConnector == null) { + VConsole.error("Hierarchy claims that " + + childConnectorId + " is a child for " + + connectorId + " (" + + connector.getClass().getName() + + ") but no connector with id " + + childConnectorId + + " has been registered"); + continue; + } newChildren.add(childConnector); if (childConnector.getParent() != ccc) { // Avoid extra calls to setParent @@ -1270,6 +1294,20 @@ public class ApplicationConnection { event.setParent(ccc); ccc.setChildren((List) newChildren); events.add(event); + + // Remove parent for children that are no longer + // attached to this (avoid updating children if they + // have already been assigned to a new parent) + for (ComponentConnector oldChild : oldChildren) { + if (oldChild.getParent() != ccc) { + continue; + } + + // TODO This could probably be optimized + if (!newChildren.contains(oldChild)) { + oldChild.setParent(null); + } + } } catch (final Throwable e) { VConsole.error(e); } @@ -2156,7 +2194,9 @@ public class ApplicationConnection { @Deprecated public void unregisterPaintable(ServerConnector p) { - connectorMap.unregisterConnector(p); + System.out.println("unregisterPaintable (unnecessarily) called for " + + p.getConnectorId()); + // connectorMap.unregisterConnector(p); } public VTooltip getVTooltip() { diff --git a/src/com/vaadin/terminal/gwt/client/ConnectorMap.java b/src/com/vaadin/terminal/gwt/client/ConnectorMap.java index 1ec3b84eeb..f5ec33de5f 100644 --- a/src/com/vaadin/terminal/gwt/client/ConnectorMap.java +++ b/src/com/vaadin/terminal/gwt/client/ConnectorMap.java @@ -7,10 +7,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.Map; -import java.util.Set; import com.google.gwt.core.client.GWT; import com.google.gwt.dom.client.Element; @@ -31,8 +29,6 @@ public class ConnectorMap { private final ComponentDetailMap idToComponentDetail = ComponentDetailMap .create(); - private Set unregistryBag = new HashSet(); - /** * Returns a {@link ServerConnector} by its id * @@ -174,86 +170,54 @@ public class ConnectorMap { * the connector to remove */ public void unregisterConnector(ServerConnector connector) { - - // add to unregistry queue - if (connector == null) { - VConsole.error("WARN: Trying to unregister null connector"); + VConsole.error("Trying to unregister null connector"); return; } - String id = connector.getConnectorId(); + + String connectorId = connector.getConnectorId(); + VConsole.log("Unregistering connector " + connectorId + " (" + + connector.getClass().getName() + ")"); + + // Warn if widget is still attached to DOM. It should never be at this + // point. Widget widget = null; if (connector instanceof ComponentConnector) { widget = ((ComponentConnector) connector).getWidget(); } - if (id == null) { - VConsole.log("Tried to unregister a " - + connector.getClass().getName() + " (" + id - + ") which was not registered"); - } else { - unregistryBag.add(id); - } - if (widget != null && widget instanceof HasWidgets) { - unregisterChildConnectors((HasWidgets) widget); + if (widget != null && widget.isAttached()) { + VConsole.log("Widget for unregistered connector " + connectorId + + " is still attached to the DOM."); } + idToComponentDetail.remove(connectorId); + idToConnector.remove(connectorId); + if (connector instanceof ComponentContainerConnector) { + for (ComponentConnector child : ((ComponentContainerConnector) connector) + .getChildren()) { + unregisterConnector(child); + } + } } - public ComponentConnector[] getRegisteredComponentConnectors() { + /** + * Gets all registered {@link ComponentConnector} instances + * + * @return An array of all registered {@link ComponentConnector} instances + */ + public ComponentConnector[] getComponentConnectors() { ArrayList result = new ArrayList(); for (ServerConnector connector : getConnectors()) { if (connector instanceof ComponentConnector) { - ComponentConnector componentConnector = (ComponentConnector) connector; - if (!unregistryBag.contains(connector.getConnectorId())) { - result.add(componentConnector); - } + result.add((ComponentConnector) connector); } } return result.toArray(new ComponentConnector[result.size()]); } - void purgeUnregistryBag(boolean unregisterConnectors) { - if (unregisterConnectors) { - for (String connectorId : unregistryBag) { - // TODO purge shared state for pid - ServerConnector connector = getConnector(connectorId); - if (connector == null) { - /* - * this should never happen, but it does :-( See e.g. - * com.vaadin.tests.components.accordion.RemoveTabs (with - * test script) - */ - VConsole.error("Tried to unregister component (id=" - + connectorId - + ") that is never registered (or already unregistered)"); - continue; - } - VConsole.log("Unregistering connector " - + connector.getClass().getName() + " " + connectorId); - Widget widget = null; - if (connector instanceof ComponentConnector) { - widget = ((ComponentConnector) connector).getWidget(); - } - - // check if can be cleaned - if (widget == null || !widget.isAttached()) { - // clean reference to paintable - idToComponentDetail.remove(connectorId); - idToConnector.remove(connectorId); - } - /* - * else NOP : same component has been reattached to another - * parent or replaced by another component implementation. - */ - } - } - - unregistryBag.clear(); - } - /** * Unregisters the child connectors for the given container recursively. * @@ -265,7 +229,9 @@ public class ConnectorMap { * @param container * The container that contains the connectors that should be * unregistered + * @deprecated Only here for now to support the broken VScrollTable behavior */ + @Deprecated public void unregisterChildConnectors(HasWidgets container) { // FIXME: This should be based on the paintable hierarchy final Iterator it = container.iterator(); @@ -274,8 +240,11 @@ public class ConnectorMap { ComponentConnector p = getConnector(w); if (p != null) { // This will unregister the paintable and all its children - unregisterConnector(p); - } else if (w instanceof HasWidgets) { + idToComponentDetail.remove(p.getConnectorId()); + idToConnector.remove(p.getConnectorId()); + } + + if (w instanceof HasWidgets) { // For normal widget containers, unregister the children unregisterChildConnectors((HasWidgets) w); } diff --git a/src/com/vaadin/terminal/gwt/client/LayoutManager.java b/src/com/vaadin/terminal/gwt/client/LayoutManager.java index 8bd7a3dac7..885199b6eb 100644 --- a/src/com/vaadin/terminal/gwt/client/LayoutManager.java +++ b/src/com/vaadin/terminal/gwt/client/LayoutManager.java @@ -116,7 +116,7 @@ public class LayoutManager { ConnectorMap paintableMap = connection.getConnectorMap(); ComponentConnector[] paintableWidgets = paintableMap - .getRegisteredComponentConnectors(); + .getComponentConnectors(); int passes = 0; Duration totalDuration = new Duration(); @@ -282,7 +282,7 @@ public class LayoutManager { public void foceLayout() { ConnectorMap paintableMap = connection.getConnectorMap(); ComponentConnector[] paintableWidgets = paintableMap - .getRegisteredComponentConnectors(); + .getComponentConnectors(); for (ComponentConnector vPaintableWidget : paintableWidgets) { MeasuredSize measuredSize = getMeasuredSize(vPaintableWidget); measuredSize.setHeightNeedsUpdate(); diff --git a/src/com/vaadin/terminal/gwt/client/ui/AbstractComponentConnector.java b/src/com/vaadin/terminal/gwt/client/ui/AbstractComponentConnector.java index b7aea9b735..81f0b59d06 100644 --- a/src/com/vaadin/terminal/gwt/client/ui/AbstractComponentConnector.java +++ b/src/com/vaadin/terminal/gwt/client/ui/AbstractComponentConnector.java @@ -190,7 +190,9 @@ public abstract class AbstractComponentConnector extends AbstractConnector } else { VConsole.error("Parent of connector " + getClass().getName() - + " is null. This is typically an indication of a broken component hierarchy"); + + " (" + + getConnectorId() + + ") is null. This is typically an indication of a broken component hierarchy"); } } @@ -432,8 +434,8 @@ public abstract class AbstractComponentConnector extends AbstractConnector * GWT.create(). */ protected T initRPC(T clientToServerRpc) { - ((InitializableClientToServerRpc) clientToServerRpc).initRpc(getConnectorId(), - getConnection()); + ((InitializableClientToServerRpc) clientToServerRpc).initRpc( + getConnectorId(), getConnection()); return clientToServerRpc; } diff --git a/src/com/vaadin/terminal/gwt/client/ui/PanelConnector.java b/src/com/vaadin/terminal/gwt/client/ui/PanelConnector.java index f990fc590d..8f54b13bb9 100644 --- a/src/com/vaadin/terminal/gwt/client/ui/PanelConnector.java +++ b/src/com/vaadin/terminal/gwt/client/ui/PanelConnector.java @@ -271,28 +271,12 @@ public class PanelConnector extends AbstractComponentContainerConnector @Override public void connectorHierarchyChanged(ConnectorHierarchyChangedEvent event) { super.connectorHierarchyChanged(event); + // We always have 1 child, Panel takes care of ensuring content is never + // null + ComponentConnector newChild = getChildren().get(0); + Widget newChildWidget = newChild.getWidget(); - possiblyUnregisterOldChild(event); - - // We always have 0 or 1 child - ComponentConnector newChild = null; - if (getChildren().size() != 0) { - // We now have one child - newChild = getChildren().get(0); - } - - getWidget().setWidget(newChild.getWidget()); + getWidget().setWidget(newChildWidget); } - private void possiblyUnregisterOldChild(ConnectorHierarchyChangedEvent event) { - // Did we have a child that needs to be unregistered? - if (event.getOldChildren().size() != 0) { - ComponentConnector oldChild = event.getOldChildren().get(0); - // We had a child, unregister it - // TODO Should be handled by the framework - if (oldChild.getParent() == null) { - getConnection().unregisterPaintable(oldChild); - } - } - } } -- 2.39.5