From 4989af13e6c6544eca5c46736fbdd54fc47b4831 Mon Sep 17 00:00:00 2001
From: WandererFan <WandererFan@gmail.com>
Date: Mon, 9 Jun 2025 11:48:00 -0400
Subject: [PATCH] [TD]Cosmetic vertex tag save/restore fail  (fix #21069)
 (#21374)

* [TD]fix tag handling on Save/Restore

* [TD]fix linter messages

* [TD]remove c-style cast x4

* [TD]do not write VertexTag element in Vertex::Save
---
 src/Mod/TechDraw/App/CosmeticVertex.cpp |  60 +++++++------
 src/Mod/TechDraw/App/CosmeticVertex.h   |   4 +-
 src/Mod/TechDraw/App/Geometry.cpp       | 115 ++++++++++++------------
 src/Mod/TechDraw/App/Geometry.h         |  28 +++---
 src/Mod/TechDraw/App/Tag.cpp            |  18 +++-
 src/Mod/TechDraw/App/Tag.h              |  12 ++-
 6 files changed, 140 insertions(+), 97 deletions(-)

diff --git a/src/Mod/TechDraw/App/CosmeticVertex.cpp b/src/Mod/TechDraw/App/CosmeticVertex.cpp
index f577337b41..a9a0d4412b 100644
--- a/src/Mod/TechDraw/App/CosmeticVertex.cpp
+++ b/src/Mod/TechDraw/App/CosmeticVertex.cpp
@@ -47,33 +47,36 @@ CosmeticVertex::CosmeticVertex() : TechDraw::Vertex()
     color = Preferences::vertexColor();
     size  = Preferences::vertexScale() *
             LineGroup::getDefaultWidth("Thin");
+
     hlrVisible = true;
     cosmetic = true;
+    cosmeticTag = getTagAsString();
 }
 
-CosmeticVertex::CosmeticVertex(const TechDraw::CosmeticVertex* cv) : TechDraw::Vertex(cv)
+CosmeticVertex::CosmeticVertex(const TechDraw::CosmeticVertex* cv) : TechDraw::Vertex(cv),
+    permaPoint(cv->permaPoint),
+    linkGeom(cv->linkGeom),
+    color(cv->color),
+    size(cv->size),
+    style(cv->style),
+    visible(cv->visible)
 {
-    permaPoint = cv->permaPoint;
-    linkGeom = cv->linkGeom;
-    color = cv->color;
-    size  = cv->size;
-    style = cv->style;
-    visible = cv->visible;
+    // Base fields
     hlrVisible = true;
     cosmetic = true;
+    cosmeticTag = getTagAsString();
 }
 
-CosmeticVertex::CosmeticVertex(const Base::Vector3d& loc) : TechDraw::Vertex(loc)
+CosmeticVertex::CosmeticVertex(const Base::Vector3d& loc) : TechDraw::Vertex(loc),
+    permaPoint(loc),
+    color(Preferences::vertexColor())
 {
-    permaPoint = loc;
-    linkGeom = -1;
-    color = Preferences::vertexColor();
     size  = Preferences::vertexScale() *
             LineGroup::getDefaultWidth("Thick");
-    style = 1;        //TODO: implement styled vertexes
-    visible = true;
+
     hlrVisible = true;
     cosmetic = true;
+    cosmeticTag = getTagAsString();
 }
 
 void CosmeticVertex::move(const Base::Vector3d& newPos)
@@ -119,14 +122,16 @@ void CosmeticVertex::Save(Base::Writer &writer) const
                 << "X=\"" <<  permaPoint.x <<
                 "\" Y=\"" <<  permaPoint.y <<
                 "\" Z=\"" <<  permaPoint.z <<
-                 "\"/>" << endl;
-    writer.Stream() << writer.ind() << "<LinkGeom value=\"" <<  linkGeom << "\"/>" << endl;
-    writer.Stream() << writer.ind() << "<Color value=\"" <<  color.asHexString() << "\"/>" << endl;
-    writer.Stream() << writer.ind() << "<Size value=\"" <<  size << "\"/>" << endl;
-    writer.Stream() << writer.ind() << "<Style value=\"" <<  style << "\"/>" << endl;
+                 "\"/>" << '\n';
+    writer.Stream() << writer.ind() << "<LinkGeom value=\"" <<  linkGeom << "\"/>" << '\n';
+    writer.Stream() << writer.ind() << "<Color value=\"" <<  color.asHexString() << "\"/>" << '\n';
+    writer.Stream() << writer.ind() << "<Size value=\"" <<  size << "\"/>" << '\n';
+    writer.Stream() << writer.ind() << "<Style value=\"" <<  style << "\"/>" << '\n';
     const char v = visible?'1':'0';
-    writer.Stream() << writer.ind() << "<Visible value=\"" <<  v << "\"/>" << endl;
-    Tag::Save(writer);
+    writer.Stream() << writer.ind() << "<Visible value=\"" <<  v << "\"/>" << '\n';
+
+    //NOLINTNEXTLINE
+    Tag::Save(writer);      // as "Tag"
 }
 
 void CosmeticVertex::Restore(Base::XMLReader &reader)
