From d4ed28888c4abb60bdbe91f03ed6ad83ee5d4df0 Mon Sep 17 00:00:00 2001
From: Ajinkya Dahale <AjinkyaDahale@users.noreply.github.com>
Date: Wed, 8 Jan 2025 10:44:03 +0530
Subject: [PATCH] [Sketcher] More refactor of `delConstraintOnPoint`

---
 src/Mod/Sketcher/App/SketchObject.cpp | 60 +++++++++++++--------------
 1 file changed, 28 insertions(+), 32 deletions(-)

diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp
index 976907a530..43463377b9 100644
--- a/src/Mod/Sketcher/App/SketchObject.cpp
+++ b/src/Mod/Sketcher/App/SketchObject.cpp
@@ -107,6 +107,7 @@
 
 #include <memory>
 
+#include "GeoEnum.h"
 #include "SketchObject.h"
 #include "SketchObjectPy.h"
 #include "SolverGeometryExtension.h"
@@ -2375,21 +2376,25 @@ int SketchObject::delConstraintOnPoint(int geoId, PointPos posId, bool onlyCoinc
 
     findReplacement();
 
-    auto performCoincidenceChecksOrChanges = [&](auto& constr) -> std::optional<bool> {
-        if (constr->Type != Sketcher::Coincident) {
-            if (onlyCoincident) {
-                return true;
-            }
-            return std::nullopt;
+    auto performCoincidenceChecksOrChanges = [&](auto& constr) -> bool {
+        if (replaceGeoId == GeoEnum::GeoUndef) {
+            return false;
         }
-        bool firstSucceeded = transferToReplacement(constr->First, constr->FirstPos);
-        bool secondSucceeded = transferToReplacement(constr->Second, constr->SecondPos);
-        return (firstSucceeded || secondSucceeded)
-            && (constr->First != constr->Second || constr->FirstPos != constr->SecondPos);
+        if (constr->involvesGeoIdAndPosId(replaceGeoId, replacePosId)) {
+            return false;
+        }
+        // Assuming `constr` already involves geoId and posId, all conditions are already met
+        constr->substituteIndexAndPos(geoId, posId, replaceGeoId, replacePosId);
+        return true;
     };
 
-    auto performOtherConstraintChecksOrChanges = [&](auto& constr) -> std::optional<bool> {
+    auto performAllConstraintChecksOrChanges = [&](auto& constr) -> std::optional<bool> {
+        if (constr->Type != Sketcher::Coincident && onlyCoincident) {
+            return true;
+        }
         switch (constr->Type) {
+            case Sketcher::Coincident:
+                return performCoincidenceChecksOrChanges(constr);
             case Sketcher::Distance:
             case Sketcher::DistanceX:
             case Sketcher::DistanceY: {
@@ -2412,37 +2417,28 @@ int SketchObject::delConstraintOnPoint(int geoId, PointPos posId, bool onlyCoinc
                 return false;
             }
             default:
-                return true;
+                return std::nullopt;
         }
     };
 
     // remove or redirect any constraints associated with the given point
     for (auto& constr : vals) {
         // keep the constraint if it doesn't involve the point
-        if (constr->involvesGeoIdAndPosId(geoId, posId)) {
-            auto didCoincidenceCheckWork = performCoincidenceChecksOrChanges(constr);
-            if (didCoincidenceCheckWork == true) {
-                newVals.push_back(constr);
+        if (!constr->involvesGeoIdAndPosId(geoId, posId)) {
+            // for these constraints remove the constraint even if it is not directly associated
+            // with the given point
+            if ((constr->Type == Sketcher::Distance || constr->Type == Sketcher::DistanceX
+                 || constr->Type == Sketcher::DistanceY)
+                && (constr->First == geoId && constr->FirstPos == PointPos::none
+                    && (posId == PointPos::start || posId == PointPos::end))) {
                 continue;
             }
-            else if (didCoincidenceCheckWork == false) {
-                continue;
-            }
-            // The check failed, which means it's not a coincidence constraint and
-            // `onlyCoincident` is `false`
-            if (performOtherConstraintChecksOrChanges(constr) == false) {
-                continue;
-            }
-        }
-        // for these constraints remove the constraint even if it is not directly associated with
-        // the given point
-        if ((constr->Type == Sketcher::Distance || constr->Type == Sketcher::DistanceX
-             || constr->Type == Sketcher::DistanceY)
-            && (constr->First == geoId && constr->FirstPos == PointPos::none
-                && (posId == PointPos::start || posId == PointPos::end))) {
+            newVals.push_back(constr);
             continue;
         }
-        newVals.push_back(constr);
+        if (performAllConstraintChecksOrChanges(constr) != false) {
+            newVals.push_back(constr);
+        }
     }
 
     if (newVals.size() < vals.size()) {