Make ColorPicker an AbstractField<Color>

Remove old ColorSelector API from ColorPicker.
The various subcomponents still implement ColorSelector for now.

Change-Id: If0e30eda71c5fe30f7128056a65fb127a4953089

3 files changed, 99 insertions, 152 deletions

diff --git a/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java b/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java
index 695187a6eb..d9a668bb9a 100644
--- a/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java
+++ b/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java
@@ -16,23 +16,16 @@
 package com.vaadin.ui;
 
 import java.io.Serializable;
-import java.lang.reflect.Method;
 import java.util.Collection;
 import java.util.Objects;
 
 import org.jsoup.nodes.Attributes;
 import org.jsoup.nodes.Element;
 
-import com.vaadin.shared.Registration;
 import com.vaadin.shared.ui.colorpicker.Color;
 import com.vaadin.shared.ui.colorpicker.ColorPickerServerRpc;
 import com.vaadin.shared.ui.colorpicker.ColorPickerState;
-import com.vaadin.ui.Window.CloseEvent;
-import com.vaadin.ui.Window.CloseListener;
-import com.vaadin.ui.components.colorpicker.ColorChangeEvent;
-import com.vaadin.ui.components.colorpicker.ColorChangeListener;
 import com.vaadin.ui.components.colorpicker.ColorPickerPopup;
-import com.vaadin.ui.components.colorpicker.ColorSelector;
 import com.vaadin.ui.declarative.DesignAttributeHandler;
 import com.vaadin.ui.declarative.DesignContext;
 
@@ -42,27 +35,15 @@ import com.vaadin.ui.declarative.DesignContext;
  *
  * @since 7.0.0
  */