@@ -135,31 +140,36 @@ void CosmeticVertex::Restore(Base::XMLReader &reader)
         return;
     }
     TechDraw::Vertex::Restore(reader);
-    reader.readElement("PermaPoint");
+
+    // Vertex::Restore call to readNextElement may leave us already positioned on the PermaPoint element.
+    if(strcmp(reader.localName(),"PermaPoint") != 0) {
+        reader.readElement("PermaPoint");
+    }
     permaPoint.x = reader.getAttribute<double>("X");
     permaPoint.y = reader.getAttribute<double>("Y");
     permaPoint.z = reader.getAttribute<double>("Z");
     reader.readElement("LinkGeom");
-    linkGeom = reader.getAttribute<long>("value");
+    linkGeom = reader.getAttribute<int>("value");
     reader.readElement("Color");
     std::string temp = reader.getAttribute<const char*>("value");
     color.fromHexString(temp);
     reader.readElement("Size");
     size = reader.getAttribute<double>("value");
     reader.readElement("Style");
-    style = reader.getAttribute<long>("value");
+    style = reader.getAttribute<int>("value");
     reader.readElement("Visible");
     visible = reader.getAttribute<bool>("value");
+
     Tag::Restore(reader);
 }
 
-Base::Vector3d CosmeticVertex::scaled(const double factor)
+Base::Vector3d CosmeticVertex::scaled(const double factor) const
 {
     return permaPoint * factor;
 }
 
 //! returns a transformed version of our coordinates (permaPoint)
