Some improvements to the color selection UI (#1019)

* Tab and Shift+Tab keys cycle through RGB/HSB text fields
* Sliders don't get the keyboard focus (only text fields)
* Up/Down keys increase/decrease the text field value

src/app/commands/cmd_palette_editor.cpp

@@ -251,10 +251,10 @@ PaletteEntryEditor::PaletteEntryEditor()
   , m_selfPalChange(false)
   , m_fromPalette(0, 0)
 {
-  m_colorType.addItem("RGB");
-  m_colorType.addItem("HSB");
-  m_changeMode.addItem("Abs");
-  m_changeMode.addItem("Rel");
+  m_colorType.addItem("RGB")->setFocusStop(false);
+  m_colorType.addItem("HSB")->setFocusStop(false);
+  m_changeMode.addItem("Abs")->setFocusStop(false);
+  m_changeMode.addItem("Rel")->setFocusStop(false);
 
   m_topBox.setBorder(gfx::Border(0));
   m_topBox.setChildSpacing(0);

src/app/ui/color_popup.cpp

@@ -58,11 +58,11 @@ ColorPopup::ColorPopup(bool canPin)
   , m_canPin(canPin)
   , m_disableHexUpdate(false)
 {
-  m_colorType.addItem("Index");
-  m_colorType.addItem("RGB");
-  m_colorType.addItem("HSB");
-  m_colorType.addItem("Gray");
-  m_colorType.addItem("Mask");
+  m_colorType.addItem("Index")->setFocusStop(false);
+  m_colorType.addItem("RGB")->setFocusStop(false);
+  m_colorType.addItem("HSB")->setFocusStop(false);
+  m_colorType.addItem("Gray")->setFocusStop(false);
+  m_colorType.addItem("Mask")->setFocusStop(false);
 
   m_topBox.setBorder(gfx::Border(0));
   m_topBox.setChildSpacing(0);

src/app/ui/color_sliders.cpp

@@ -13,6 +13,7 @@
 #include "app/ui/skin/skin_slider_property.h"
 #include "app/ui/skin/skin_theme.h"
 #include "base/bind.h"
+#include "base/scoped_value.h"
 #include "ui/box.h"
 #include "ui/entry.h"
 #include "ui/graphics.h"
@@ -84,26 +85,100 @@ namespace {
 
   class ColorEntry : public Entry {
   public:
-    ColorEntry() : Entry(4, "0") {
+    ColorEntry(Slider* absSlider, Slider* relSlider)
+      : Entry(4, "0")
+      , m_absSlider(absSlider)
+      , m_relSlider(relSlider)
+      , m_recent_focus(false) {
+    }
+
+  private:
+    int minValue() const {
+      if (m_absSlider->isVisible())
+        return m_absSlider->getMinValue();
+      else if (m_relSlider->isVisible())
+        return m_relSlider->getMinValue();
+      else
+        return 0;
+    }
+
+    int maxValue() const {
+      if (m_absSlider->isVisible())
+        return m_absSlider->getMaxValue();
+      else if (m_relSlider->isVisible())
+        return m_relSlider->getMaxValue();
+      else
+        return 0;
     }
 
     bool onProcessMessage(Message* msg) override {
       switch (msg->type()) {
 
+        case kFocusEnterMessage:
+          m_recent_focus = true;
+          break;
+
         case kKeyDownMessage:
           if (Entry::onProcessMessage(msg))
             return true;
-          // Process focus movement key here because if our
-          // CustomizedGuiManager catches this kKeyDownMessage it will
-          // process it as a shortcut to switch the Timeline.
-          else if (manager()->processFocusMovementMessage(msg))
-            return true;
-          else
-            return false;
+
+          if (hasFocus()) {
+            int scancode = static_cast<KeyMessage*>(msg)->scancode();
+
+            switch (scancode) {
+              // Enter just remove the focus
+              case kKeyEnter:
+              case kKeyEnterPad:
+                releaseFocus();
+                return true;
+
+              case kKeyDown:
+              case kKeyUp: {
+                int value = textInt();
+                if (scancode == kKeyDown)
+                  --value;
+                else
+                  ++value;
+
+                setTextf("%d", MID(minValue(), value, maxValue()));
+                selectAllText();
+
+                onChange();
+                return true;
+              }
+            }
+
+            // Process focus movement key here because if our
+            // CustomizedGuiManager catches this kKeyDownMessage it
+            // will process it as a shortcut to switch the Timeline.
+            //
+            // Note: The default ui::Manager handles focus movement
+            // shortcuts only for foreground windows.
+            // TODO maybe that should change
+            if (hasFocus() &&
+                manager()->processFocusMovementMessage(msg))
+              return true;
+          }
+          return false;
       }
-      return Entry::onProcessMessage(msg);
+
+      bool result = Entry::onProcessMessage(msg);
+
+      if (msg->type() == kMouseDownMessage && m_recent_focus) {
+        m_recent_focus = false;
+        selectAllText();
+      }
+
+      return result;
     }
 
+    Slider* m_absSlider;
+    Slider* m_relSlider;
+
+    // TODO remove this calling setFocus() in
+    //      Widget::onProcessMessage() instead of
+    //      Manager::handleWindowZOrder()
+    bool m_recent_focus;
   };
 
 }
@@ -115,6 +190,7 @@ ColorSliders::ColorSliders()
   : Widget(kGenericWidget)
   , m_grid(3, false)
   , m_mode(Absolute)
+  , m_lockEntry(-1)
 {
   addChild(&m_grid);
   m_grid.setChildSpacing(0);
@@ -161,7 +237,7 @@ void ColorSliders::addSlider(Channel channel, const char* labelText, int min, in
   Label*  label     = new Label(labelText);
   Slider* absSlider = new Slider(min, max, 0);
   Slider* relSlider = new Slider(min-max, max-min, 0);
-  Entry*  entry     = new ColorEntry();
+  Entry*  entry     = new ColorEntry(absSlider, relSlider);
 
   m_label.push_back(label);
   m_absSlider.push_back(absSlider);
@@ -180,6 +256,8 @@ void ColorSliders::addSlider(Channel channel, const char* labelText, int min, in
   HBox* box = new HBox();
   box->addChild(absSlider);
   box->addChild(relSlider);
+  absSlider->setFocusStop(false);
+  relSlider->setFocusStop(false);
   absSlider->setExpansive(true);
   relSlider->setExpansive(true);
   relSlider->setVisible(false);
@@ -218,6 +296,8 @@ void ColorSliders::onSliderChange(int i)
 
 void ColorSliders::onEntryChange(int i)
 {
+  base::ScopedValue<int> lock(m_lockEntry, i, m_lockEntry);
+
   // Update the slider related to the changed entry widget.
   int value = m_entry[i]->textInt();
 
@@ -245,10 +325,15 @@ void ColorSliders::onControlChange(int i)
 // Updates the entry related to the changed slider widget.
 void ColorSliders::updateEntryText(int entryIndex)
 {
+  if (m_lockEntry == entryIndex)
+    return;
+
   Slider* slider = (m_mode == Absolute ? m_absSlider[entryIndex]:
                                          m_relSlider[entryIndex]);
 
   m_entry[entryIndex]->setTextf("%d", slider->getValue());
+  if (m_entry[entryIndex]->hasFocus())
+    m_entry[entryIndex]->selectAllText();
 }
 
 void ColorSliders::updateSlidersBgColor(const app::Color& color)

src/app/ui/color_sliders.h

@@ -72,6 +72,7 @@ namespace app {
     std::vector<Channel> m_channel;
     ui::Grid m_grid;
     Mode m_mode;
+    int m_lockEntry;
   };
 
   //////////////////////////////////////////////////////////////////////

src/app/ui/hex_color_entry.cpp

@@ -14,21 +14,41 @@
 #include "app/ui/hex_color_entry.h"
 #include "base/hex.h"
 #include "gfx/border.h"
+#include "ui/message.h"
 #include "ui/theme.h"
 
 namespace app {
 
 using namespace ui;
 
+HexColorEntry::CustomEntry::CustomEntry()
+  : Entry(16, "")
+{
+}
+
+bool HexColorEntry::CustomEntry::onProcessMessage(ui::Message* msg)
+{
+  switch (msg->type()) {
+    case kMouseDownMessage:
+      setFocusStop(true);
+      requestFocus();
+      break;
+    case kFocusLeaveMessage:
+      setFocusStop(false);
+      break;
+  }
+  return Entry::onProcessMessage(msg);
+}
+
 HexColorEntry::HexColorEntry()
   : Box(HORIZONTAL)
   , m_label("#")
-  , m_entry(16, "")
 {
   addChild(&m_label);
   addChild(&m_entry);
 
   m_entry.Change.connect(&HexColorEntry::onEntryChange, this);
+  m_entry.setFocusStop(false);
 
   initTheme();
 

src/app/ui/hex_color_entry.h

@@ -30,8 +30,15 @@ namespace app {
     void onEntryChange();
 
   private:
+    class CustomEntry : public ui::Entry {
+    public:
+      CustomEntry();
+    private:
+      bool onProcessMessage(ui::Message* msg) override;
+    };
+
     ui::Label m_label;
-    ui::Entry m_entry;
+    CustomEntry m_entry;
   };
 
 } // namespace app

src/app/ui/popup_window_pin.cpp

@@ -32,6 +32,7 @@ PopupWindowPin::PopupWindowPin(const std::string& text, ClickBehavior clickBehav
 {
   SkinTheme* theme = SkinTheme::instance();
 
+  m_pin.setFocusStop(false);
   m_pin.Click.connect(&PopupWindowPin::onPinClick, this);
   m_pin.setIconInterface(
     new ButtonIconImpl(theme->parts.unpinned(),

src/app/ui/skin/skin_theme.cpp

@@ -895,9 +895,13 @@ void SkinTheme::paintCheckBox(PaintEvent& ev)
   if (iconInterface)
     paintIcon(widget, g, iconInterface, icon.x, icon.y);
 
-  // draw focus
-  if (look != WithoutBordersLook && widget->hasFocus())
+  // Draw focus
+  if (look != WithoutBordersLook &&
+      (widget->hasFocus() || (iconInterface &&
+                              widget->text().empty() &&
+                              widget->hasMouseOver()))) {
     drawRect(g, bounds, parts.checkFocus().get(), gfx::ColorNone);
+  }
 }
 
 void SkinTheme::paintGrid(PaintEvent& ev)