-public abstract class AbstractColorPicker extends AbstractComponent
-        implements CloseListener, ColorSelector {
-    private static final Method COLOR_CHANGE_METHOD;
-    static {
-        try {
-            COLOR_CHANGE_METHOD = ColorChangeListener.class.getDeclaredMethod(
-                    "colorChanged", new Class[] { ColorChangeEvent.class });
-        } catch (final java.lang.NoSuchMethodException e) {
-            // This should never happen
-            throw new java.lang.RuntimeException(
-                    "Internal error finding methods in ColorPicker");
-        }
-    }
+public abstract class AbstractColorPicker extends AbstractField<Color> {
 
     /**
-     * Interface for converting 2d-coordinates to a Color
+     * Interface for converting 2d-coordinates to a Color.
      */
     public interface Coordinates2Color extends Serializable {
 
         /**
-         * Calculate color from coordinates
+         * Calculates a color from coordinates.
          *
          * @param x
          *            the x-coordinate
@@ -74,7 +55,7 @@ public abstract class AbstractColorPicker extends AbstractComponent
         public Color calculate(int x, int y);
 
         /**
-         * Calculate coordinates from color
+         * Calculates coordinates from a color.
          *
          * @param c
          *            the c
@@ -84,8 +65,15 @@ public abstract class AbstractColorPicker extends AbstractComponent
         public int[] calculate(Color c);
     }
 
+    /**
+     * The style of the color picker popup.
+     */
     public enum PopupStyle {
-        POPUP_NORMAL("normal"), POPUP_SIMPLE("simple");
+        /** A full popup with all tabs visible. */
+        POPUP_NORMAL("normal"),
+
+        /** A simple popup with only the swatches (palette) tab. */
+        POPUP_SIMPLE("simple");
 
         private String style;
 
@@ -116,13 +104,13 @@ public abstract class AbstractColorPicker extends AbstractComponent
     /** The popup window. */
     private ColorPickerPopup window;
 
-    /** The color. */
+    /** The currently selected color. */
     protected Color color;
 
     /** The UI. */
     private UI parent;
 
-    protected String popupCaption = null;
+    private String popupCaption = null;
     private int positionX = 0;
     private int positionY = 0;
 
@@ -160,25 +148,33 @@ public abstract class AbstractColorPicker extends AbstractComponent
     public AbstractColorPicker(String popupCaption, Color initialColor) {
         super();
         registerRpc(rpc);
-        setColor(initialColor);
+        setValue(initialColor);
         this.popupCaption = popupCaption;
         setDefaultStyles();
         setCaption("");
     }
 
+    /**
+     * Returns the current selected color of this color picker.
+     * 
+     * @return the selected color, not null
+     */
     @Override
-    public void setColor(Color color) {
-        this.color = color;
-
-        if (window != null) {
-            window.setColor(color);
-        }
-        getState().color = color.getCSS();
+    public Color getValue() {
+        return color;
     }
 
+    /**
+     * Sets the selected color of this color picker. If the new color is not
+     * equal to getValue(), fires a value change event.
+     * 
+     * @param color
+     *            the new selected color, not null
+     */
     @Override
-    public Color getColor() {
-        return color;
+    public void setValue(Color color) {
+        Objects.requireNonNull(color, "color cannot be null");
+        super.setValue(color);
     }
 
     /**
@@ -187,6 +183,8 @@ public abstract class AbstractColorPicker extends AbstractComponent
      * available.
      *
      * @param enabled
+     *            {@code true} to enable the default caption, {@code false} to
+     *            disable
      */
     public void setDefaultCaptionEnabled(boolean enabled) {
         getState().showDefaultCaption = enabled;
@@ -195,13 +193,16 @@ public abstract class AbstractColorPicker extends AbstractComponent
     /**
      * Returns true if the component shows the default caption (css-code for the
      * currently selected color, e.g. #ffffff) if no other caption is available.
+     * 
+     * @return {@code true} if the default caption is enabled, {@code false}
+     *         otherwise
      */
     public boolean isDefaultCaptionEnabled() {
         return getState(false).showDefaultCaption;
     }
 
     /**
-     * Sets the position of the popup window
+     * Sets the position of the popup window.
      *
      * @param x
      *            the x-coordinate
@@ -218,75 +219,39 @@ public abstract class AbstractColorPicker extends AbstractComponent
         }
     }
 
-    @Override
-    public Registration addColorChangeListener(ColorChangeListener listener) {
-        addListener(ColorChangeEvent.class, listener, COLOR_CHANGE_METHOD);
-        return () -> removeListener(ColorChangeEvent.class, listener);
-    }
-
-    @Override
-    @Deprecated
-    public void removeColorChangeListener(ColorChangeListener listener) {
-        removeListener(ColorChangeEvent.class, listener);
-    }
-
-    @Override
-    public void windowClose(CloseEvent e) {
-        if (e.getWindow() == window) {
-            getState().popupVisible = false;
-        }
-    }
-
     /**
-     * Fired when a color change event occurs
-     *
-     * @param event
-     *            The color change event
-     */
-    protected void colorChanged(ColorChangeEvent event) {
-        setColor(event.getColor());
-        fireColorChanged();
-    }
-
-    /**
-     * Notifies the listeners that the selected color has changed
-     */
-    public void fireColorChanged() {
-        fireEvent(new ColorChangeEvent(this, color));
-    }
-
-    /**
-     * The style for the popup window
+     * Sets the style of the popup window.
      *
      * @param style
-     *            The style
+     *            the popup window style
      */
     public void setPopupStyle(PopupStyle style) {
         popupStyle = style;
 
         switch (style) {
-        case POPUP_NORMAL: {
+        case POPUP_NORMAL:
             setRGBVisibility(true);
             setHSVVisibility(true);
             setSwatchesVisibility(true);
             setHistoryVisibility(true);
             setTextfieldVisibility(true);
             break;
-        }
 
-        case POPUP_SIMPLE: {
+        case POPUP_SIMPLE:
             setRGBVisibility(false);
             setHSVVisibility(false);
             setSwatchesVisibility(true);
             setHistoryVisibility(false);
             setTextfieldVisibility(false);
             break;
-        }
+
+        default:
+            assert false : "Unknown popup style " + style;
         }
     }
 
     /**
-     * Gets the style for the popup window
+     * Gets the style for the popup window.
      *
      * @since 7.5.0
      * @return popup window style
@@ -296,10 +261,10 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Set the visibility of the RGB Tab
+     * Sets the visibility of the RGB tab.
      *
      * @param visible
-     *            The visibility
+     *            {@code true} to display the RGB tab, {@code false} to hide it
      */
     public void setRGBVisibility(boolean visible) {
 
@@ -314,7 +279,7 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Gets the visibility of the RGB Tab
+     * Gets the visibility of the RGB Tab.
      *
      * @since 7.5.0
      * @return visibility of the RGB tab
@@ -324,10 +289,10 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Set the visibility of the HSV Tab
+     * Sets the visibility of the HSV Tab.
      *
      * @param visible
-     *            The visibility
+     *            {@code true} to display the HSV tab, {@code false} to hide it
      */
     public void setHSVVisibility(boolean visible) {
         if (!visible && !rgbVisible && !swatchesVisible) {
@@ -341,20 +306,22 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Gets the visibility of the HSV Tab
+     * Gets the visibility of the HSV tab.
      *
      * @since 7.5.0
-     * @return visibility of the HSV tab
+     * @return {@code true} if the HSV tab is currently displayed, {@code false}
+     *         otherwise
      */
     public boolean getHSVVisibility() {
         return hsvVisible;
     }
 
     /**
-     * Set the visibility of the Swatches Tab
+     * Sets the visibility of the Swatches (palette) tab.
      *
      * @param visible
-     *            The visibility
+     *            {@code true} to display the Swatches tab, {@code false} to
+     *            hide it
      */
     public void setSwatchesVisibility(boolean visible) {
         if (!visible && !hsvVisible && !rgbVisible) {
@@ -368,20 +335,22 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Gets the visibility of the Swatches Tab
+     * Gets the visibility of the Swatches (palette) tab.
      *
      * @since 7.5.0
-     * @return visibility of the swatches tab
+     * @return {@code true} if the Swatches tab is currently displayed,
+     *         {@code false} otherwise
      */
     public boolean getSwatchesVisibility() {
         return swatchesVisible;
     }
 
     /**
-     * Sets the visibility of the Color History
+     * Sets the visibility of the color history, displaying recently picked
+     * colors.
      *
      * @param visible
-     *            The visibility
+     *            {@code true} to display the history, {@code false} to hide it
      */
     public void setHistoryVisibility(boolean visible) {
         historyVisible = visible;
@@ -391,20 +360,22 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Gets the visibility of the Color History
+     * Gets the visibility of the Color history.
      *
      * @since 7.5.0
-     * @return visibility of color history
+     * @return {@code true} if the history is currently displayed, {@code false}
+     *         otherwise
      */
     public boolean getHistoryVisibility() {
         return historyVisible;
     }
 
     /**
-     * Sets the visibility of the CSS color code text field
+     * Sets the visibility of the CSS color code text field.
      *
      * @param visible
-     *            The visibility
+     *            {@code true} to display the CSS text field, {@code false} to
+     *            hide it
      */
     public void setTextfieldVisibility(boolean visible) {
         textfieldVisible = visible;
@@ -414,10 +385,11 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Gets the visibility of CSS color code text field
+     * Gets the visibility of CSS color code text field.
      *
      * @since 7.5.0
-     * @return visibility of css color code text field
+     * @return {@code true} if the CSS text field is currently displayed,
+     *         {@code false} otherwise
      */
     public boolean getTextfieldVisibility() {
         return textfieldVisible;
@@ -434,10 +406,9 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Sets the default styles of the component
-     *
+     * Sets the default styles of the component.
      */
-    abstract protected void setDefaultStyles();
+    protected abstract void setDefaultStyles();
 
     /**
      * Shows a popup-window for color selection.
@@ -454,10 +425,11 @@ public abstract class AbstractColorPicker extends AbstractComponent
     }
 
     /**
-     * Shows or hides popup-window depending on the given parameter. If there is
-     * no such window yet, one is created.
+     * Shows or hides the popup window depending on the given parameter. If
+     * there is no such window yet, one is created.
      *
      * @param open
+     *            {@code true} to display the popup, {@code false} to hide it
      */
     protected void showPopup(boolean open) {
         if (open && !isReadOnly()) {
@@ -465,9 +437,9 @@ public abstract class AbstractColorPicker extends AbstractComponent
                 parent = getUI();
             }
 
-            if (window == null) {
+            Color color = getValue();
 
-                // Create the popup
+            if (window == null) {
                 window = new ColorPickerPopup(color);
                 window.setCaption(popupCaption);
 
@@ -478,19 +450,18 @@ public abstract class AbstractColorPicker extends AbstractComponent
                 window.setPreviewVisible(textfieldVisible);
 
                 window.setImmediate(true);
-                window.addCloseListener(this);
-                window.addColorChangeListener(new ColorChangeListener() {
-                    @Override
-                    public void colorChanged(ColorChangeEvent event) {
-                        AbstractColorPicker.this.colorChanged(event);
-                    }
-                });
+                window.addCloseListener(
+                        event -> getState().popupVisible = false);
+                window.addColorChangeListener(
+                        event -> setValue(event.getColor()));
 
                 window.getHistory().setColor(color);
-                parent.addWindow(window);
-                window.setVisible(true);
                 window.setPositionX(positionX);
                 window.setPositionY(positionY);
+                window.setVisible(true);
+
+                parent.addWindow(window);
+                window.focus();
 
             } else if (!parent.equals(window.getParent())) {
 
@@ -503,7 +474,9 @@ public abstract class AbstractColorPicker extends AbstractComponent
                 window.setColor(color);
                 window.getHistory().setColor(color);
                 window.setVisible(true);
+
                 parent.addWindow(window);
+                window.focus();
             }
 
         } else if (window != null) {
@@ -513,37 +486,6 @@ public abstract class AbstractColorPicker extends AbstractComponent
         getState().popupVisible = open;
     }
 
-    /**
-     * Set whether the caption text is rendered as HTML or not. You might need
-     * to re-theme component to allow higher content than the original text
-     * style.
-     *
-     * If set to true, the captions are passed to the browser as html and the
-     * developer is responsible for ensuring no harmful html is used. If set to
-     * false, the content is passed to the browser as plain text.
-     *
-     * @param htmlContentAllowed
-     *            <code>true</code> if caption is rendered as HTML,
-     *            <code>false</code> otherwise
-     * @deprecated as of , use {@link #setCaptionAsHtml(boolean)} instead
-     */
-    @Deprecated
-    public void setHtmlContentAllowed(boolean htmlContentAllowed) {
-        setCaptionAsHtml(htmlContentAllowed);
-    }
-
-    /**
-     * Return HTML rendering setting
-     *
-     * @return <code>true</code> if the caption text is to be rendered as HTML,
-     *         <code>false</code> otherwise
-     * @deprecated as of , use {@link #isCaptionAsHtml()} instead
-     */
-    @Deprecated
-    public boolean isHtmlContentAllowed() {
-        return isCaptionAsHtml();
-    }
-
     @Override
     public void readDesign(Element design, DesignContext designContext) {
         super.readDesign(design, designContext);
@@ -554,7 +496,7 @@ public abstract class AbstractColorPicker extends AbstractComponent
             String hexColor = DesignAttributeHandler
                     .readAttribute("color", attributes, String.class)
                     .substring(1);
-            setColor(new Color(Integer.parseInt(hexColor, 16)));
+            doSetValue(new Color(Integer.parseInt(hexColor, 16)));
         }
         if (design.hasAttr("popup-style")) {
             setPopupStyle(PopupStyle.valueOf(
@@ -573,7 +515,7 @@ public abstract class AbstractColorPicker extends AbstractComponent
 
         Attributes attribute = design.attributes();
         DesignAttributeHandler.writeAttribute("color", attribute,
-                color.getCSS(), Color.WHITE.getCSS(), String.class);
+                getValue().getCSS(), Color.WHITE.getCSS(), String.class);
         DesignAttributeHandler.writeAttribute("popup-style", attribute,
                 (popupStyle == PopupStyle.POPUP_NORMAL ? "normal" : "simple"),
                 "normal", String.class);
@@ -589,4 +531,10 @@ public abstract class AbstractColorPicker extends AbstractComponent
         result.add("popup-style");
         return result;
     }
+
+    @Override
+    protected void doSetValue(Color color) {
+        this.color = color;
+        getState().color = color.getCSS();
+    }
 }
diff --git a/server/src/main/java/com/vaadin/ui/ColorPicker.java b/server/src/main/java/com/vaadin/ui/ColorPicker.java
index 67002373d0..706fd6518b 100644
--- a/server/src/main/java/com/vaadin/ui/ColorPicker.java
+++ b/server/src/main/java/com/vaadin/ui/ColorPicker.java
@@ -63,5 +63,4 @@ public class ColorPicker extends AbstractColorPicker {
         setPrimaryStyleName(STYLENAME_BUTTON);
         addStyleName(STYLENAME_DEFAULT);
     }
-
 }
diff --git a/server/src/test/java/com/vaadin/tests/server/component/colorpicker/AbstractColorPickerDeclarativeTest.java b/server/src/test/java/com/vaadin/tests/server/component/colorpicker/AbstractColorPickerDeclarativeTest.java
index 2e3b8e412e..8018d6603e 100644
--- a/server/src/test/java/com/vaadin/tests/server/component/colorpicker/AbstractColorPickerDeclarativeTest.java
+++ b/server/src/test/java/com/vaadin/tests/server/component/colorpicker/AbstractColorPickerDeclarativeTest.java
@@ -34,7 +34,7 @@ public class AbstractColorPickerDeclarativeTest
                 + " history-visibility=false textfield-visibility=false />";
         ColorPicker colorPicker = new ColorPicker();
         int colorInt = Integer.parseInt("fafafa", 16);
-        colorPicker.setColor(new Color(colorInt));
+        colorPicker.setValue(new Color(colorInt));
         colorPicker.setDefaultCaptionEnabled(true);
         colorPicker.setPosition(100, 100);
         colorPicker.setPopupStyle(PopupStyle.POPUP_SIMPLE);
@@ -63,7 +63,7 @@ public class AbstractColorPickerDeclarativeTest
                 + " history-visibility=false textfield-visibility=false />";
         AbstractColorPicker colorPicker = new ColorPickerArea();
         int colorInt = Integer.parseInt("fafafa", 16);
-        colorPicker.setColor(new Color(colorInt));
+        colorPicker.setValue(new Color(colorInt));
         colorPicker.setDefaultCaptionEnabled(true);
         colorPicker.setPosition(100, 100);
         colorPicker.setPopupStyle(PopupStyle.POPUP_SIMPLE);