-Base::Vector3d CosmeticVertex::rotatedAndScaled(const double scale, const double rotDegrees)
+Base::Vector3d CosmeticVertex::rotatedAndScaled(const double scale, const double rotDegrees) const
 {
     Base::Vector3d scaledPoint = scaled(scale);
     if (rotDegrees != 0.0) {
diff --git a/src/Mod/TechDraw/App/CosmeticVertex.h b/src/Mod/TechDraw/App/CosmeticVertex.h
index 4aec13a592..94899a3421 100644
--- a/src/Mod/TechDraw/App/CosmeticVertex.h
+++ b/src/Mod/TechDraw/App/CosmeticVertex.h
@@ -54,8 +54,8 @@ public:
 
     std::string toString() const;
     void dump(const char* title) override;
-    Base::Vector3d scaled(const double factor);
-    Base::Vector3d rotatedAndScaled(const double scale, const double rotDegrees);
+    Base::Vector3d scaled(const double factor) const;
+    Base::Vector3d rotatedAndScaled(const double scale, const double rotDegrees) const;
 
     static Base::Vector3d makeCanonicalPoint(DrawViewPart* dvp, Base::Vector3d point, bool unscale = true);
     static Base::Vector3d makeCanonicalPointInverted(DrawViewPart* dvp, Base::Vector3d invertedPoint, bool unscale = true);
diff --git a/src/Mod/TechDraw/App/Geometry.cpp b/src/Mod/TechDraw/App/Geometry.cpp
index 2ce1c6cbcf..66ec46da13 100644
--- a/src/Mod/TechDraw/App/Geometry.cpp
+++ b/src/Mod/TechDraw/App/Geometry.cpp
@@ -1252,64 +1252,61 @@ BezierSegment::BezierSegment(const TopoDS_Edge &e)
 }
 
 //**** Vertex
-Vertex::Vertex()
+Vertex::Vertex() :
+    extractType(ExtractionType::Plain),    // obsolete?
+    hlrVisible(false),
+    ref3D(-1),                              // obsolete
+    m_center(false),
+    cosmetic(false),
+    cosmeticLink(-1),
+    m_reference(false)
+
 {
     pnt = Base::Vector3d(0.0, 0.0, 0.0);
-    extractType = ExtractionType::Plain;       //obs?
-    hlrVisible = false;
-    ref3D = -1;                        //obs. never used.
-    m_center = false;
     BRepBuilderAPI_MakeVertex mkVert(gp_Pnt(0.0, 0.0, 0.0));
     occVertex = mkVert.Vertex();
-    cosmetic = false;
-    cosmeticLink = -1;
     cosmeticTag = std::string();
-    m_reference = false;
 }
 
-Vertex::Vertex(const Vertex* v)
+Vertex::Vertex(const Vertex* v) :
+    extractType(v->extractType),    // obsolete?
+    hlrVisible(v->hlrVisible),
+    ref3D(v->ref3D),                              // obsolete
+    m_center(v->m_center),
+    occVertex(v->occVertex),
+    cosmetic(v->cosmetic),
+    cosmeticLink(v->cosmeticLink),
+    cosmeticTag(v->cosmeticTag),
+    m_reference(v->m_reference)
 {
     pnt = v->point();
-    extractType = v->extractType;       //obs?
-    hlrVisible = v->hlrVisible;
-    ref3D = v->ref3D;                  //obs. never used.
-    m_center = v->m_center;
-    occVertex = v->occVertex;
-    cosmetic = v->cosmetic;
-    cosmeticLink = v->cosmeticLink;
-    cosmeticTag = v->cosmeticTag;
-    m_reference = false;
 }
 
-Vertex::Vertex(double x, double y)
+Vertex::Vertex(double x, double y) :
+    extractType(ExtractionType::Plain),    // obsolete?
+    hlrVisible(false),
+    ref3D(-1),                              // obsolete
+    m_center(false),
+    cosmetic(false),
+    cosmeticLink(-1),
+    m_reference(false)
 {
     pnt = Base::Vector3d(x, y, 0.0);
-    extractType = ExtractionType::Plain;       //obs?
-    hlrVisible = false;
-    ref3D = -1;                        //obs. never used.
-    m_center = false;
     BRepBuilderAPI_MakeVertex mkVert(gp_Pnt(x, y, 0.0));
     occVertex = mkVert.Vertex();
-    cosmetic = false;
-    cosmeticLink = -1;
     cosmeticTag = std::string();
-    m_reference = false;
 }
 
 Vertex::Vertex(Base::Vector3d v) : Vertex(v.x, v.y)
 {
-//    Base::Console().message("V::V(%s)\n",
-//                            DrawUtil::formatVector(v).c_str());
+
 }
 
 
 bool Vertex::isEqual(const Vertex& v, double tol)
 {
     double dist = (pnt - (v.pnt)).Length();
-    if (dist <= tol) {
-        return true;
-    }
-    return false;
+    return (dist <= tol);
 }
 
 void Vertex::Save(Base::Writer &writer) const
@@ -1318,24 +1315,18 @@ void Vertex::Save(Base::Writer &writer) const
                 << "X=\"" <<  pnt.x <<
                 "\" Y=\"" <<  pnt.y <<
                 "\" Z=\"" <<  pnt.z <<
-                 "\"/>" << endl;
+                 "\"/>" << '\n';
 
-    writer.Stream() << writer.ind() << "<Extract value=\"" <<  extractType << "\"/>" << endl;
+    writer.Stream() << writer.ind() << "<Extract value=\"" <<  extractType << "\"/>" << '\n';
     const char v = hlrVisible ? '1':'0';
-    writer.Stream() << writer.ind() << "<HLRVisible value=\"" <<  v << "\"/>" << endl;
-    writer.Stream() << writer.ind() << "<Ref3D value=\"" <<  ref3D << "\"/>" << endl;
+    writer.Stream() << writer.ind() << "<HLRVisible value=\"" <<  v << "\"/>" << '\n';
+    writer.Stream() << writer.ind() << "<Ref3D value=\"" <<  ref3D << "\"/>" << '\n';
     const char c = m_center ?'1':'0';
-    writer.Stream() << writer.ind() << "<IsCenter value=\"" <<  c << "\"/>" << endl;
+    writer.Stream() << writer.ind() << "<IsCenter value=\"" <<  c << "\"/>" << '\n';
     const char c2 = cosmetic?'1':'0';
-    writer.Stream() << writer.ind() << "<Cosmetic value=\"" <<  c2 << "\"/>" << endl;
-    writer.Stream() << writer.ind() << "<CosmeticLink value=\"" <<  cosmeticLink << "\"/>" << endl;
-    writer.Stream() << writer.ind() << "<CosmeticTag value=\"" <<  cosmeticTag << "\"/>" << endl;
-
-    //do we need to save this?  always recreated by program.
-//    const char r = reference?'1':'0';
-//    writer.Stream() << writer.ind() << "<Reference value=\"" <<  r << "\"/>" << endl;
-
-    Tag::Save(writer);
+    writer.Stream() << writer.ind() << "<Cosmetic value=\"" <<  c2 << "\"/>" << '\n';
+    writer.Stream() << writer.ind() << "<CosmeticLink value=\"" <<  cosmeticLink << "\"/>" << '\n';
+    writer.Stream() << writer.ind() << "<CosmeticTag value=\"" <<  cosmeticTag << "\"/>" << '\n';
 }
 
 void Vertex::Restore(Base::XMLReader &reader)
