Merge pull request #2004 from idotobi/fix-color-selection-when-eraser-is-active

Change color selection behaviour if Eraser tool is selected via a button

src/control/LastSelectedTool.cpp

@@ -1,16 +0,0 @@
-#include "LastSelectedTool.h"
-
-#include "Tool.h"
-
-LastSelectedTool::LastSelectedTool(Tool* tool): tool(tool) { this->applyFrom(tool); }
-
-LastSelectedTool::~LastSelectedTool() = default;
-
-/**
- * Restore the tool config and return it
- */
-auto LastSelectedTool::restoreAndGet() -> Tool* {
-    tool->applyFrom(this);
-
-    return tool;
-}

src/control/LastSelectedTool.h

@@ -1,38 +0,0 @@
-/*
- * Xournal++
- *
- * Handle last selected tool config, if the tool is changed e.g. by right click
- *
- * @author Xournal++ Team
- * https://github.com/xournalpp/xournalpp
- *
- * @license GNU GPLv2 or later
- */
-
-#pragma once
-
-#include <string>
-#include <vector>
-
-#include "ToolBase.h"
-#include "XournalType.h"
-
-class Tool;
-
-class LastSelectedTool: public ToolBase {
-public:
-    LastSelectedTool(Tool* tool);
-    virtual ~LastSelectedTool();
-
-public:
-    /**
-     * Restore the tool config and return it
-     */
-    Tool* restoreAndGet();
-
-private:
-    /**
-     * The last tool
-     */
-    Tool* tool;
-};

src/control/ToolBase.cpp

@@ -4,18 +4,6 @@ ToolBase::ToolBase() = default;
 
 ToolBase::~ToolBase() = default;
 
-/**
- * Apply data from another ToolBase or any extending class
- */
-void ToolBase::applyFrom(const ToolBase* t) {
-    this->color = t->color;
-    this->size = t->size;
-    this->drawingType = t->drawingType;
-    this->fill = t->fill;
-    this->fillAlpha = t->fillAlpha;
-    this->lineStyle = t->lineStyle;
-}
-
 /**
  * @return Color of the tool for all drawing tools
  */

src/control/ToolBase.h

@@ -26,11 +26,6 @@ public:
     ToolBase();
     virtual ~ToolBase();
 
-    /**
-     * Apply data from another ToolBase or any extending class
-     */
-    void applyFrom(const ToolBase* t);
-
 private:
     void operator=(const ToolBase& t);
     ToolBase(const ToolBase& t);

src/control/ToolHandler.cpp

@@ -8,7 +8,6 @@
 #include "model/StrokeStyle.h"
 
 #include "Actions.h"
-#include "LastSelectedTool.h"
 #include "Util.h"
 
 ToolListener::~ToolListener() = default;
