Fix WheelAction::ToolSameGroup/ToolOtherGroup in DraggingValueState (fix #3459)

These actions weren't working correctly because we have to take
special care when we change the tool group, i.e. the "initial
position" used to calculate the delta from "mouse pos - initial pos"
must be updated to reset the delta to change tools inside the same
tool (this is the new m_initialPosSameGroup member variable).

src/app/ui/editor/dragging_value_state.cpp

@@ -10,7 +10,9 @@
 
 #include "app/ui/editor/dragging_value_state.h"
 
+#include "app/tools/tool.h"
 #include "app/ui/editor/editor.h"
+#include "app/ui/toolbar.h"
 #include "ui/display.h"
 #include "ui/message.h"
 #include "ui/scale.h"
@@ -25,6 +27,7 @@ using namespace ui;
 DraggingValueState::DraggingValueState(Editor* editor, const Keys& keys)
   : m_keys(keys)
   , m_initialPos(editor->display()->nativeWindow()->pointFromScreen(ui::get_mouse_position()))
+  , m_initialPosSameGroup(m_initialPos)
   , m_initialFgColor(StateWithWheelBehavior::initialFgColor())
   , m_initialBgColor(StateWithWheelBehavior::initialBgColor())
   , m_initialFgTileIndex(StateWithWheelBehavior::initialFgTileIndex())
@@ -71,7 +74,13 @@ bool DraggingValueState::onMouseMove(Editor* editor, MouseMessage* msg)
   m_fgColor = m_initialFgColor;
 
   for (const KeyPtr& key : m_keys) {
-    const gfx::Point delta = (msg->position() - m_initialPos);
+    gfx::Point initialPos;
+    if (key->wheelAction() == WheelAction::ToolSameGroup)
+      initialPos = m_initialPosSameGroup;
+    else
+      initialPos = m_initialPos;
+
+    const gfx::Point delta = (msg->position() - initialPos);
     const DragVector deltaV(delta.x, delta.y);
     const DragVector invDragVector(key->dragVector().x,
                                    -key->dragVector().y);
@@ -144,4 +153,31 @@ void DraggingValueState::changeFgColor(Color c)
   m_fgColor = c;
 }
 
+tools::Tool* DraggingValueState::getInitialToolInActiveGroup()
+{
+  return StateWithWheelBehavior::getInitialToolInActiveGroup();
+}
+
+void DraggingValueState::onToolChange(tools::Tool* tool)
+{
+  ToolBar::instance()->selectTool(tool);
+}
+
+void DraggingValueState::onToolGroupChange(Editor* editor,
+                                           tools::ToolGroup* group)
+{
+  if (getActiveTool()->getGroup() != group) {
+    StateWithWheelBehavior::onToolGroupChange(editor, group);
+
+    // Update reference initial position to change tools in the same
+    // group. Useful when the same key modifiers are associated to
+    // WheelAction::ToolSameGroup and WheelAction::ToolOtherGroup at
+    // the same time. This special position is needed to avoid jumping
+    // "randomly" to other tools when we change to another group (as
+    // the delta from the m_initialPos is accumulated).
+    m_initialPosSameGroup = editor->display()->nativeWindow()
+      ->pointFromScreen(ui::get_mouse_position());
+  }
+}
+
 } // namespace app

src/app/ui/editor/dragging_value_state.h

@@ -48,8 +48,14 @@ namespace app {
     tools::Tool* initialTool() const override { return m_initialTool; }
     void changeFgColor(Color c) override;
 
+    tools::Tool* getInitialToolInActiveGroup() override;
+    void onToolChange(tools::Tool* tool) override;
+    void onToolGroupChange(Editor* editor,
+                           tools::ToolGroup* group) override;
+
     Keys m_keys;
     gfx::Point m_initialPos;
+    gfx::Point m_initialPosSameGroup;
 
     Color m_initialFgColor;
     Color m_initialBgColor;

src/app/ui/editor/state_with_wheel_behavior.cpp

@@ -64,7 +64,6 @@ static inline void adjust_unit(bool preciseWheel, double dz, double& v)
 }
 
 StateWithWheelBehavior::StateWithWheelBehavior()