@@ -1350,26 +1341,40 @@ void Vertex::Restore(Base::XMLReader &reader)
     reader.readElement("Visible");
     hlrVisible = reader.getAttribute<bool>("value");
     reader.readElement("Ref3D");
-    ref3D = reader.getAttribute<long>("value");
+    ref3D = reader.getAttribute<int>("value");
     reader.readElement("IsCenter");
-    hlrVisible = reader.getAttribute<bool>("value");
+    m_center = reader.getAttribute<bool>("value");
     reader.readElement("Cosmetic");
     cosmetic = reader.getAttribute<bool>("value");
     reader.readElement("CosmeticLink");
-    cosmeticLink = reader.getAttribute<long>("value");
+    cosmeticLink = reader.getAttribute<int>("value");
     reader.readElement("CosmeticTag");
     cosmeticTag = reader.getAttribute<const char*>("value");
 
-    //will restore read to eof looking for "Reference" in old docs??  YES!!
-//    reader.readElement("Reference");
-//    m_reference = reader.getAttribute<bool>("value");
-
-    Tag::Restore(reader, "VertexTag");
+    // restore tag from VertexTag if it exists
+    restoreVertexTag(reader);
 
     BRepBuilderAPI_MakeVertex mkVert(gp_Pnt(pnt.x, pnt.y, pnt.z));
     occVertex = mkVert.Vertex();
 }
 
