8500 Let the framework handle unregistration of Connectors

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<ServerConnector> currentConnectors = new ArrayList<ServerConnector>(
+                        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() {

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<String> unregistryBag = new HashSet<String>();
-
     /**
      * 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<ComponentConnector> result = new ArrayList<ComponentConnector>();
 
         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<Widget> 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);
             }

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();

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 extends ServerRpc> T initRPC(T clientToServerRpc) {
-        ((InitializableClientToServerRpc) clientToServerRpc).initRpc(getConnectorId(),
-                getConnection());
+        ((InitializableClientToServerRpc) clientToServerRpc).initRpc(
+                getConnectorId(), getConnection());
         return clientToServerRpc;
     }
 

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);
-            }
-        }
-    }
 }