From a2e8f04cdf627d1ec673899289174b954b8766c0 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Wed, 14 Mar 2012 09:52:50 +0200 Subject: [PATCH] Separated Component.isEnabled/isVisible from Connector enabled state. Connector.isConnectorEnabled determines if the Connector can receive messages from its counter part. Component isEnabled/isVisible only returns the state of the component. Made Table implement HasComponents. --- .../gwt/client/ApplicationConnection.java | 3 + .../vaadin/terminal/gwt/client/Connector.java | 7 ++ .../terminal/gwt/client/ConnectorMap.java | 35 --------- .../gwt/client/ui/AbstractConnector.java | 9 +++ .../gwt/client/ui/TableConnector.java | 1 - .../terminal/gwt/client/ui/VScrollTable.java | 16 ---- .../server/AbstractCommunicationManager.java | 24 +++++- src/com/vaadin/ui/AbstractComponent.java | 73 ++++++++++++++----- src/com/vaadin/ui/AbstractSelect.java | 3 +- src/com/vaadin/ui/Component.java | 49 ++++++------- src/com/vaadin/ui/Root.java | 5 ++ src/com/vaadin/ui/Table.java | 16 +++- .../table/TableWithChildComponents.java | 66 +++++++++++++++++ 13 files changed, 207 insertions(+), 100 deletions(-) create mode 100644 tests/testbench/com/vaadin/tests/components/table/TableWithChildComponents.java diff --git a/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java b/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java index a7362cbb8c..66dcd069ab 100644 --- a/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java +++ b/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java @@ -1092,6 +1092,7 @@ public class ApplicationConnection { } private void unregisterRemovedConnectors() { + int unregistered = 0; List currentConnectors = new ArrayList( connectorMap.getConnectors()); for (ServerConnector c : currentConnectors) { @@ -1104,6 +1105,7 @@ public class ApplicationConnection { // children. The RootConnector should never be // unregistered even though it has no parent. connectorMap.unregisterConnector(cc); + unregistered++; } 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"); @@ -1112,6 +1114,7 @@ public class ApplicationConnection { } + VConsole.log("* Unregistered " + unregistered + " connectors"); } private void createConnectorsIfNeeded(ValueMap json) { diff --git a/src/com/vaadin/terminal/gwt/client/Connector.java b/src/com/vaadin/terminal/gwt/client/Connector.java index 45abe10007..59bcdd6bfe 100644 --- a/src/com/vaadin/terminal/gwt/client/Connector.java +++ b/src/com/vaadin/terminal/gwt/client/Connector.java @@ -29,4 +29,11 @@ public interface Connector { */ public String getConnectorId(); + /** + * Checks if the communicator is enabled. An enabled communicator is allowed + * to receive messages from its counter-part. + * + * @return true if the connector can receive messages, false otherwise + */ + public boolean isConnectorEnabled(); } diff --git a/src/com/vaadin/terminal/gwt/client/ConnectorMap.java b/src/com/vaadin/terminal/gwt/client/ConnectorMap.java index f5ec33de5f..79b16be3a7 100644 --- a/src/com/vaadin/terminal/gwt/client/ConnectorMap.java +++ b/src/com/vaadin/terminal/gwt/client/ConnectorMap.java @@ -7,12 +7,10 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.Iterator; import java.util.Map; import com.google.gwt.core.client.GWT; import com.google.gwt.dom.client.Element; -import com.google.gwt.user.client.ui.HasWidgets; import com.google.gwt.user.client.ui.Widget; import com.vaadin.terminal.Paintable; import com.vaadin.terminal.gwt.client.RenderInformation.Size; @@ -218,39 +216,6 @@ public class ConnectorMap { return result.toArray(new ComponentConnector[result.size()]); } - /** - * Unregisters the child connectors for the given container recursively. - * - * Use when after removing a connector that contains other connectors. Does - * not unregister the given container itself. Does not actually remove the - * widgets from the DOM. - * - * @see #unregisterConnector(ServerConnector) - * @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(); - while (it.hasNext()) { - final Widget w = it.next(); - ComponentConnector p = getConnector(w); - if (p != null) { - // This will unregister the paintable and all its children - idToComponentDetail.remove(p.getConnectorId()); - idToConnector.remove(p.getConnectorId()); - } - - if (w instanceof HasWidgets) { - // For normal widget containers, unregister the children - unregisterChildConnectors((HasWidgets) w); - } - } - } - /** * FIXME: Should not be here * diff --git a/src/com/vaadin/terminal/gwt/client/ui/AbstractConnector.java b/src/com/vaadin/terminal/gwt/client/ui/AbstractConnector.java index 8ba2bb4d31..84ccfeadaa 100644 --- a/src/com/vaadin/terminal/gwt/client/ui/AbstractConnector.java +++ b/src/com/vaadin/terminal/gwt/client/ui/AbstractConnector.java @@ -123,4 +123,13 @@ public abstract class AbstractConnector implements ServerConnector { return (Collection) rpcImplementations.get(rpcInterfaceId); } + /* + * (non-Javadoc) + * + * @see com.vaadin.terminal.gwt.client.Connector#isConnectorEnabled() + */ + public boolean isConnectorEnabled() { + // Client side can always receive message from the server + return true; + } } diff --git a/src/com/vaadin/terminal/gwt/client/ui/TableConnector.java b/src/com/vaadin/terminal/gwt/client/ui/TableConnector.java index 0d17a7ccc2..eae0cd61ee 100644 --- a/src/com/vaadin/terminal/gwt/client/ui/TableConnector.java +++ b/src/com/vaadin/terminal/gwt/client/ui/TableConnector.java @@ -182,7 +182,6 @@ public class TableConnector extends AbstractComponentContainerConnector } getWidget().hideScrollPositionAnnotation(); - getWidget().purgeUnregistryBag(); // selection is no in sync with server, avoid excessive server visits by // clearing to flag used during the normal operation diff --git a/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java b/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java index 9166ffdf9f..e589b8eead 100644 --- a/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java +++ b/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java @@ -430,7 +430,6 @@ public class VScrollTable extends FlowPanel implements HasWidgets, */ boolean recalcWidths = false; - private final ArrayList lazyUnregistryBag = new ArrayList(); boolean rendering = false; private boolean hasFocus = false; private int dragmode; @@ -826,7 +825,6 @@ public class VScrollTable extends FlowPanel implements HasWidgets, void initializeRows(UIDL uidl, UIDL rowData) { if (scrollBody != null) { scrollBody.removeFromParent(); - lazyUnregistryBag.add(scrollBody); } scrollBody = createScrollBody(); @@ -1106,19 +1104,6 @@ public class VScrollTable extends FlowPanel implements HasWidgets, } } - /** - * Unregisters Paintables in "trashed" HasWidgets (IScrollTableBodys or - * IScrollTableRows). This is done lazily as Table must survive from - * "subtreecaching" logic. - */ - void purgeUnregistryBag() { - for (Iterator iterator = lazyUnregistryBag.iterator(); iterator - .hasNext();) { - ConnectorMap.get(client).unregisterChildConnectors(iterator.next()); - } - lazyUnregistryBag.clear(); - } - void updateActionMap(UIDL mainUidl) { UIDL actionsUidl = mainUidl.getChildByTagName("actions"); if (actionsUidl == null) { @@ -4136,7 +4121,6 @@ public class VScrollTable extends FlowPanel implements HasWidgets, Element td = toBeRemoved.getElement().getChild(i).cast(); client.registerTooltip(VScrollTable.this, td, null); } - lazyUnregistryBag.add(toBeRemoved); tBodyElement.removeChild(toBeRemoved.getElement()); orphan(toBeRemoved); renderedRows.remove(index); diff --git a/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java b/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java index c509cbb33e..fc5f79a2e7 100644 --- a/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java +++ b/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java @@ -61,6 +61,7 @@ import com.vaadin.terminal.VariableOwner; import com.vaadin.terminal.WrappedRequest; import com.vaadin.terminal.WrappedResponse; import com.vaadin.terminal.gwt.client.ApplicationConnection; +import com.vaadin.terminal.gwt.client.Connector; import com.vaadin.terminal.gwt.client.communication.MethodInvocation; import com.vaadin.terminal.gwt.client.communication.SharedState; import com.vaadin.terminal.gwt.server.BootstrapHandler.BootstrapContext; @@ -951,7 +952,7 @@ public abstract class AbstractCommunicationManager implements JSONArray children = new JSONArray(); for (Component child : getChildComponents(parent)) { - if (child.isVisible()) { + if (isVisible(child)) { String childConnectorId = getPaintableId(child); children.put(childConnectorId); } @@ -1160,6 +1161,15 @@ public abstract class AbstractCommunicationManager implements } } + private boolean isVisible(Component child) { + HasComponents parent = (HasComponents) child.getParent(); + if (parent == null || !child.isVisible()) { + return child.isVisible(); + } + + return parent.isComponentVisible(child) && isVisible(parent); + } + private static class NullIterator implements Iterator { public boolean hasNext() { @@ -1458,7 +1468,15 @@ public abstract class AbstractCommunicationManager implements final VariableOwner owner = getVariableOwner(invocation .getConnectorId()); - if (owner != null && owner.isEnabled()) { + boolean connectorEnabled; + if (owner instanceof Connector) { + connectorEnabled = ((Connector) owner).isConnectorEnabled(); + } else { + // TODO Remove + connectorEnabled = owner.isEnabled(); + } + + if (owner != null && connectorEnabled) { VariableChange change = new VariableChange(invocation); // TODO could optimize with a single value map if only one @@ -2050,7 +2068,7 @@ public abstract class AbstractCommunicationManager implements if (componentsRoot != r) { resultset.remove(p); } else if (component.getParent() != null - && !component.getParent().isVisible()) { + && !isVisible(component.getParent())) { /* * Do not return components in an invisible subtree. * diff --git a/src/com/vaadin/ui/AbstractComponent.java b/src/com/vaadin/ui/AbstractComponent.java index b9e8f18368..9665cf011a 100644 --- a/src/com/vaadin/ui/AbstractComponent.java +++ b/src/com/vaadin/ui/AbstractComponent.java @@ -68,7 +68,7 @@ public abstract class AbstractComponent implements Component, MethodEventSource /** * The container this component resides in. */ - private Component parent = null; + private HasComponents parent = null; /** * The EventRouter used for the event model. @@ -360,17 +360,18 @@ public abstract class AbstractComponent implements Component, MethodEventSource } /* - * Tests if the component is enabled or not. Don't add a JavaDoc comment - * here, we use the default documentation from implemented interface. + * (non-Javadoc) + * + * @see com.vaadin.ui.Component#isEnabled() */ public boolean isEnabled() { - return getState().isEnabled() - && (getParent() == null || getParent().isEnabled()); + return getState().isEnabled(); } /* - * Enables or disables the component. Don't add a JavaDoc comment here, we - * use the default documentation from implemented interface. + * (non-Javadoc) + * + * @see com.vaadin.ui.Component#setEnabled(boolean) */ public void setEnabled(boolean enabled) { if (getState().isEnabled() != enabled) { @@ -379,6 +380,29 @@ public abstract class AbstractComponent implements Component, MethodEventSource } } + /* + * (non-Javadoc) + * + * @see com.vaadin.terminal.gwt.client.Connector#isConnectorEnabled() + */ + public boolean isConnectorEnabled() { + if (getParent() == null) { + // No parent -> the component cannot receive updates from the client + return false; + } else { + boolean thisEnabledAndVisible = isEnabled() && isVisible(); + if (!thisEnabledAndVisible) { + return false; + } + + if (!getParent().isConnectorEnabled()) { + return false; + } + + return getParent().isComponentVisible(this); + } + } + /* * Tests if the component is in the immediate mode. Don't add a JavaDoc * comment here, we use the default documentation from implemented @@ -409,12 +433,7 @@ public abstract class AbstractComponent implements Component, MethodEventSource * @see com.vaadin.ui.Component#isVisible() */ public boolean isVisible() { - if (getParent() == null) { - return getState().isVisible(); - } else { - return getState().isVisible() && getParent().isVisible() - && ((HasComponents) getParent()).isComponentVisible(this); - } + return getState().isVisible(); } /* @@ -423,7 +442,6 @@ public abstract class AbstractComponent implements Component, MethodEventSource * @see com.vaadin.ui.Component#setVisible(boolean) */ public void setVisible(boolean visible) { - if (getState().isVisible() != visible) { getState().setVisible(visible); // Instead of requesting repaint normally we @@ -517,7 +535,7 @@ public abstract class AbstractComponent implements Component, MethodEventSource * Gets the component's parent component. Don't add a JavaDoc comment here, * we use the default documentation from implemented interface. */ - public Component getParent() { + public HasComponents getParent() { return parent; } @@ -525,7 +543,7 @@ public abstract class AbstractComponent implements Component, MethodEventSource * Sets the parent component. Don't add a JavaDoc comment here, we use the * default documentation from implemented interface. */ - public void setParent(Component parent) { + public void setParent(HasComponents parent) { // If the parent is not changed, don't do anything if (parent == this.parent) { @@ -741,7 +759,7 @@ public abstract class AbstractComponent implements Component, MethodEventSource // Paint the contents of the component // Only paint content of visible components. - if (isVisible()) { + if (isVisibleInContext()) { if (eventIdentifiers != null) { target.addAttribute("eventListeners", @@ -763,6 +781,27 @@ public abstract class AbstractComponent implements Component, MethodEventSource repaintRequestListenersNotified = false; } + /** + * Checks if the component is visible and its parent is visible, + * recursively. + *

+ * This is only a helper until paint is moved away from this class. + * + * @return + */ + @Deprecated + protected boolean isVisibleInContext() { + HasComponents p = getParent(); + while (p != null) { + if (!p.isVisible()) { + return false; + } + p = p.getParent(); + } + // All parents visible, return this state + return isVisible(); + } + /** * Build CSS compatible string representation of height. * diff --git a/src/com/vaadin/ui/AbstractSelect.java b/src/com/vaadin/ui/AbstractSelect.java index 47f132aa77..6a6c452896 100644 --- a/src/com/vaadin/ui/AbstractSelect.java +++ b/src/com/vaadin/ui/AbstractSelect.java @@ -35,6 +35,7 @@ import com.vaadin.terminal.Resource; import com.vaadin.terminal.gwt.client.ui.dd.VIsOverId; import com.vaadin.terminal.gwt.client.ui.dd.VItemIdIs; import com.vaadin.terminal.gwt.client.ui.dd.VerticalDropLocation; +import com.vaadin.ui.AbstractSelect.ItemCaptionMode; /** *

@@ -581,7 +582,7 @@ public abstract class AbstractSelect extends AbstractField implements * to the terminal or null if no items is visible. */ public Collection getVisibleItemIds() { - if (isVisible()) { + if (isVisibleInContext()) { return getItemIds(); } return null; diff --git a/src/com/vaadin/ui/Component.java b/src/com/vaadin/ui/Component.java index d3efb74118..6ca4d5e286 100644 --- a/src/com/vaadin/ui/Component.java +++ b/src/com/vaadin/ui/Component.java @@ -205,8 +205,14 @@ public interface Component extends Connector, Paintable, VariableOwner, * component are also disabled. Components are enabled by default. * *

- * As a security feature, all variable change events for disabled components - * are blocked on the server-side. + * As a security feature, all updates for disabled components are blocked on + * the server-side. + *

+ * + *

+ * Note that this method only returns the status of the component and does + * not take parents into account. Even though this method returns true the + * component can be disabled to the user if a parent is disabled. *

* * @return true if the component and its parent are enabled, @@ -219,9 +225,6 @@ public interface Component extends Connector, Paintable, VariableOwner, * Enables or disables the component. The user can not interact disabled * components, which are shown with a style that indicates the status, * usually shaded in light gray color. Components are enabled by default. - * Children of a disabled component are automatically disabled; if a child - * component is explicitly set as disabled, changes in the disabled status - * of its parents do not change its status. * * 
      * Button enabled = new Button("Enabled");
@@ -251,27 +254,22 @@ public interface Component extends Connector, Paintable, VariableOwner,
      * 
      * 

      * Visible components are drawn in the user interface, while invisible ones
-     * are not. The effect is not merely a cosmetic CSS change, but the entire
-     * HTML element will be empty. Making a component invisible through this
-     * property can alter the positioning of other components.
-     * 

-     * 
-     * 

-     * A component is visible only if all its parents are also visible. Notice
-     * that if a child component is explicitly set as invisible, changes in the
-     * visibility status of its parents do not change its status.
+     * are not. The effect is not merely a cosmetic CSS change - no information
+     * about an invisible component will be sent to the client. The effect is
+     * thus the same as removing the component from its parent. Making a
+     * component invisible through this property can alter the positioning of
+     * other components.
      * 

      * 
      * 

-     * This method does not check whether the component is attached (see
-     * {@link #attach()}). The component and all its parents may be considered
-     * "visible", but not necessarily attached to application. To test if
-     * component will actually be drawn, check both its visibility and that
-     * {@link #getApplication()} does not return {@code null}.
+     * A component is visible only if all its parents are also visible. This is
+     * not checked by this method though, so even if this method returns true,
+     * the component can be hidden from the user because a parent is set to
+     * invisible.
      * 

      * 
-     * @return true if the component is visible in the user
-     *         interface, false if not
+     * @return true if the component has been set to be visible in
+     *         the user interface, false if not
      * @see #setVisible(boolean)
      * @see #attach()
      */
@@ -282,8 +280,9 @@ public interface Component extends Connector, Paintable, VariableOwner,
      * 
      * 

      * Visible components are drawn in the user interface, while invisible ones
-     * are not. The effect is not merely a cosmetic CSS change, but the entire
-     * HTML element will be empty.
+     * are not. The effect is not merely a cosmetic CSS change - no information
+     * about an invisible component will be sent to the client. The effect is
+     * thus the same as removing the component from its parent.
      * 

      * 
      * 
@@ -318,7 +317,7 @@ public interface Component extends Connector, Paintable, VariableOwner,
      * @return the parent component
      * @see #setParent(Component)
      */
-    public Component getParent();
+    public HasComponents getParent();
 
     /**
      * Sets the parent component of the component.
@@ -349,7 +348,7 @@ public interface Component extends Connector, Paintable, VariableOwner,
      *             if a parent is given even though the component already has a
      *             parent
      */
-    public void setParent(Component parent);
+    public void setParent(HasComponents parent);
 
     /**
      * Tests whether the component is in the read-only mode. The user can not
diff --git a/src/com/vaadin/ui/Root.java b/src/com/vaadin/ui/Root.java
index 47f962bf3d..42d921d93f 100644
--- a/src/com/vaadin/ui/Root.java
+++ b/src/com/vaadin/ui/Root.java
@@ -1562,4 +1562,9 @@ public abstract class Root extends AbstractComponentContainer implements
         }
     }
 
+    @Override
+    public boolean isConnectorEnabled() {
+        // TODO How can a Root be invisible? What does it mean?
+        return isVisible() && isEnabled();
+    }
 }
diff --git a/src/com/vaadin/ui/Table.java b/src/com/vaadin/ui/Table.java
index c9d4ca63a3..52981b875f 100644
--- a/src/com/vaadin/ui/Table.java
+++ b/src/com/vaadin/ui/Table.java
@@ -77,7 +77,7 @@ import com.vaadin.terminal.gwt.client.ui.dd.VLazyInitItemIdentifiers;
 @ClientWidget(TableConnector.class)
 public class Table extends AbstractSelect implements Action.Container,
         Container.Ordered, Container.Sortable, ItemClickSource,
-        ItemClickNotifier, DragSource, DropTarget {
+        ItemClickNotifier, DragSource, DropTarget, HasComponents {
 
     private static final Logger logger = Logger
             .getLogger(Table.class.getName());
@@ -5288,11 +5288,23 @@ public class Table extends AbstractSelect implements Action.Container,
 
     @Override
     public void setVisible(boolean visible) {
-        if (!isVisible() && visible) {
+        if (!isVisibleInContext() && visible) {
             // We need to ensure that the rows are sent to the client when the
             // Table is made visible if it has been rendered as invisible.
             setRowCacheInvalidated(true);
         }
         super.setVisible(visible);
     }
+
+    public Iterator iterator() {
+        return getComponentIterator();
+    }
+
+    public Iterator getComponentIterator() {
+        return visibleComponents.iterator();
+    }
+
+    public boolean isComponentVisible(Component childComponent) {
+        return true;
+    }
 }
diff --git a/tests/testbench/com/vaadin/tests/components/table/TableWithChildComponents.java b/tests/testbench/com/vaadin/tests/components/table/TableWithChildComponents.java
new file mode 100644
index 0000000000..1d1c162cae
--- /dev/null
+++ b/tests/testbench/com/vaadin/tests/components/table/TableWithChildComponents.java
@@ -0,0 +1,66 @@
+package com.vaadin.tests.components.table;
+
+import com.vaadin.data.Item;
+import com.vaadin.tests.components.TestBase;
+import com.vaadin.tests.util.Log;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.Button.ClickListener;
+import com.vaadin.ui.Component;
+import com.vaadin.ui.NativeButton;
+import com.vaadin.ui.Table;
+import com.vaadin.ui.Table.ColumnGenerator;
+
+public class TableWithChildComponents extends TestBase implements ClickListener {
+
+    private static final String COL2 = "Column 2 - generated";
+    private static final String COL1 = "Column 1 - components";
+    private Log log = new Log(10);
+
+    @Override
+    protected void setup() {
+        Table table = new Table();
+        table.setWidth("500px");
+        table.setPageLength(10);
+        table.addContainerProperty(COL1, Component.class, null);
+        table.addContainerProperty(COL2, Component.class, null);
+
+        table.addGeneratedColumn(COL2, new ColumnGenerator() {
+
+            public Object generateCell(Table source, Object itemId,
+                    Object columnId) {
+                return new Button(
+                        "Item id: " + itemId + " column: " + columnId,
+                        TableWithChildComponents.this);
+            }
+        });
+
+        for (int i = 0; i < 100; i++) {
+            Item item = table.addItem("Row " + i);
+            item.getItemProperty(COL1).setValue(
+                    new NativeButton("Row " + i + " native", this));
+        }
+
+        addComponent(table);
+        addComponent(log);
+
+    }
+
+    @Override
+    protected String getDescription() {
+        // TODO Auto-generated method stub
+        return null;
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        // TODO Auto-generated method stub
+        return null;
+    }
+
+    public void buttonClick(ClickEvent event) {
+        log.log("Click on " + event.getButton().getCaption());
+
+    }
+
+}
-- 
2.39.5