@@ -96,8 +95,9 @@ void ToolHandler::initTools() {
     tools[TOOL_FLOATING_TOOLBOX - TOOL_PEN] = std::make_unique<Tool>("showFloatingToolbox", TOOL_FLOATING_TOOLBOX,
                                                                      Color{0x000000U}, TOOL_CAP_NONE, nullptr);
 
-
-    selectTool(TOOL_PEN);
+    this->buttonSelectedTool = &getTool(TOOL_PEN);
+    this->toolbarSelectedTool = &getTool(TOOL_PEN);
+    this->currentTool = &getTool(TOOL_PEN);
 }
 
 ToolHandler::~ToolHandler() {
@@ -133,12 +133,18 @@ void ToolHandler::eraserTypeChanged() {
 
 auto ToolHandler::getEraserType() -> EraserType { return this->eraserType; }
 
-void ToolHandler::selectTool(ToolType type, bool fireToolChanged) {
+void ToolHandler::selectTool(ToolType type, bool fireToolChanged, bool triggeredByButton) {
     if (type < 1 || type > TOOL_COUNT) {
         g_warning("unknown tool selected: %i\n", type);
         return;
     }
-    this->current = tools[type - TOOL_PEN].get();
+    this->triggeredByButton = triggeredByButton;
+    if (this->triggeredByButton) {
+        this->buttonSelectedTool = &getTool(type);
+    } else {
+        this->toolbarSelectedTool = &getTool(type);
+    }
+    this->currentTool = &getTool(type);
 
     if (fireToolChanged) {
         this->fireToolChanged();
@@ -153,11 +159,20 @@ void ToolHandler::fireToolChanged() {
 
 auto ToolHandler::getTool(ToolType type) -> Tool& { return *(this->tools[type - TOOL_PEN]); }
 
-auto ToolHandler::getToolType() -> ToolType { return this->current->type; }
+auto ToolHandler::getToolType(ToolPointer toolPointer) -> ToolType {
+    Tool* tool = getToolPointer(toolPointer);
+    return tool->type;
+}
 
-auto ToolHandler::hasCapability(ToolCapabilities cap) -> bool { return (current->capabilities & cap) != 0; }
+auto ToolHandler::hasCapability(ToolCapabilities cap, ToolPointer toolPointer) -> bool {
+    Tool* tool = getToolPointer(toolPointer);
+    return (tool->capabilities & cap) != 0;
+}
 
-auto ToolHandler::getSize() -> ToolSize { return this->current->getSize(); }
+auto ToolHandler::getSize(ToolPointer toolPointer) -> ToolSize {
+    Tool* tool = getToolPointer(toolPointer);
+    return tool->getSize();
+}
 
 auto ToolHandler::getPenSize() -> ToolSize { return tools[TOOL_PEN - TOOL_PEN]->getSize(); }
 
@@ -168,7 +183,7 @@ auto ToolHandler::getHilighterSize() -> ToolSize { return tools[TOOL_HILIGHTER -
 void ToolHandler::setPenSize(ToolSize size) {
     this->tools[TOOL_PEN - TOOL_PEN]->setSize(size);
 
-    if (this->current->type == TOOL_PEN) {
+    if (this->currentTool->type == TOOL_PEN) {
         this->listener->toolSizeChanged();
     }
 }
@@ -176,7 +191,7 @@ void ToolHandler::setPenSize(ToolSize size) {
 void ToolHandler::setEraserSize(ToolSize size) {
     this->tools[TOOL_ERASER - TOOL_PEN]->setSize(size);
 
-    if (this->current->type == TOOL_ERASER) {
+    if (this->currentTool->type == TOOL_ERASER) {
         this->listener->toolSizeChanged();
     }
 }
@@ -184,7 +199,7 @@ void ToolHandler::setEraserSize(ToolSize size) {
 void ToolHandler::setHilighterSize(ToolSize size) {
     this->tools[TOOL_HILIGHTER - TOOL_PEN]->setSize(size);
 
-    if (this->current->type == TOOL_HILIGHTER) {
+    if (this->currentTool->type == TOOL_HILIGHTER) {
         this->listener->toolSizeChanged();
     }
 }
@@ -192,7 +207,7 @@ void ToolHandler::setHilighterSize(ToolSize size) {
 void ToolHandler::setPenFillEnabled(bool fill, bool fireEvent) {
     this->tools[TOOL_PEN - TOOL_PEN]->setFill(fill);
 
-    if (this->current->type == TOOL_PEN && fireEvent) {
+    if (this->currentTool->type == TOOL_PEN && fireEvent) {
         this->listener->toolFillChanged();
     }
 }
@@ -206,7 +221,7 @@ auto ToolHandler::getPenFill() -> int { return this->tools[TOOL_PEN - TOOL_PEN]-
 void ToolHandler::setHilighterFillEnabled(bool fill, bool fireEvent) {
     this->tools[TOOL_HILIGHTER - TOOL_PEN]->setFill(fill);
 
-    if (this->current->type == TOOL_HILIGHTER && fireEvent) {
+    if (this->currentTool->type == TOOL_HILIGHTER && fireEvent) {
         this->listener->toolFillChanged();
     }
 }
@@ -217,23 +232,23 @@ void ToolHandler::setHilighterFill(int alpha) { this->tools[TOOL_HILIGHTER - TOO
 
 auto ToolHandler::getHilighterFill() -> int { return this->tools[TOOL_HILIGHTER - TOOL_PEN]->getFillAlpha(); }
 
-auto ToolHandler::getThickness() -> double {
-    if (this->current->thickness) {
-        return this->current->thickness[this->current->getSize()];
+auto ToolHandler::getThickness(ToolPointer toolPointer) -> double {
+    Tool* tool = getToolPointer(toolPointer);
+    if (tool->thickness) {
+        return tool->thickness[tool->getSize()];
     }
 
-
-    g_warning("Request size of \"%s\"", this->current->getName().c_str());
+    g_warning("Request size of \"%s\"", tool->getName().c_str());
     return 0;
 }
 
-void ToolHandler::setSize(ToolSize size) {
+void ToolHandler::setSize(ToolSize size, ToolPointer toolPointer) {
+    Tool* tool = getToolPointer(toolPointer);
     if (size < TOOL_SIZE_VERY_FINE || size > TOOL_SIZE_VERY_THICK) {
         g_warning("ToolHandler::setSize: Invalid size! %i", size);
         return;
     }
-
-    this->current->setSize(size);
+    tool->setSize(size);
     this->listener->toolSizeChanged();
 }
 
@@ -245,6 +260,9 @@ void ToolHandler::setLineStyle(const LineStyle& style) {
 /**
  * Select the color for the tool
  *
+ * If the buttonSelectedTool is the Eraser changing the color has no effect on the Eraser.
+ * In this case the toolbarSelectedTool's color will be changed instead.
+ *
  * @param color Color
  * @param userSelection
  * 			true if the user selected the color
@@ -252,29 +270,45 @@ void ToolHandler::setLineStyle(const LineStyle& style) {
  * 			and therefore should not be applied to a selection
  */
 void ToolHandler::setColor(Color color, bool userSelection) {
-    this->current->setColor(color);
+    if ((this->buttonSelectedTool->capabilities & TOOL_CAP_COLOR) == 0) {
+        this->toolbarSelectedTool->setColor(color);
+    }
+    this->currentTool->setColor(color);
     this->listener->toolColorChanged(userSelection);
     this->listener->setCustomColorSelected();
 }
 
-auto ToolHandler::getColor() -> Color { return current->getColor(); }
+auto ToolHandler::getColor(ToolPointer toolPointer) -> Color {
+    Tool* tool = getToolPointer(toolPointer);
+    return tool->getColor();
+}
 
 /**
  * @return -1 if fill is disabled, else the fill alpha value
  */
-auto ToolHandler::getFill() -> int {
-    if (!current->getFill()) {
+auto ToolHandler::getFill(ToolPointer toolPointer) -> int {
+    Tool* tool = getToolPointer(toolPointer);
+    if (!tool->getFill()) {
         return -1;
     }
-
-    return current->getFillAlpha();
+    return tool->getFillAlpha();
 }
 
-auto ToolHandler::getLineStyle() -> const LineStyle& { return current->getLineStyle(); }
+auto ToolHandler::getLineStyle(ToolPointer toolPointer) -> const LineStyle& {
+    Tool* tool = getToolPointer(toolPointer);
+    return tool->getLineStyle();
+}
 
-auto ToolHandler::getDrawingType() -> DrawingType { return current->getDrawingType(); }
+auto ToolHandler::getDrawingType(ToolPointer toolPointer) -> DrawingType {
+    Tool* tool = getToolPointer(toolPointer);
+    return tool->getDrawingType();
+}
 
-void ToolHandler::setDrawingType(DrawingType drawingType) { current->setDrawingType(drawingType); }
+// TODO
+void ToolHandler::setDrawingType(DrawingType drawingType, ToolPointer toolPointer) {
+    Tool* tool = getToolPointer(toolPointer);
+    tool->setDrawingType(drawingType);
+}
 
 auto ToolHandler::getTools() const -> std::array<std::unique_ptr<Tool>, TOOL_COUNT> const& { return tools; }
 
@@ -282,7 +316,7 @@ void ToolHandler::saveSettings() {
     SElement& s = settings->getCustomElement("tools");
     s.clear();
 
-    s.setString("current", this->current->getName());
+    s.setString("current", this->currentTool->getName());
 
     for (auto&& tool: tools) {
         SElement& st = s.child(tool->getName());
@@ -350,7 +384,7 @@ void ToolHandler::loadSettings() {
             SElement& st = s.child(tool->getName());
 
             if (selectedTool == tool->getName()) {
-                this->current = tool.get();
+                this->currentTool = tool.get();
             }
 
             int iColor{};
@@ -412,22 +446,10 @@ void ToolHandler::loadSettings() {
     }
 }
 
-void ToolHandler::copyCurrentConfig() {
-    // If there is no last config, create one, if there is already one
-    // do not overwrite this config!
-    if (this->lastSelectedTool == nullptr) {
-        this->lastSelectedTool = new LastSelectedTool(this->current);
-    }
-}
+void ToolHandler::pointCurrentToolToButtonTool() { this->currentTool = this->buttonSelectedTool; }
 
-void ToolHandler::restoreLastConfig() {
-    if (this->lastSelectedTool == nullptr) {
-        return;
-    }
-
-    this->current = this->lastSelectedTool->restoreAndGet();
-    delete this->lastSelectedTool;
-    this->lastSelectedTool = nullptr;
+void ToolHandler::pointCurrentToolToToolbarTool() {
+    this->currentTool = this->toolbarSelectedTool;
 
     this->listener->toolColorChanged(false);
     this->listener->toolSizeChanged();
@@ -451,8 +473,8 @@ void ToolHandler::setSelectionEditTools(bool setColor, bool setSize, bool setFil
         t->setFill(false);
     }
 
-    if (this->current->type == TOOL_SELECT_RECT || this->current->type == TOOL_SELECT_REGION ||
-        this->current->type == TOOL_SELECT_OBJECT || this->current->type == TOOL_PLAY_OBJECT) {
+    if (this->currentTool->type == TOOL_SELECT_RECT || this->currentTool->type == TOOL_SELECT_REGION ||
+        this->currentTool->type == TOOL_SELECT_OBJECT || this->currentTool->type == TOOL_PLAY_OBJECT) {
         this->listener->toolColorChanged(false);
         this->listener->toolSizeChanged();
         this->listener->toolFillChanged();
@@ -460,9 +482,9 @@ void ToolHandler::setSelectionEditTools(bool setColor, bool setSize, bool setFil
     }
 }
 
-auto ToolHandler::isSinglePageTool() -> bool {
-    ToolType toolType = this->getToolType();
-    DrawingType drawingType = this->getDrawingType();
+auto ToolHandler::isSinglePageTool(ToolPointer toolPointer) -> bool {
+    ToolType toolType = this->getToolType(toolPointer);
+    DrawingType drawingType = this->getDrawingType(toolPointer);
 
     return toolType == (TOOL_PEN && (drawingType == DRAWING_TYPE_ARROW || drawingType == DRAWING_TYPE_CIRCLE ||
                                      drawingType == DRAWING_TYPE_COORDINATE_SYSTEM ||
@@ -472,3 +494,16 @@ auto ToolHandler::isSinglePageTool() -> bool {
            toolType == TOOL_DRAW_COORDINATE_SYSTEM || toolType == TOOL_DRAW_ARROW ||
            toolType == TOOL_FLOATING_TOOLBOX || toolType == TOOL_DRAW_SPLINE;
 }
+
+auto ToolHandler::getToolPointer(ToolPointer toolpointer) -> Tool* {
+    switch (toolpointer) {
+        case ToolPointer::current:
+            return this->currentTool;
+        case ToolPointer::toolbar:
+            return this->toolbarSelectedTool;
+        case ToolPointer::button:
+            return this->buttonSelectedTool;
+        default:
+            g_error("This ToolPointer does not exist.");
+    }
+}

src/control/ToolHandler.h

@@ -23,7 +23,7 @@
 #include "Tool.h"
 #include "XournalType.h"
 
-class LastSelectedTool;
+enum ToolPointer { current, toolbar, button };
 
 class ToolListener {
 public:
@@ -54,21 +54,21 @@ public:
      * 			and therefore should not be applied to a selection
      */
     void setColor(Color color, bool userSelection);
-    Color getColor();
+    Color getColor(ToolPointer toolpointer = ToolPointer::current);
 
     /**
      * @return -1 if fill is disabled, else the fill alpha value
      */
-    int getFill();
+    int getFill(ToolPointer toolpointer = ToolPointer::current);
 
-    DrawingType getDrawingType();
-    void setDrawingType(DrawingType drawingType);
+    DrawingType getDrawingType(ToolPointer toolpointer = ToolPointer::current);
+    void setDrawingType(DrawingType drawingType, ToolPointer toolpointer = ToolPointer::current);
 
-    const LineStyle& getLineStyle();
+    const LineStyle& getLineStyle(ToolPointer toolpointer = ToolPointer::current);
 
-    ToolSize getSize();
-    void setSize(ToolSize size);
-    double getThickness();
+    ToolSize getSize(ToolPointer toolpointer = ToolPointer::current);
+    void setSize(ToolSize size, ToolPointer toolpointer = ToolPointer::current);
+    double getThickness(ToolPointer toolpointer = ToolPointer::current);
 
     void setLineStyle(const LineStyle& style);
 
@@ -89,8 +89,8 @@ public:
     void setHilighterFill(int alpha);
     int getHilighterFill();
 
-    void selectTool(ToolType type, bool fireToolChanged = true);
-    ToolType getToolType();
+    void selectTool(ToolType type, bool fireToolChanged = true, bool stylus = false);
+    ToolType getToolType(ToolPointer toolpointer = ToolPointer::current);
     void fireToolChanged();
 
     Tool& getTool(ToolType type);
@@ -99,13 +99,13 @@ public:
     EraserType getEraserType();
     void eraserTypeChanged();
 
-    bool hasCapability(ToolCapabilities cap);
+    bool hasCapability(ToolCapabilities cap, ToolPointer toolpointer = ToolPointer::current);
 
     void saveSettings();
     void loadSettings();
 
-    void copyCurrentConfig();
-    void restoreLastConfig();
+    void pointCurrentToolToButtonTool();
+    void pointCurrentToolToToolbarTool();
 
     std::array<std::unique_ptr<Tool>, TOOL_COUNT> const& getTools() const;
 
@@ -121,19 +121,25 @@ public:
      * pointer moves to another
      * @return
      */
-    bool isSinglePageTool();
+    bool isSinglePageTool(ToolPointer toolpointer = ToolPointer::current);
+
+    bool triggeredByButton = false;
 
 protected:
     void initTools();
 
 private:
     std::array<std::unique_ptr<Tool>, TOOL_COUNT> tools;
-    Tool* current = nullptr;
+
+    // get Pointer based on Enum used for public setters and getters
+    Tool* getToolPointer(ToolPointer toolpointer);
+    Tool* currentTool = nullptr;
 
     /**
      * Last selected tool, reference with color values etc.
      */
-    LastSelectedTool* lastSelectedTool = nullptr;
+    Tool* toolbarSelectedTool = nullptr;
+    Tool* buttonSelectedTool = nullptr;
 
     EraserType eraserType = ERASER_TYPE_DEFAULT;
 

src/control/settings/ButtonConfig.cpp

@@ -25,7 +25,7 @@ void ButtonConfig::acceptActions(ToolHandler* toolHandler) {
         return;
     }
 
-    toolHandler->selectTool(this->action, false);
+    toolHandler->selectTool(this->action, false, true);
 
     if (this->action == TOOL_PEN || this->action == TOOL_HILIGHTER || this->action == TOOL_ERASER) {
 
@@ -38,10 +38,6 @@ void ButtonConfig::acceptActions(ToolHandler* toolHandler) {
         }
     }
 
-    if (this->action == TOOL_PEN || this->action == TOOL_HILIGHTER || this->action == TOOL_TEXT) {
-        toolHandler->setColor(this->color, false);
-    }
-
     if (this->action == TOOL_ERASER && this->eraserMode != ERASER_TYPE_NONE) {
         toolHandler->setEraserType(this->eraserMode);
     }

src/gui/inputdevices/MouseInputHandler.cpp

@@ -120,10 +120,10 @@ auto MouseInputHandler::changeTool(InputEvent const& event) -> bool {
     }
 
     if (cfg && cfg->getAction() != TOOL_NONE) {
-        toolHandler->copyCurrentConfig();
+        toolHandler->pointCurrentToolToButtonTool();
         cfg->acceptActions(toolHandler);
     } else {
-        toolHandler->restoreLastConfig();
+        toolHandler->pointCurrentToolToToolbarTool();
     }
 
     return false;

src/gui/inputdevices/PenInputHandler.cpp

@@ -300,7 +300,7 @@ auto PenInputHandler::actionEnd(InputEvent const& event) -> bool {
     xournal->selection = nullptr;
     this->sequenceStartPage = nullptr;
 
-    toolHandler->restoreLastConfig();
+    toolHandler->pointCurrentToolToToolbarTool();
 
     // we need this workaround so it's possible to select something with the middle button
     if (tmpSelection) {

src/gui/inputdevices/StylusInputHandler.cpp

@@ -181,10 +181,10 @@ auto StylusInputHandler::changeTool(InputEvent const& event) -> bool {
     }
 
     if (cfg && cfg->getAction() != TOOL_NONE) {
-        toolHandler->copyCurrentConfig();
+        toolHandler->pointCurrentToolToButtonTool();
         cfg->acceptActions(toolHandler);
     } else {
-        toolHandler->restoreLastConfig();
+        toolHandler->pointCurrentToolToToolbarTool();
     }
 
     return false;

src/gui/inputdevices/TouchDrawingInputHandler.cpp

@@ -90,10 +90,10 @@ auto TouchDrawingInputHandler::changeTool(InputEvent const& event) -> bool {
     }
 
     if (cfg && cfg->getAction() != TOOL_NONE) {
-        toolHandler->copyCurrentConfig();
+        toolHandler->pointCurrentToolToButtonTool();
         cfg->acceptActions(toolHandler);
     } else {
-        toolHandler->restoreLastConfig();
+        toolHandler->pointCurrentToolToToolbarTool();
     }
 
     return false;

src/gui/inputdevices/old/InputSequence.cpp

@@ -340,7 +340,7 @@ void InputSequence::actionEnd(guint32 time) {
     EditSelection* tmpSelection = xournal->selection;
     xournal->selection = nullptr;
 
-    h->restoreLastConfig();
+    h->pointCurrentToolToToolbarTool();
 
     // we need this workaround so it's possible to select something with the middle button
     if (tmpSelection) {
@@ -424,10 +424,10 @@ auto InputSequence::changeTool() -> bool {
     }
 
     if (cfg && cfg->getAction() != TOOL_NONE) {
-        h->copyCurrentConfig();
+        h->pointCurrentToolToButtonTool();
         cfg->acceptActions(h);
     } else {
-        h->restoreLastConfig();
+        h->pointCurrentToolToToolbarTool();
     }
 
     return false;

src/gui/toolbarMenubar/ColorToolItem.cpp

@@ -2,6 +2,7 @@
 
 #include <config.h>
 
+#include "control/ToolEnums.h"
 #include "gui/toolbarMenubar/icon/ColorSelectImage.h"
 #include "model/ToolbarColorNames.h"
 
@@ -105,16 +106,21 @@ void ColorToolItem::showColorchooser() {
  * Enable / Disable the tool item
  */
 void ColorToolItem::enable(bool enabled) {
-    if (!enabled && toolHandler->getToolType() == TOOL_ERASER) {
-        if (this->icon) {
+    if (!enabled && !toolHandler->hasCapability(TOOL_CAP_COLOR, ToolPointer::current) &&
+        toolHandler->hasCapability(TOOL_CAP_COLOR, ToolPointer::toolbar)) {
+        if (this->icon && toolHandler->triggeredByButton) {
+            // allow changes if currentTool has no colour capability
+            // and mainTool has Colour capability
             icon->setState(COLOR_ICON_STATE_PEN);
+            AbstractToolItem::enable(true);
+        } else {
+            // disallow changes in color
+            icon->setState(COLOR_ICON_STATE_DISABLED);
+            AbstractToolItem::enable(false);
         }
-        AbstractToolItem::enable(true);
-        switchToPen = true;
         return;
     }
 
-    switchToPen = false;
     AbstractToolItem::enable(enabled);
     if (this->icon) {
         if (enabled) {
@@ -126,10 +132,6 @@ void ColorToolItem::enable(bool enabled) {
 }
 
 void ColorToolItem::activated(GdkEvent* event, GtkMenuItem* menuitem, GtkToolButton* toolbutton) {
-    if (switchToPen) {
-        toolHandler->selectTool(TOOL_PEN, true);
-    }
-
     if (inUpdate) {
         return;
     }

src/gui/toolbarMenubar/ColorToolItem.h

@@ -75,11 +75,6 @@ private:
      */
     ColorSelectImage* icon = nullptr;
 
-    /**
-     * Switch to pen if the color icon is pressed
-     */
-    bool switchToPen = false;
-
     GtkWindow* parent = nullptr;
     ToolHandler* toolHandler = nullptr;