+//! look at the next element in the file.  If it is a VertexTag, set the tag.
+//! readNextElement will stop searching when it encounters an end element (ex: </CosmeticVertex>) or
+//! end of document.
+void Vertex::restoreVertexTag(Base::XMLReader& reader)
+{
+    if (!reader.readNextElement()) {
+        return;
+    }
+
+    if(strcmp(reader.localName(),"VertexTag") == 0) {
+        std::string temp = reader.getAttribute<const char*>("value");
+        setTag(Tag::fromString(temp));
+    }
+    // else we can not set the tag here.  if this is a CosmeticVertex, the tag will be set later.
+    // the tag is not used for geometry vertices.
+}
+
 void Vertex::dump(const char* title)
 {
     Base::Console().message("TD::Vertex - %s - point: %s vis: %d cosmetic: %d  cosLink: %d cosTag: %s\n",
@@ -1382,7 +1387,7 @@ TopoShape Vertex::asTopoShape(double scale)
     Base::Vector3d point = Base::convertTo<Base::Vector3d>(BRep_Tool::Pnt(getOCCVertex()));
     point = point / scale;
     BRepBuilderAPI_MakeVertex mkVert(Base::convertTo<gp_Pnt>(point));
-    return TopoShape(mkVert.Vertex());
+    return {mkVert.Vertex()};
 }
 
 
diff --git a/src/Mod/TechDraw/App/Geometry.h b/src/Mod/TechDraw/App/Geometry.h
index 81a316a1e4..9833d8ed65 100644
--- a/src/Mod/TechDraw/App/Geometry.h
+++ b/src/Mod/TechDraw/App/Geometry.h
@@ -143,22 +143,22 @@ class TechDrawExport BaseGeom : public std::enable_shared_from_this<BaseGeom>, p
         void setGeomType(GeomType type) { geomType = type; }
         EdgeClass getClassOfEdge() { return classOfEdge; }
         void setClassOfEdge(EdgeClass newClass) { classOfEdge = newClass; }
-        bool getHlrVisible() { return hlrVisible; }
+        bool getHlrVisible() const { return hlrVisible; }
         void setHlrVisible(bool state) { hlrVisible = state; }
-        bool getReversed()  { return reversed; }
+        bool getReversed() const  { return reversed; }
         void setReversed(bool state) { reversed = state; }
-        int getRef3d()  { return ref3D; }
+        int getRef3d() const { return ref3D; }
         void setRef3d(int ref)  { ref3D = ref; }
         TopoDS_Edge getOCCEdge()  { return occEdge; }
-        void setOCCEdge(TopoDS_Edge newEdge)  { occEdge = newEdge; }
-        bool getCosmetic()  { return cosmetic; }
+        void setOCCEdge(const TopoDS_Edge& newEdge)  { occEdge = newEdge; }
+        bool getCosmetic() const  { return cosmetic; }
         void setCosmetic (bool state)  { cosmetic = state; }
         SourceType source() { return m_source; }
         void source(SourceType s) { m_source = s; }
-        int sourceIndex() { return m_sourceIndex; }
+        int sourceIndex() const { return m_sourceIndex; }
         void sourceIndex(int si) { m_sourceIndex = si; }
-        std::string getCosmeticTag() { return cosmeticTag; }
-        void setCosmeticTag(std::string t) { cosmeticTag = t; }
+        std::string getCosmeticTag() const { return cosmeticTag; }
+        void setCosmeticTag(const std::string& t) { cosmeticTag = t; }
         Part::TopoShape asTopoShape(double scale = 1.0);
 
         virtual double getStartAngle() { return 0.0; }
@@ -167,6 +167,8 @@ class TechDrawExport BaseGeom : public std::enable_shared_from_this<BaseGeom>, p
         virtual void clockwiseAngle(bool direction) { (void) direction; }
 
 protected:
+// TODO: change direct access to these data members by setters/getters
+//NOLINTBEGIN (misc-non-private-member-variables-in-classes)
         GeomType geomType;
         ExtractionType extractType;     //obs
         EdgeClass classOfEdge;
@@ -179,7 +181,7 @@ protected:
         SourceType m_source;
         int m_sourceIndex;
         std::string cosmeticTag;
-
+//NOLINTEND (misc-non-private-member-variables-in-classes)
 };
 using BaseGeomPtrVector = std::vector<BaseGeomPtr>;    //new style
 
@@ -364,6 +366,7 @@ class TechDrawExport Face
 };
 using FacePtr = std::shared_ptr<Face>;
 
