Partly eliminate the use of Property.toString().

The toString() method should not be used on a property to get its value.
Added getStringValue() to AbstractField, Label etc. Using them
where applicable. Added comments to some problematic locations where
Property.toString() is known to be used.

AbstractField.toString() and Label.toString() still return the same
values as before, but it will throw an exception in future revisions.

Migration needed: Replace explicit and implicit uses of
Property.toString() with use property.getValue() and its
string representation.
Alternatively, use AbstractProperty.getStringValue() and
AbstractField.getStringValue() instead of Property.toString() during
migration.

src/com/vaadin/data/Property.java

@@ -60,6 +60,8 @@ public interface Property extends Serializable {
      * should at least understand the format returned by the
      * <code>toString</code> method of the Property.
      * 
+     * TODO correct this comment as eliminating Property.toString()
+     * 
      * @param newValue
      *            New value of the Property. This should be assignable to the
      *            type returned by getType, but also String type should be
@@ -74,16 +76,6 @@ public interface Property extends Serializable {
     public void setValue(Object newValue) throws Property.ReadOnlyException,
             Property.ConversionException;
 
-    /**
-     * Returns the value of the Property in human readable textual format. The
-     * return value should be assignable to the <code>setValue</code> method if
-     * the Property is not in read-only mode.
-     * 
-     * @return <code>String</code> representation of the value stored in the
-     *         Property
-     */
-    public String toString();
-
     /**
      * Returns the type of the Property. The methods <code>getValue</code> and
      * <code>setValue</code> must be compatible with this type: one must be able

src/com/vaadin/data/util/AbstractProperty.java

@@ -60,9 +60,23 @@ public abstract class AbstractProperty implements Property,
      * <code>setValue</code> method if the Property is not in read-only mode.
      * 
      * @return String representation of the value stored in the Property
+     * @deprecated use the property value directly, or {@link #getStringValue()}
+     *             during migration period
      */
+    @Deprecated
     @Override
     public String toString() {
+        return getStringValue();
+    }
+
+    /**
+     * Returns the value of the <code>Property</code> in human readable textual
+     * format.
+     * 
+     * @return String representation of the value stored in the Property
+     * @since 7.0
+     */
+    public String getStringValue() {
         final Object value = getValue();
         if (value == null) {
             return null;

src/com/vaadin/data/util/IndexedContainer.java

@@ -702,6 +702,7 @@ public class IndexedContainer extends
 
             for (final Iterator<?> i = propertyIds.iterator(); i.hasNext();) {
                 final Object propertyId = i.next();
+                // TODO do not use Property.toString()
                 retValue += getItemProperty(propertyId).toString();
                 if (i.hasNext()) {
                     retValue += " ";
@@ -910,9 +911,23 @@ public class IndexedContainer extends
          * 
          * @return <code>String</code> representation of the value stored in the
          *         Property
+         * @deprecated use the property value directly, or
+         *             {@link #getStringValue()} during migration period
          */
+        @Deprecated
         @Override
         public String toString() {
+            return getStringValue();
+        }
+
+        /**
+         * Returns the value of the <code>Property</code> in human readable
+         * textual format.
+         * 
+         * @return String representation of the value stored in the Property
+         * @since 7.0
+         */
+        public String getStringValue() {
             final Object value = getValue();
             if (value == null) {
                 return null;

src/com/vaadin/data/util/PropertyFormatter.java

@@ -98,7 +98,7 @@ public abstract class PropertyFormatter extends AbstractProperty implements
                         .removeListener(this);
             }
             readOnly = isReadOnly();
-            prevValue = toString();
+            prevValue = getStringValue();
         }
 
         dataSource = newDataSource;
@@ -116,7 +116,7 @@ public abstract class PropertyFormatter extends AbstractProperty implements
         if (isReadOnly() != readOnly) {
             fireReadOnlyStatusChange();
         }
-        String newVal = toString();
+        String newVal = getStringValue();
         if ((prevValue == null && newVal != null)
                 || (prevValue != null && !prevValue.equals(newVal))) {
             fireValueChange();
@@ -135,17 +135,18 @@ public abstract class PropertyFormatter extends AbstractProperty implements
      *         String given by format().
      */
     public Object getValue() {
-        return toString();
+        return getStringValue();
     }
 
     /**
-     * Get the formatted value.
+     * Get the formatted value. For PropertyFormatter, this is identical with
+     * {@link #getValue()}.
      * 
      * @return If the datasource returns null, this is null. Otherwise this is
      *         String given by format().
      */
     @Override
-    public String toString() {
+    public String getStringValue() {
         Object value = dataSource == null ? false : dataSource.getValue();
         if (value == null) {
             return null;
@@ -154,6 +155,7 @@ public abstract class PropertyFormatter extends AbstractProperty implements
     }
 
     /** Reflects the read-only status of the datasource. */
+    @Override
     public boolean isReadOnly() {
         return dataSource == null ? false : dataSource.isReadOnly();
     }
@@ -190,6 +192,7 @@ public abstract class PropertyFormatter extends AbstractProperty implements
      * @param newStatus
      *            the new read-only status of the Property.
      */
+    @Override
     public void setReadOnly(boolean newStatus) {
         if (dataSource != null) {
             dataSource.setReadOnly(newStatus);
@@ -209,7 +212,7 @@ public abstract class PropertyFormatter extends AbstractProperty implements
         } else {
             try {
                 dataSource.setValue(parse((String) newValue));
-                if (!newValue.equals(toString())) {
+                if (!newValue.equals(getStringValue())) {
                     fireValueChange();
                 }
             } catch (Exception e) {

src/com/vaadin/data/util/PropertysetItem.java

@@ -143,7 +143,7 @@ public class PropertysetItem implements Item, Item.PropertySetChangeNotifier,
 
         for (final Iterator<?> i = getItemPropertyIds().iterator(); i.hasNext();) {
             final Object propertyId = i.next();
-            retValue += getItemProperty(propertyId).toString();
+            retValue += getItemProperty(propertyId).getValue();
             if (i.hasNext()) {
                 retValue += " ";
             }

src/com/vaadin/data/util/filter/SimpleStringFilter.java

@@ -41,11 +41,15 @@ public final class SimpleStringFilter implements Filter {
 
     public boolean passesFilter(Object itemId, Item item) {
         final Property p = item.getItemProperty(propertyId);
-        if (p == null || p.toString() == null) {
+        if (p == null) {
             return false;
         }
-        final String value = ignoreCase ? p.toString().toLowerCase() : p
-                .toString();
+        Object propertyValue = p.getValue();
+        if (propertyValue == null) {
+            return false;
+        }
+        final String value = ignoreCase ? propertyValue.toString()
+                .toLowerCase() : propertyValue.toString();
         if (onlyMatchPrefix) {
             if (!value.startsWith(filterString)) {
                 return false;

src/com/vaadin/data/util/sqlcontainer/ColumnProperty.java

@@ -168,13 +168,37 @@ final public class ColumnProperty implements Property {
         return propertyId;
     }
 
+    /**
+     * Returns the value of the Property in human readable textual format.
+     * 
+     * @see java.lang.Object#toString()
+     * @deprecated get the string representation from the value, or use
+     *             getStringValue() during migration
+     */
+    @Deprecated
     @Override
     public String toString() {
-        Object val = getValue();
-        if (val == null) {
+        return getStringValue();
+    }
+
+    /**
+     * Returns the (UI type) value of the field converted to a String using
+     * toString().
+     * 
+     * This method exists to help migration from the use of Property.toString()
+     * to get the field value - for new applications, access getValue()
+     * directly. This method may disappear in future Vaadin versions.
+     * 
+     * @return string representation of the field value or null if the value is
+     *         null
+     * @since 7.0
+     */
+    public String getStringValue() {
+        final Object value = getValue();
+        if (value == null) {
             return null;
         }
-        return val.toString();
+        return value.toString();
     }
 
     public void setOwner(RowItem owner) {

src/com/vaadin/data/util/sqlcontainer/RowItem.java

@@ -113,7 +113,8 @@ public final class RowItem implements Item {
             s.append("|");
             s.append(propId.toString());
             s.append(":");
-            s.append(getItemProperty(propId).toString());
+            Object value = getItemProperty(propId).getValue();
+            s.append((null != value) ? value.toString() : null);
         }
         return s.toString();
     }

src/com/vaadin/ui/AbstractField.java

@@ -298,8 +298,10 @@ public abstract class AbstractField extends AbstractComponent implements Field,
             try {
 
                 // Discards buffer by overwriting from datasource
-                newValue = String.class == getType() ? dataSource.toString()
-                        : dataSource.getValue();
+                newValue = dataSource.getValue();
+                if (String.class == getType()) {
+                    newValue = newValue.toString();
+                }
 
                 // If successful, remove set the buffering state to be ok
                 if (currentBufferedSourceException != null) {
@@ -387,8 +389,11 @@ public abstract class AbstractField extends AbstractComponent implements Field,
         }
         readThroughMode = readThrough;
         if (!isModified() && readThroughMode && dataSource != null) {
-            setInternalValue(String.class == getType() ? dataSource.toString()
-                    : dataSource.getValue());
+            Object newValue = dataSource.getValue();
+            if (String.class == getType()) {
+                newValue = newValue.toString();
+            }
+            setInternalValue(newValue);
             fireValueChange(false);
         }
     }
@@ -399,14 +404,33 @@ public abstract class AbstractField extends AbstractComponent implements Field,
      * Returns the value of the Property in human readable textual format.
      * 
      * @see java.lang.Object#toString()
+     * @deprecated get the string representation from the data source, or use
+     *             getStringValue() during migration
      */
+    @Deprecated
     @Override
     public String toString() {
+        return getStringValue();
+    }
+
+    /**
+     * Returns the (UI type) value of the field converted to a String using
+     * toString().
+     * 
+     * This method exists to help migration from the use of Property.toString()
+     * to get the field value - for new applications, access getValue()
+     * directly. This method may disappear in future Vaadin versions.
+     * 
+     * @return string representation of the field value or null if the value is
+     *         null
+     * @since 7.0
+     */
+    public String getStringValue() {
         final Object value = getValue();
         if (value == null) {
             return null;
         }
-        return getValue().toString();
+        return value.toString();
     }
 
     /**
@@ -441,10 +465,11 @@ public abstract class AbstractField extends AbstractComponent implements Field,
             return value;
         }
 
-        Object newValue = String.class == getType() ? dataSource.toString()
-                : dataSource.getValue();
-
-        return newValue;
+        Object result = dataSource.getValue();
+        if (String.class == getType()) {
+            result = result.toString();
+        }
+        return result;
     }
 
     /**
@@ -612,8 +637,11 @@ public abstract class AbstractField extends AbstractComponent implements Field,
         // Gets the value from source
         try {
             if (dataSource != null) {
-                setInternalValue(String.class == getType() ? dataSource
-                        .toString() : dataSource.getValue());
+                Object newValue = dataSource.getValue();
+                if (String.class == getType()) {
+                    newValue = newValue.toString();
+                }
+                setInternalValue(newValue);
             }
             modified = false;
         } catch (final Throwable e) {
@@ -1329,4 +1357,4 @@ public abstract class AbstractField extends AbstractComponent implements Field,
             focusable.focus();
         }
     }
-}
+}

src/com/vaadin/ui/AbstractTextField.java

@@ -373,7 +373,7 @@ public abstract class AbstractTextField extends AbstractField implements
 
     @Override
     protected boolean isEmpty() {
-        return super.isEmpty() || toString().length() == 0;
+        return super.isEmpty() || getStringValue().length() == 0;
     }
 
     /**
@@ -751,4 +751,4 @@ public abstract class AbstractTextField extends AbstractField implements
         removeListener(BlurEvent.EVENT_ID, BlurEvent.class, listener);
     }
 
-}
+}

src/com/vaadin/ui/Label.java

@@ -194,24 +194,24 @@ public class Label extends AbstractComponent implements Property,
             target.addAttribute("mode", CONTENT_MODE_NAME[contentMode]);
         }
         if (contentMode == CONTENT_TEXT) {
-            target.addText(toString());
+            target.addText(getStringValue());
         } else if (contentMode == CONTENT_UIDL) {
-            target.addUIDL(toString());
+            target.addUIDL(getStringValue());
         } else if (contentMode == CONTENT_XHTML) {
             target.startTag("data");
-            target.addXMLSection("div", toString(),
+            target.addXMLSection("div", getStringValue(),
                     "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd");
             target.endTag("data");
         } else if (contentMode == CONTENT_PREFORMATTED) {
             target.startTag("pre");
-            target.addText(toString());
+            target.addText(getStringValue());
             target.endTag("pre");
         } else if (contentMode == CONTENT_XML) {
-            target.addXMLSection("data", toString(), null);
+            target.addXMLSection("data", getStringValue(), null);
         } else if (contentMode == CONTENT_RAW) {
             target.startTag("data");
             target.addAttribute("escape", false);
-            target.addText(toString());
+            target.addText(getStringValue());
             target.endTag("data");
         }
 
@@ -246,12 +246,25 @@ public class Label extends AbstractComponent implements Property,
 
     /**
      * @see java.lang.Object#toString()
+     * @deprecated use the data source value or {@link #getStringValue()}
+     *             instead
      */
+    @Deprecated
     @Override
     public String toString() {
+        return getStringValue();
+    }
+
+    /**
+     * TODO temporary method to help eliminate the use of toString()
+     * 
+     * @return
+     */
+    public String getStringValue() {
         if (dataSource == null) {
             throw new IllegalStateException(DATASOURCE_MUST_BE_SET);
         }
+        // TODO do not use Property.toString()
         return dataSource.toString();
     }
 
@@ -489,17 +502,19 @@ public class Label extends AbstractComponent implements Property,
 
         if (contentMode == CONTENT_XML || contentMode == CONTENT_UIDL
                 || contentMode == CONTENT_XHTML) {
-            thisValue = stripTags(toString());
+            thisValue = stripTags(getStringValue());
         } else {
-            thisValue = toString();
+            thisValue = getStringValue();
         }
 
         if (other instanceof Label
                 && (((Label) other).getContentMode() == CONTENT_XML
                         || ((Label) other).getContentMode() == CONTENT_UIDL || ((Label) other)
                         .getContentMode() == CONTENT_XHTML)) {
-            otherValue = stripTags(other.toString());
+            otherValue = stripTags(((Label) other).getStringValue());
         } else {
+            // TODO not a good idea - and might assume that Field.toString()
+            // returns a string representation of the value
             otherValue = other.toString();
         }
 

src/com/vaadin/ui/ProgressIndicator.java

@@ -150,13 +150,16 @@ public class ProgressIndicator extends AbstractField implements Property,
 
     /**
      * @see com.vaadin.ui.AbstractField#toString()
+     * @deprecated use the data source value instead of toString()
      */
+    @Deprecated
     @Override
     public String toString() {
         if (dataSource == null) {
             throw new IllegalStateException("Datasource must be set");
         }
-        return dataSource.toString();
+        Object value = dataSource.getValue();
+        return (null != value) ? value.toString() : null;
     }
 
     /**

src/com/vaadin/ui/RichTextArea.java

@@ -342,7 +342,7 @@ public class RichTextArea extends AbstractField {
 
     @Override
     protected boolean isEmpty() {
-        return super.isEmpty() || toString().length() == 0;
+        return super.isEmpty() || getStringValue().length() == 0;
     }
 
 }

src/com/vaadin/ui/Table.java

@@ -3230,7 +3230,8 @@ public class Table extends AbstractSelect implements Action.Container,
         if (property == null) {
             return "";
         }
-        return property.toString();
+        Object value = property.getValue();
+        return (null != value) ? value.toString() : "";
     }
 
     /* Action container */

tests/server-side/com/vaadin/data/util/PropertySetItemTest.java

@@ -9,8 +9,6 @@ import org.easymock.EasyMock;
 
 import com.vaadin.data.Item.PropertySetChangeEvent;
 import com.vaadin.data.Item.PropertySetChangeListener;
-import com.vaadin.data.util.ObjectProperty;
-import com.vaadin.data.util.PropertysetItem;
 
 public class PropertySetItemTest extends TestCase {
 
@@ -395,13 +393,13 @@ public class PropertySetItemTest extends TestCase {
 
         item.addItemProperty(ID1, prop1);
 
-        Assert.assertEquals(String.valueOf(prop1), item.toString());
+        Assert.assertEquals(String.valueOf(prop1.getValue()), item.toString());
 
         item.addItemProperty(ID2, prop2);
 
         Assert.assertEquals(
-                String.valueOf(prop1) + " " + String.valueOf(prop2),
-                item.toString());
+                String.valueOf(prop1.getValue()) + " "
+                        + String.valueOf(prop2.getValue()), item.toString());
     }
 
 }

tests/server-side/com/vaadin/data/util/sqlcontainer/SQLContainerTest.java

@@ -1344,7 +1344,7 @@ public class SQLContainerTest {
                         Statement statement = conn.createStatement();
                         statement
                                 .executeUpdate("DELETE FROM people WHERE \"ID\"="
-                                        + item.getItemProperty("ID"));
+                                        + item.getItemProperty("ID").getValue());
                         statement.close();
                         return true;
                     }

tests/server-side/com/vaadin/tests/server/TestSerialization.java

@@ -10,6 +10,7 @@ import java.io.Serializable;
 import junit.framework.TestCase;
 
 import com.vaadin.data.Item;
+import com.vaadin.data.Property;
 import com.vaadin.data.util.IndexedContainer;
 import com.vaadin.data.util.MethodProperty;
 import com.vaadin.data.validator.RegexpValidator;
@@ -78,15 +79,25 @@ public class TestSerialization extends TestCase {
                 data));
         Serializable s2 = (Serializable) in.readObject();
 
+        // using special toString(Object) method to avoid calling
+        // Property.toString(), which will be temporarily disabled
         if (s.equals(s2)) {
-            System.out.println(s + " equals " + s2);
+            System.out.println(toString(s) + " equals " + toString(s2));
         } else {
-            System.out.println(s + " does NOT equal " + s2);
+            System.out.println(toString(s) + " does NOT equal " + toString(s2));
         }
 
         return s2;
     }
 
+    private static String toString(Object o) {
+        if (o instanceof Property) {
+            return String.valueOf(((Property) o).getValue());
+        } else {
+            return String.valueOf(o);
+        }
+    }
+
     public static class Data implements Serializable {
         private String dummyGetter;
         private String dummyGetterAndSetter;

tests/testbench/com/vaadin/tests/TestForContainerFilterable.java

@@ -62,12 +62,12 @@ public class TestForContainerFilterable extends CustomComponent {
         filterButton.addListener(new Button.ClickListener() {
             public void buttonClick(ClickEvent event) {
                 ic.removeAllContainerFilters();
-                if (fooFilter.toString().length() > 0) {
-                    ic.addContainerFilter("foo", fooFilter.toString(), false,
+                if (fooFilter.getStringValue().length() > 0) {
+                    ic.addContainerFilter("foo", fooFilter.getStringValue(), false,
                             false);
                 }
-                if (barFilter.toString().length() > 0) {
-                    ic.addContainerFilter("bar", barFilter.toString(), true,
+                if (barFilter.getStringValue().length() > 0) {
+                    ic.addContainerFilter("bar", barFilter.getStringValue(), true,
                             true);
                 }
                 count.setValue("Rows in table: " + ic.size());

tests/testbench/com/vaadin/tests/TestForPreconfiguredComponents.java

@@ -160,6 +160,7 @@ public class TestForPreconfiguredComponents extends CustomComponent implements
         t.addListener(new Listener() {
             public void componentEvent(Event event) {
                 status.addComponent(new Label(event.getClass().getName()));
+                // TODO should not use Property.toString()
                 status.addComponent(new Label("selected: "
                         + event.getSource().toString()));
             }

tests/testbench/com/vaadin/tests/TestForTrees.java

@@ -142,6 +142,7 @@ public class TestForTrees extends CustomComponent implements Handler {
         t.addListener(new Listener() {
             public void componentEvent(Event event) {
                 status.addComponent(new Label(event.getClass().getName()));
+                // TODO should not use Property.toString()
                 status.addComponent(new Label("selected: "
                         + event.getSource().toString()));
             }

tests/testbench/com/vaadin/tests/components/datefield/DefaultHandleUnparsableDateField.java

@@ -17,7 +17,7 @@ public class DefaultHandleUnparsableDateField extends TestBase {
         date.addListener(new Property.ValueChangeListener() {
             public void valueChange(ValueChangeEvent event) {
                 if (date.isValid()) {
-                    getMainWindow().showNotification(date.toString());
+                    getMainWindow().showNotification(date.getStringValue());
                 }
 
             }

tests/testbench/com/vaadin/tests/components/textfield/TextChangeEventsWithNonImmediateValueChange.java

@@ -33,6 +33,7 @@ public class TextChangeEventsWithNonImmediateValueChange extends TestBase {
         tf.addListener(new ValueChangeListener() {
 
             public void valueChange(ValueChangeEvent event) {
+                // TODO should not use Property.toString()
                 l.log("Value change:" + event.getProperty().toString());
             }
         });

tests/testbench/com/vaadin/tests/dd/DDTest2.java

@@ -94,6 +94,7 @@ public class DDTest2 extends TestBase {
                 if (transferable instanceof TableTransferable) {
                     TableTransferable tr = (TableTransferable) transferable;
                     System.out.println("From table row" + tr.getPropertyId());
+                    // TODO should not use Property.toString()
                     data = tr.getSourceContainer().getItem(tr.getItemId())
                             .getItemProperty(tr.getPropertyId()).toString();
 
@@ -147,6 +148,7 @@ public class DDTest2 extends TestBase {
                     // if the source is from table (not from tree1 itself),
                     // transfer Name property and use it as an identifier in
                     // tree1
+                    // TODO should not use Property.toString()
                     String name = sourceContainer.getItem(itemId)
                             .getItemProperty("Name").toString();
 

tests/testbench/com/vaadin/tests/tickets/Ticket1397.java

@@ -31,7 +31,7 @@ public class Ticket1397 extends Application {
 
         PopupView.Content content = new PopupView.Content() {
             public String getMinimizedValueAsHTML() {
-                return prop.toString();
+                return String.valueOf(prop.getValue());
             }
 
             public Component getPopupComponent() {
@@ -69,7 +69,7 @@ public class Ticket1397 extends Application {
         panel2.addComponent(new myButton());
         PopupView.Content content2 = new PopupView.Content() {
             public String getMinimizedValueAsHTML() {
-                return prop2.toString();
+                return String.valueOf(prop2.getValue());
             }
 
             public Component getPopupComponent() {
@@ -90,7 +90,7 @@ public class Ticket1397 extends Application {
         PopupView.Content content3 = new PopupView.Content() {
 
             public String getMinimizedValueAsHTML() {
-                return op.toString();
+                return String.valueOf(op.getValue());
             }
 
             public Component getPopupComponent() {
@@ -114,7 +114,7 @@ public class Ticket1397 extends Application {
             final InlineDateField df = new InlineDateField("", new Date());
             PopupView pp = new PopupView(new PopupView.Content() {
                 public String getMinimizedValueAsHTML() {
-                    return df.toString();
+                    return String.valueOf(df.getValue());
                 }
 
                 public Component getPopupComponent() {
@@ -131,7 +131,8 @@ public class Ticket1397 extends Application {
                         + " and see how the overview-version changes.");
 
                 public String getMinimizedValueAsHTML() {
-                    return "" + tf.toString().length() + " characters of info";
+                    return "" + String.valueOf(tf.getValue()).length()
+                            + " characters of info";
                 }
 
                 public Component getPopupComponent() {

tests/testbench/com/vaadin/tests/tickets/Ticket2053.java

@@ -41,8 +41,9 @@ public class Ticket2053 extends Application {
                 c.addComponent(tf);
                 tf.addListener(new Property.ValueChangeListener() {
                     public void valueChange(ValueChangeEvent event) {
+                        // TODO should not use Property.toString()
                         main.addComponent(new Label(name + " send text:"
-                                + tf.toString()));
+                                + tf.getStringValue()));
                     }
                 });
                 for (int i = 0; i < 3; i++) {

tests/testbench/com/vaadin/tests/tickets/Ticket2090.java

@@ -30,7 +30,7 @@ public class Ticket2090 extends Application {
         height.addListener(new Property.ValueChangeListener() {
             public void valueChange(ValueChangeEvent event) {
                 try {
-                    target.setHeight(height.toString());
+                    target.setHeight(height.getStringValue());
                     height.setComponentError(null);
                     updateLabel();
                 } catch (Exception e) {
@@ -41,7 +41,7 @@ public class Ticket2090 extends Application {
         width.addListener(new Property.ValueChangeListener() {
             public void valueChange(ValueChangeEvent event) {
                 try {
-                    target.setWidth(width.toString());
+                    target.setWidth(width.getStringValue());
                     width.setComponentError(null);
                     updateLabel();
                 } catch (Exception e) {