Apply more reviewer suggestions

src/core/control/Control.cpp

@@ -649,18 +649,15 @@ void Control::actionPerformed(ActionType type, ActionGroup group, GtkToolButton*
                 selectTool(TOOL_HAND);
             }
             break;
-        case ACTION_SETSQUARE:
-            if (auto xournal = this->win->getXournal();
-                !this->geometryToolController ||
-                this->geometryToolController->getType() != GeometryToolType::SETSQUARE) {
-                resetGeometryTool();
+        case ACTION_SETSQUARE: {
+            bool needsNewSetsquare = !this->geometryToolController ||
+                                     this->geometryToolController->getType() != GeometryToolType::SETSQUARE;
+            resetGeometryTool();
+            if (needsNewSetsquare) {
                 makeGeometryTool(GeometryToolType::SETSQUARE);
-                xournal->getViewFor(getCurrentPageNo())->rerenderPage();
-            } else {
-                resetGeometryTool();
-                xournal->getViewFor(getCurrentPageNo())->rerenderPage();
             }
             break;
+        }
         case ACTION_TOOL_FLOATING_TOOLBOX:
             if (enabled) {
                 selectTool(TOOL_FLOATING_TOOLBOX);
@@ -1066,25 +1063,40 @@ void Control::actionPerformed(ActionType type, ActionGroup group, GtkToolButton*
 
 void Control::makeGeometryTool(GeometryToolType tool) {
     auto view = this->win->getXournal()->getViewFor(getCurrentPageNo());
-    if (tool == GeometryToolType::SETSQUARE) {
-        auto setsquare = new Setsquare();
-        view->addOverlayView(std::make_unique<xoj::view::SetsquareView>(setsquare, view, zoom));
-        this->geometryTool = std::unique_ptr<GeometryTool>(setsquare);
-        this->geometryToolController = std::make_unique<SetsquareController>(view, setsquare);
-        std::unique_ptr<GeometryToolInputHandler> geometryToolInputHandler =
-                std::make_unique<SetsquareInputHandler>(this->win->getXournal(), geometryToolController.get());
-        geometryToolInputHandler->registerToPool(setsquare->getHandlerPool());
-        auto xournal = GTK_XOURNAL(this->win->getXournal()->getWidget());
-        xournal->input->setGeometryToolInputHandler(std::move(geometryToolInputHandler));
-        fireActionSelected(GROUP_GEOMETRY_TOOL, ACTION_SETSQUARE);
+    auto* xournal = GTK_XOURNAL(this->win->getXournal()->getWidget());
+    switch (tool) {
+        case SETSQUARE: {
+            auto setsquare = new Setsquare();
+            view->addOverlayView(std::make_unique<xoj::view::SetsquareView>(setsquare, view, zoom));
+            this->geometryTool = std::unique_ptr<GeometryTool>(setsquare);
+            this->geometryToolController = std::make_unique<SetsquareController>(view, setsquare);
+            std::unique_ptr<GeometryToolInputHandler> geometryToolInputHandler =
+                    std::make_unique<SetsquareInputHandler>(this->win->getXournal(), geometryToolController.get());
+            geometryToolInputHandler->registerToPool(setsquare->getHandlerPool());
+            xournal->input->setGeometryToolInputHandler(std::move(geometryToolInputHandler));
+            fireActionSelected(GROUP_GEOMETRY_TOOL, ACTION_SETSQUARE);
+            geometryTool->notify();
+            break;
+        }
+        default:
+            g_warning("Unknown geometry tool type %d", tool);
     }
 }
 
 void Control::resetGeometryTool() {
+    Range rg;
+    bool hasGeometryTool = false;
+    if (this->geometryTool) {
+        hasGeometryTool = true;
+        rg = this->geometryTool->getToolRange(true);
+    }
     this->geometryToolController.reset();
     this->geometryTool.reset();
-    auto xournal = GTK_XOURNAL(this->win->getXournal()->getWidget());
+    auto* xournal = GTK_XOURNAL(this->win->getXournal()->getWidget());
     xournal->input->resetGeometryToolInputHandler();
+    if (win && hasGeometryTool) {
+        (win->getXournal()->getViewFor(getCurrentPageNo()))->rerenderRange(rg);
+    }
     fireActionSelected(GROUP_GEOMETRY_TOOL, ACTION_NONE);
 }
 
@@ -1388,11 +1400,14 @@ void Control::deletePage() {
 
     // if the current page contains the geometry tool, reset it
     size_t pNr = getCurrentPageNo();
-    doc->lock();
-    if (geometryToolController && doc->indexOf(geometryToolController->getPage()) == pNr) {
-        resetGeometryTool();
+    if (geometryToolController) {
+        doc->lock();
+        auto page = doc->indexOf(geometryToolController->getPage());
+        doc->unlock();
+        if (page == pNr) {
+            resetGeometryTool();
+        }
     }
-    doc->unlock();
     // don't allow delete pages if we have less than 2 pages,
     // so we can be (more or less) sure there is at least one page.
     if (this->doc->getPageCount() < 2) {
@@ -2271,8 +2286,8 @@ auto Control::openFile(fs::path filepath, int scrollToPage, bool forceOpen) -> b
         parentFolderPath = missingFilePath.parent_path().string();
         filename = missingFilePath.filename().string();
 #else
-        // since POSIX systems detect the whole Windows path as a filename, this checks whether missingFilePath contains
-        // a Windows path
+        // since POSIX systems detect the whole Windows path as a filename, this checks whether missingFilePath
+        // contains a Windows path
         std::regex regex(R"([A-Z]:\\(?:.*\\)*(.*))");
         std::cmatch matchInfo;
 

src/core/control/GeometryToolController.cpp

@@ -19,7 +19,7 @@ GeometryToolController::GeometryToolController(XojPageView* view, GeometryTool*
 
 GeometryToolController::~GeometryToolController() = default;
 
-void GeometryToolController::move(double x, double y) {
+void GeometryToolController::translate(double x, double y) {
     geometryTool->setTranslationX(geometryTool->getTranslationX() + x);
     geometryTool->setTranslationY(geometryTool->getTranslationY() + y);
     geometryTool->notify();
@@ -59,15 +59,17 @@ void GeometryToolController::markPoint(double x, double y) {
     cross->addPoint(Point(x + MARK_SIZE, y - MARK_SIZE));
     cross->addPoint(Point(x - MARK_SIZE, y + MARK_SIZE));
 
+    const auto doc = control->getDocument();
     const auto page = view->getPage();
+    doc->lock();
     control->getLayerController()->ensureLayerExists(page);
     const auto layer = page->getSelectedLayer();
+    layer->addElement(cross);
+    doc->unlock();
 
     const auto undo = control->getUndoRedoHandler();
     undo->addUndoAction(std::make_unique<InsertUndoAction>(page, layer, cross));
 
-    layer->addElement(cross);
-
     const Rectangle<double> rect{cross->getX(), cross->getY(), cross->getElementWidth(), cross->getElementHeight()};
     view->rerenderRect(rect.x, rect.y, rect.width, rect.height);
 }
@@ -116,6 +118,6 @@ void GeometryToolController::initializeStroke() {
     }
 }
 
-auto GeometryToolController::getPage() const -> PageRef { return view->getPage(); }
+auto GeometryToolController::getPage() const -> const PageRef { return view->getPage(); }
 
 auto GeometryToolController::getView() const -> XojPageView* { return view; }

src/core/control/GeometryToolController.h

@@ -34,11 +34,11 @@ class GeometryToolController {
 
 public:
     /**
-     * @brief moves the geometry tool in x- and y-direction
+     * @brief translates the geometry tool in x- and y-direction
      * @param x the translation in x-direction (in document coordinates)
      * @param y the translation in y-direction (in document coordinates)
      */
-    void move(double x, double y);
+    void translate(double x, double y);
 
     /**
      * @brief rotates the geometry tool around its rotation center
@@ -57,7 +57,7 @@ class GeometryToolController {
     void scale(double f, double cx, double cy);
 
     /**
-     * @brief marks a point with a "x"
+     * @brief marks a point with a "x" and puts the mark onto the current layer
      * @param x the x-coordinate of the point (in document coordinates)
      * @param y the y-coordinate of the point (in document coordinates)
      */
@@ -81,7 +81,7 @@ class GeometryToolController {
     /**
      * @brief the page with respect to which the setsquare is initialized
      */
-    PageRef getPage() const;
+    const PageRef getPage() const;
 
     virtual bool isInsideGeometryTool(double x, double y, double border = 0.0) const = 0;
 

src/core/control/SetsquareController.cpp

@@ -7,7 +7,6 @@
 #include "model/Setsquare.h"
 #include "model/Stroke.h"
 #include "model/XojPage.h"
-#include "undo/InsertUndoAction.h"  // for InsertUndoAction
 
 using xoj::util::Rectangle;
 SetsquareController::SetsquareController(XojPageView* view, Setsquare* setsquare):
@@ -89,7 +88,7 @@ void SetsquareController::updateRadialStroke(double x, double y) {
     const auto p = posRelToSide(HYPOTENUSE, x, y);
     const auto rad = std::hypot(p.x, p.y);
 
-    if (rad >= geometryTool->getHeight() / std::sqrt(2.) - 1.15 || p.y > 0) {  // TODO
+    if (rad >= Setsquare::radiusFromHeight(geometryTool->getHeight()) || p.y > 0) {
         this->strokeAngle = std::atan2(p.y, p.x);
         stroke->addPoint(Point(x, y));
     } else {
@@ -104,7 +103,7 @@ void SetsquareController::updateRadialStroke(double x, double y) {
 }
 
 void SetsquareController::finalizeEdgeStroke() {
-    hypotenuseMax = std::numeric_limits<double>::min();
+    hypotenuseMax = std::numeric_limits<double>::lowest();
     hypotenuseMin = std::numeric_limits<double>::max();
     addStrokeToLayer();
 }
@@ -114,6 +113,6 @@ void SetsquareController::finalizeRadialStroke() {
     addStrokeToLayer();
 }
 
-auto SetsquareController::existsEdgeStroke() -> bool { return !(hypotenuseMax == std::numeric_limits<double>::min()); }
+auto SetsquareController::existsEdgeStroke() -> bool { return hypotenuseMax != std::numeric_limits<double>::lowest(); }
 
 auto SetsquareController::existsRadialStroke() -> bool { return !std::isnan(strokeAngle); }

src/core/control/SetsquareController.h

@@ -120,7 +120,7 @@ class SetsquareController: public GeometryToolController {
      * x-coordinates of the point to be drawn (with respect to an unrotated, and untranslated coordinate system) are
      * saved in the variables hypotenuseMin and hypotenuseMax
      */
-    double hypotenuseMax = std::numeric_limits<double>::min();
+    double hypotenuseMax = std::numeric_limits<double>::lowest();
     double hypotenuseMin = std::numeric_limits<double>::max();
 
     /**