fixes #1973, also makes uidl with table more efficient. Table should now comfort with subtree caching and no hacky requestRepaints are needed for child components. May cause regressions.

svn changeset:5510/svn branch:trunk

src/com/itmill/toolkit/terminal/gwt/client/ui/IScrollTable.java

@@ -7,6 +7,7 @@ package com.itmill.toolkit.terminal.gwt.client.ui;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 import java.util.Vector;
@@ -20,6 +21,7 @@ import com.google.gwt.user.client.Timer;
 import com.google.gwt.user.client.Window;
 import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.FlowPanel;
+import com.google.gwt.user.client.ui.HasWidgets;
 import com.google.gwt.user.client.ui.Panel;
 import com.google.gwt.user.client.ui.RootPanel;
 import com.google.gwt.user.client.ui.ScrollListener;
@@ -138,6 +140,8 @@ public class IScrollTable extends Composite implements Table, ScrollListener,
      */
     boolean recalcWidths = false;
 
+    private LinkedList lazyUnregistryBag = new LinkedList();
+
     public IScrollTable() {
 
         bodyContainer.addScrollListener(this);
@@ -264,7 +268,7 @@ public class IScrollTable extends Composite implements Table, ScrollListener,
         } else {
             if (tBody != null) {
                 tBody.removeFromParent();
-                client.unregisterChildPaintables(tBody);
+                lazyUnregistryBag.add(tBody);
             }
             tBody = new IScrollTableBody();
 
@@ -277,6 +281,20 @@ public class IScrollTable extends Composite implements Table, ScrollListener,
             }
         }
         hideScrollPositionAnnotation();
+        purgeUnregistryBag();
+    }
+
+    /**
+     * Unregisters Paintables in "trashed" HasWidgets (IScrollTableBodys or
+     * IScrollTableRows). This is done lazily as Table must survive from
+     * "subtreecaching" logic.
+     */
+    private void purgeUnregistryBag() {
+        for (Iterator iterator = lazyUnregistryBag.iterator(); iterator
+                .hasNext();) {
+            client.unregisterChildPaintables((HasWidgets) iterator.next());
+        }
+        lazyUnregistryBag.clear();
     }
 
     private void updateActionMap(UIDL c) {
@@ -1857,7 +1875,7 @@ public class IScrollTable extends Composite implements Table, ScrollListener,
             }
             final IScrollTableRow toBeRemoved = (IScrollTableRow) renderedRows
                     .get(index);
-            client.unregisterChildPaintables(toBeRemoved);
+            lazyUnregistryBag.add(toBeRemoved);
             DOM.removeChild(tBody, toBeRemoved.getElement());
             orphan(toBeRemoved);
             renderedRows.remove(index);
@@ -2096,6 +2114,9 @@ public class IScrollTable extends Composite implements Table, ScrollListener,
                 }
                 DOM.appendChild(td, container);
                 DOM.appendChild(getElement(), td);
+                // ensure widget not attached to another element (possible tBody
+                // change)
+                w.removeFromParent();
                 DOM.appendChild(container, w.getElement());
                 adopt(w);
                 childWidgets.add(w);
@@ -2106,8 +2127,15 @@ public class IScrollTable extends Composite implements Table, ScrollListener,
             }
 
             public boolean remove(Widget w) {
-                // TODO Auto-generated method stub
-                return false;
+                if (childWidgets.contains(w)) {
+                    orphan(w);
+                    DOM.removeChild(DOM.getParent(w.getElement()), w
+                            .getElement());
+                    childWidgets.remove(w);
+                    return true;
+                } else {
+                    return false;
+                }
             }
 
             private void handleClickEvent(Event event) {

src/com/itmill/toolkit/ui/Table.java

@@ -1233,6 +1233,8 @@ public class Table extends AbstractSelect implements Action.Container,
             Object[][] cells = new Object[cols + CELL_FIRSTCOL][rows];
             if (rows == 0) {
                 pageBuffer = cells;
+                unregisterPropertiesAndComponents(oldListenedProperties,
+                        oldVisibleComponents);
                 return;
             }
 
@@ -1357,30 +1359,45 @@ public class Table extends AbstractSelect implements Action.Container,
             // Saves the results to internal buffer
             pageBuffer = cells;
 
-            if (oldVisibleComponents != null) {
-                for (final Iterator i = oldVisibleComponents.iterator(); i
-                        .hasNext();) {
-                    Component c = (Component) i.next();
-                    if (!visibleComponents.contains(c)) {
-                        c.setParent(null);
-                    }
+            unregisterPropertiesAndComponents(oldListenedProperties,
+                    oldVisibleComponents);
+
+            requestRepaint();
+        }
+
+    }
+
+    /**
+     * Helper method to remove listeners and maintain correct component
+     * hierarchy. Detaches properties and components if those are no more
+     * rendered in client.
+     * 
+     * @param oldListenedProperties
+     *            set of properties that where listened in last render
+     * @param oldVisibleComponents
+     *            set of components that where attached in last render
+     */
+    private void unregisterPropertiesAndComponents(
+            HashSet oldListenedProperties, HashSet oldVisibleComponents) {
+        if (oldVisibleComponents != null) {
+            for (final Iterator i = oldVisibleComponents.iterator(); i
+                    .hasNext();) {
+                Component c = (Component) i.next();
+                if (!visibleComponents.contains(c)) {
+                    c.setParent(null);
                 }
             }
+        }
 
-            if (oldListenedProperties != null) {
-                for (final Iterator i = oldListenedProperties.iterator(); i
-                        .hasNext();) {
-                    Property.ValueChangeNotifier o = (ValueChangeNotifier) i
-                            .next();
-                    if (!listenedProperties.contains(o)) {
-                        o.removeListener(this);
-                    }
+        if (oldListenedProperties != null) {
+            for (final Iterator i = oldListenedProperties.iterator(); i
+                    .hasNext();) {
+                Property.ValueChangeNotifier o = (ValueChangeNotifier) i.next();
+                if (!listenedProperties.contains(o)) {
+                    o.removeListener(this);
                 }
             }
-
-            requestRepaint();
         }
-
     }
 
     /**
@@ -1565,6 +1582,7 @@ public class Table extends AbstractSelect implements Action.Container,
 
         // Assure visual refresh
         resetPageBuffer();
+
         enableContentRefreshing(true);
 
     }
@@ -2001,12 +2019,6 @@ public class Table extends AbstractSelect implements Action.Container,
                     if (c == null) {
                         target.addText("");
                     } else {
-                        /*
-                         * FIXME ensuring that table never gets "cached" child
-                         * paints by calling requestRepaint for child component
-                         * IScrollTable currently can's survive those.
-                         */
-                        c.requestRepaint();
                         c.paint(target);
                     }
                 } else {