+//NOLINTNEXTLINE  (no copy, move, etc)
 class TechDrawExport Vertex : public TechDraw::Tag
 {
     public:
@@ -371,7 +374,7 @@ class TechDrawExport Vertex : public TechDraw::Tag
         explicit Vertex(const Vertex* v);
         Vertex(double x, double y);
         explicit Vertex(Base::Vector3d v);
-        virtual ~Vertex()  = default;
+        ~Vertex() override = default;
 
         virtual void Save(Base::Writer &/*writer*/) const;
         virtual void Restore(Base::XMLReader &/*reader*/);
@@ -402,7 +405,11 @@ class TechDrawExport Vertex : public TechDraw::Tag
 
         Part::TopoShape asTopoShape(double scale = 1.0);
 
+        void restoreVertexTag(Base::XMLReader& reader);
+
     protected:
+// TODO: change direct access to these data members by setters/getters
+//NOLINTBEGIN (misc-non-private-member-variables-in-classes)
         Base::Vector3d pnt;
         ExtractionType extractType;       //obs?
         bool hlrVisible;                 //visible according to HLR
@@ -413,6 +420,7 @@ class TechDrawExport Vertex : public TechDraw::Tag
         int cosmeticLink;                 //deprec. use cosmeticTag
         std::string cosmeticTag;
         bool m_reference;                   //reference vertex (ex robust dimension)
+//NOLINTEND (misc-non-private-member-variables-in-classes)
 };
 using VertexPtr = std::shared_ptr<Vertex>;
 
diff --git a/src/Mod/TechDraw/App/Tag.cpp b/src/Mod/TechDraw/App/Tag.cpp
index c8c082f5de..667762f7ee 100644
--- a/src/Mod/TechDraw/App/Tag.cpp
+++ b/src/Mod/TechDraw/App/Tag.cpp
@@ -37,7 +37,9 @@
 
 using namespace TechDraw;
 
-Tag::Tag()
+// lint complains if tag is not initialized here.
+Tag::Tag() :
+    tag(boost::uuids::uuid())
 {
     createNewTag();
 }
@@ -52,6 +54,14 @@ std::string Tag::getTagAsString() const
     return boost::uuids::to_string(getTag());
 }
 
+boost::uuids::uuid Tag::fromString(const std::string& tagString)
+{
+    boost::uuids::string_generator gen;
+    boost::uuids::uuid u1 = gen(tagString);
+    return u1;
+
+}
+
 void Tag::setTag(const boost::uuids::uuid& newTag)
 {
     tag = newTag;
@@ -87,7 +97,7 @@ void Tag::Restore(Base::XMLReader& reader, std::string_view elementName)
     // Setting elementName is only for backwards compatibility!
     reader.readElement(elementName.data());
     std::string temp = reader.getAttribute<const char*>("value");
-    boost::uuids::string_generator gen;
-    boost::uuids::uuid u1 = gen(temp);
-    tag = u1;
+    tag = fromString(temp);
 }
+
+
diff --git a/src/Mod/TechDraw/App/Tag.h b/src/Mod/TechDraw/App/Tag.h
index 307e74bce0..132cb86737 100644
--- a/src/Mod/TechDraw/App/Tag.h
+++ b/src/Mod/TechDraw/App/Tag.h
@@ -24,6 +24,9 @@
 #ifndef TECHDRAW_TAG_H
 #define TECHDRAW_TAG_H
 
+#include <string>
+#include <string_view>
+
 #include <boost/uuid/uuid.hpp>
 
 #include <Mod/TechDraw/TechDrawGlobal.h>
@@ -34,12 +37,19 @@ class Writer;
 }
 
 namespace TechDraw {
+
+
+//NOLINTNEXTLINE    (default destructor but no copy)
 class TechDrawExport Tag {
 public:
+    virtual ~Tag() = default;
     //Uniqueness
     boost::uuids::uuid getTag() const;
     virtual std::string getTagAsString() const;
 
+    static boost::uuids::uuid fromString(const std::string& tagString);
+
+
 protected:
     Tag();
     void setTag(const boost::uuids::uuid& newTag);
@@ -49,7 +59,7 @@ protected:
 
 private:
     void createNewTag();
-    boost::uuids::uuid tag;
+    boost::uuids::uuid tag{};
 };
 }