-  : m_groupTool(initialTool())
 {
 }
 
@@ -308,7 +307,7 @@ void StateWithWheelBehavior::processWheelAction(
     }
 
     case WheelAction::ToolSameGroup: {
-      const tools::Tool* tool = m_groupTool;
+      const tools::Tool* tool = getInitialToolInActiveGroup();
 
       auto toolBox = App::instance()->toolBox();
       std::vector<tools::Tool*> tools;
@@ -322,24 +321,25 @@ void StateWithWheelBehavior::processWheelAction(
       auto it = std::find(begin, end, tool);
       if (it != end) {
         int i = std::round(dz);
-        while (i++ < 0) {
+        while (i < 0) {
+          ++i;
           if (it == begin)
             it = end;
           --it;
         }
-        while (i-- > 0) {
+        while (i > 0) {
+          --i;
           ++it;
           if (it == end)
             it = begin;
         }
-        if (tool != *it)
+        onToolChange(*it);
-          ToolBar::instance()->selectTool(*it);
       }
       break;
     }
 
     case WheelAction::ToolOtherGroup: {
-      tools::Tool* tool = initialTool();
+      const tools::Tool* tool = initialTool();
 
       auto toolBox = App::instance()->toolBox();
       auto begin = toolBox->begin_group();
@@ -347,18 +347,19 @@ void StateWithWheelBehavior::processWheelAction(
       auto it = std::find(begin, end, tool->getGroup());
       if (it != end) {
         int i = std::round(dz);
-        while (i++ < 0) {
+        while (i < 0) {
+          ++i;
           if (it == begin)
             it = end;
           --it;
         }
-        while (i-- > 0) {
+        while (i > 0) {
+          --i;
           ++it;
           if (it == end)
             it = begin;
         }
-        ToolBar::instance()->selectToolGroup(*it);
+        onToolGroupChange(editor, *it);
-        m_groupTool = getActiveTool();
       }
       break;
     }
@@ -721,4 +722,29 @@ void StateWithWheelBehavior::disableQuickTool() const
   }
 }
 
+tools::Tool* StateWithWheelBehavior::getInitialToolInActiveGroup()
+{
+  if (!m_tool)
+    m_tool = initialTool();
+  return m_tool;
+}
+
+void StateWithWheelBehavior::onToolChange(tools::Tool* tool)
+{
+  ToolBar::instance()->selectTool(tool);
+  m_tool = tool;
+}
+
+void StateWithWheelBehavior::onToolGroupChange(Editor* editor,
+                                               tools::ToolGroup* group)
+{
+  ToolBar::instance()->selectToolGroup(group);
+
+  // Update initial tool in active group as the group has just
+  // changed. Useful when the same key modifiers are associated to
+  // WheelAction::ToolSameGroup and WheelAction::ToolOtherGroup at the
+  // same time.
+  m_tool = getActiveTool();
+}
+
 } // namespace app

src/app/ui/editor/state_with_wheel_behavior.h

@@ -21,12 +21,13 @@ namespace render {
   class Zoom;
 }
 
-namespace tools {
-  class Tool;
-}
-
 namespace app {
 
+  namespace tools {
+    class Tool;
+    class ToolGroup;
+  }
+
   class StateWithWheelBehavior : public EditorState {
   public:
     StateWithWheelBehavior();
@@ -62,13 +63,19 @@ namespace app {
     virtual tools::Tool* initialTool() const;
     virtual void changeFgColor(Color c);
 
+    virtual tools::Tool* getInitialToolInActiveGroup();
+    virtual void onToolChange(tools::Tool* tool);
+    virtual void onToolGroupChange(Editor* editor,
+                                   tools::ToolGroup* group);
+
+    tools::Tool* getActiveTool() const;
+
   private:
     void setZoom(Editor* editor, const render::Zoom& zoom, const gfx::Point& mousePos);
-    tools::Tool* getActiveTool() const;
     void disableQuickTool() const;
 
     mutable doc::LayerList m_browsableLayers;
-    tools::Tool* m_groupTool;
+    tools::Tool* m_tool = nullptr;
   };
 
 } // namespace app