Reading properties of components should not set state to dirty (#14060).

Added Automatic Testcase. The testcase needs a default constructor,
which has been added.
The test also found an edge case in Form.java which has been corrected,
as well as one missing getState(false) in AbstractMedia.

Change-Id: Id764c9e1596123015a84f6c2a9507f03bde383b1

8 files changed, 145 insertions, 34 deletions

diff --git a/server/src/com/vaadin/ui/AbstractMedia.java b/server/src/com/vaadin/ui/AbstractMedia.java
index d0ff8494f0..0bd8c3ea77 100644
--- a/server/src/com/vaadin/ui/AbstractMedia.java
+++ b/server/src/com/vaadin/ui/AbstractMedia.java
@@ -146,7 +146,7 @@ public abstract class AbstractMedia extends AbstractComponent {
      */
     public List<Resource> getSources() {
         ArrayList<Resource> sources = new ArrayList<Resource>();
-        for (URLReference ref : getState().sources) {
+        for (URLReference ref : getState(false).sources) {
             sources.add(((ResourceReference) ref).getResource());
         }
         return sources;
diff --git a/server/src/com/vaadin/ui/DragAndDropWrapper.java b/server/src/com/vaadin/ui/DragAndDropWrapper.java
index cb94a774a5..3d3356b338 100644
--- a/server/src/com/vaadin/ui/DragAndDropWrapper.java
+++ b/server/src/com/vaadin/ui/DragAndDropWrapper.java
@@ -187,6 +187,10 @@ public class DragAndDropWrapper extends CustomComponent implements DropTarget,
 
     private Set<String> sentIds = new HashSet<String>();
 
+    private DragAndDropWrapper() {
+        super();
+    }
+
     /**
      * Wraps given component in a {@link DragAndDropWrapper}.
      * 
@@ -194,7 +198,8 @@ public class DragAndDropWrapper extends CustomComponent implements DropTarget,
      *            the component to be wrapped
      */
     public DragAndDropWrapper(Component root) {
-        super(root);
+        this();
+        setCompositionRoot(root);
     }
 
     /**
diff --git a/server/src/com/vaadin/ui/Form.java b/server/src/com/vaadin/ui/Form.java
index 039108a578..391ee45536 100644
--- a/server/src/com/vaadin/ui/Form.java
+++ b/server/src/com/vaadin/ui/Form.java
@@ -1059,8 +1059,9 @@ public class Form extends AbstractField<Object> implements Item.Editor,
      * @return the Field.
      */
     private Field<?> getFirstFocusableField() {
-        if (getItemPropertyIds() != null) {
-            for (Object id : getItemPropertyIds()) {
+        Collection<?> itemPropertyIds = getItemPropertyIds();
+        if (itemPropertyIds != null && itemPropertyIds.size() > 0) {
+            for (Object id : itemPropertyIds) {
                 if (id != null) {
                     Field<?> field = getField(id);
                     if (field.isEnabled() && !field.isReadOnly()) {
@@ -1070,7 +1071,7 @@ public class Form extends AbstractField<Object> implements Item.Editor,
             }
             // fallback: first field if none of the fields is enabled and
             // writable
-            Object id = getItemPropertyIds().iterator().next();
+            Object id = itemPropertyIds.iterator().next();
             if (id != null) {
                 return getField(id);
             }
diff --git a/server/src/com/vaadin/ui/PopupView.java b/server/src/com/vaadin/ui/PopupView.java
index fc927cf90d..90c60edc6e 100644
--- a/server/src/com/vaadin/ui/PopupView.java
+++ b/server/src/com/vaadin/ui/PopupView.java
@@ -61,6 +61,11 @@ public class PopupView extends AbstractComponent implements HasComponents {
 
     /* Constructors */
 
+    private PopupView() {
+        registerRpc(rpc);
+        setHideOnMouseOut(true);
+    }
+
     /**
      * A simple way to create a PopupPanel. Note that the minimal representation
      * may not be dynamically updated, in order to achieve this create your own
@@ -94,9 +99,7 @@ public class PopupView extends AbstractComponent implements HasComponents {
      *            the PopupView.Content that contains the information for this
      */
     public PopupView(PopupView.Content content) {
-        super();
-        registerRpc(rpc);
-        setHideOnMouseOut(true);
+        this();
         setContent(content);
     }
 
diff --git a/server/src/com/vaadin/ui/components/colorpicker/ColorPickerGradient.java b/server/src/com/vaadin/ui/components/colorpicker/ColorPickerGradient.java
index 6147fcdd96..81b178e4f0 100644
--- a/server/src/com/vaadin/ui/components/colorpicker/ColorPickerGradient.java
+++ b/server/src/com/vaadin/ui/components/colorpicker/ColorPickerGradient.java
@@ -56,7 +56,7 @@ public class ColorPickerGradient extends AbstractComponent implements
     };
 
     /** The converter. */
-    private final Coordinates2Color converter;
+    private Coordinates2Color converter;
 
     /** The foreground color. */
     private Color color;
@@ -67,6 +67,14 @@ public class ColorPickerGradient extends AbstractComponent implements
     /** The y-coordinate. */
     private int y = 0;
 
+    private ColorPickerGradient() {
+        registerRpc(rpc);
+        // width and height must be set here instead of in theme, otherwise
+        // coordinate calculations fail
+        getState().width = "220px";
+        getState().height = "220px";
+    }
+
     /**
      * Instantiates a new color picker gradient.
      * 
@@ -76,12 +84,8 @@ public class ColorPickerGradient extends AbstractComponent implements
      *            the converter
      */
     public ColorPickerGradient(String id, Coordinates2Color converter) {
-        registerRpc(rpc);
+        this();
         addStyleName(id);
-        // width and height must be set here instead of in theme, otherwise
-        // coordinate calculations fail
-        getState().width = "220px";
-        getState().height = "220px";
         this.converter = converter;
     }
 
diff --git a/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java b/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java
index e7b412f7eb..b9a8c001ce 100644
--- a/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java
+++ b/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java
@@ -143,14 +143,7 @@ public class ColorPickerPopup extends Window implements ClickListener,
      */
     private boolean updatingColors = false;
 
-    /**
-     * Instantiates a new color picker popup.
-     */
-    public ColorPickerPopup(Color initialColor) {
-        super();
-
-        selectedColor = initialColor;
-
+    private ColorPickerPopup() {
         // Set the layout
         layout = new VerticalLayout();
         layout.setSpacing(false);
@@ -162,15 +155,21 @@ public class ColorPickerPopup extends Window implements ClickListener,
         setStyleName(STYLENAME);
         setResizable(false);
         setImmediate(true);
+        // Create the history
+        history = new ColorPickerHistory();
+        history.addColorChangeListener(this);
+    }
 
+    /**
+     * Instantiates a new color picker popup.
+     */
+    public ColorPickerPopup(Color initialColor) {
+        this();
+        selectedColor = initialColor;
         initContents();
     }
 
     private void initContents() {
-        // Create the history
-        history = new ColorPickerHistory();
-        history.addColorChangeListener(this);
-
         // Create the preview on the rgb tab
         rgbPreview = new ColorPickerPreview(selectedColor);
         rgbPreview.setWidth("240px");
diff --git a/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPreview.java b/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPreview.java
index ae00b267ce..21a3630de2 100644
--- a/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPreview.java
+++ b/server/src/com/vaadin/ui/components/colorpicker/ColorPickerPreview.java
@@ -56,24 +56,23 @@ public class ColorPickerPreview extends CssLayout implements ColorSelector,
     /** The old value. */
     private String oldValue;
 
-    /**
-     * Instantiates a new color picker preview.
-     */
-    public ColorPickerPreview(Color color) {
+    private ColorPickerPreview() {
         setStyleName("v-colorpicker-preview");
         setImmediate(true);
-
-        this.color = color;
-
         field = new TextField();
         field.setImmediate(true);
         field.setSizeFull();
         field.setStyleName("v-colorpicker-preview-textfield");
         field.setData(this);
         field.addValueChangeListener(this);
-
         addComponent(field);
+    }
 
+    /**
+     * Instantiates a new color picker preview.
+     */
+    public ColorPickerPreview(Color color) {
+        this();
         setColor(color);
     }
 
diff --git a/server/tests/src/com/vaadin/tests/server/component/StateGetDoesNotMarkDirty.java b/server/tests/src/com/vaadin/tests/server/component/StateGetDoesNotMarkDirty.java
new file mode 100644
index 0000000000..280d638707
--- /dev/null
+++ b/server/tests/src/com/vaadin/tests/server/component/StateGetDoesNotMarkDirty.java
@@ -0,0 +1,100 @@
+package com.vaadin.tests.server.component;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+import junit.framework.TestCase;
+
+import org.mockito.Mockito;
+
+import com.vaadin.tests.VaadinClasses;
+import com.vaadin.ui.Component;
+import com.vaadin.ui.ConnectorTracker;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.UI;
+
+public class StateGetDoesNotMarkDirty extends TestCase {
+
+    private Set<String> excludedMethods = new HashSet<String>();
+
+    @Override
+    protected void setUp() throws Exception {
+        excludedMethods.add(Label.class.getName() + "getDataSourceValue");
+        excludedMethods.add("getConnectorId");
+    }
+
+    public void testGetDoesntMarkStateDirty() throws Exception {
+        for (Class<? extends Component> c : VaadinClasses.getComponents()) {
+            Component newInstance = construct(c);
+            prepareMockUI(newInstance);
+
+            Set<Method> methods = new HashSet<Method>();
+            methods.addAll(Arrays.asList(c.getMethods()));
+            methods.addAll(Arrays.asList(c.getDeclaredMethods()));
+            for (Method method : methods) {
+                try {
+                    if (method.getName().startsWith("is")
+                            || method.getName().startsWith("get")) {
+                        if (method.getName().startsWith("getState")) {
+                            continue;
+                        }
+                        if (method.getParameterTypes().length > 0) {
+                            // usually getters do not have params, if they have
+                            // we still wouldnt know what to put into
+                            continue;
+                        }
+                        if (excludedMethods.contains(c.getName()
+                                + method.getName())) {
+                            // blacklisted method for specific classes
+                            continue;
+                        }
+                        if (excludedMethods.contains(method.getName())) {
+                            // blacklisted method for all classes
+                            continue;
+                        }
+                        // just to make sure we can invoke it
+                        method.setAccessible(true);
+                        method.invoke(newInstance);
+                    }
+                } catch (Exception e) {
+                    System.err.println("problem with method " + c.getName()
+                            + "# " + method.getName());
+                    e.printStackTrace();
+                    throw e;
+                }
+            }
+        }
+
+    }
+
+    private void prepareMockUI(Component newInstance) {
+        UI ui = Mockito.mock(UI.class);
+        Mockito.when(ui.getLocale()).thenReturn(Locale.ENGLISH);
+        ConnectorTracker connectorTracker = Mockito
+                .mock(ConnectorTracker.class);
+        Mockito.when(ui.getConnectorTracker()).thenReturn(connectorTracker);
+        Mockito.doThrow(new RuntimeException("getState(true) called in getter"))
+                .when(connectorTracker).markDirty(newInstance);
+
+        newInstance.setParent(ui);
+    }
+
+    private Component construct(Class<? extends Component> c) {
+        try {
+            try {
+                Constructor<? extends Component> declaredConstructor = c
+                        .getDeclaredConstructor();
+                declaredConstructor.setAccessible(true);
+                return declaredConstructor.newInstance();
+            } catch (NoSuchMethodException e) {
+                return c.newInstance();
+            }
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+    }
+}