From 72cfd14768388a3828205ce89d0b67b3cc94ba7c Mon Sep 17 00:00:00 2001 From: wmayer Date: Mon, 30 Dec 2024 17:44:04 +0100 Subject: [PATCH 1/4] Gui: const correctness in ViewProviderPlane --- src/Gui/ViewProviderPlane.cpp | 8 ++++---- src/Gui/ViewProviderPlane.h | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Gui/ViewProviderPlane.cpp b/src/Gui/ViewProviderPlane.cpp index b11c95c68b..42864dbc00 100644 --- a/src/Gui/ViewProviderPlane.cpp +++ b/src/Gui/ViewProviderPlane.cpp @@ -152,7 +152,7 @@ void ViewProviderPlane::setLabelVisibility(bool val) labelSwitch->whichChild = val ? SO_SWITCH_ALL : SO_SWITCH_NONE; } -unsigned long ViewProviderPlane::getColor(std::string& role) +unsigned long ViewProviderPlane::getColor(const std::string& role) const { auto planesRoles = App::LocalCoordinateSystem::PlaneRoles; if (role == planesRoles[0]) { @@ -167,7 +167,7 @@ unsigned long ViewProviderPlane::getColor(std::string& role) return 0; } -std::string ViewProviderPlane::getLabelText(std::string& role) +std::string ViewProviderPlane::getLabelText(const std::string& role) const { std::string text; auto planesRoles = App::LocalCoordinateSystem::PlaneRoles; @@ -183,7 +183,7 @@ std::string ViewProviderPlane::getLabelText(std::string& role) return text; } -std::string ViewProviderPlane::getRole() +std::string ViewProviderPlane::getRole() const { // Note: Role property of App::Plane is not set yet when attaching. const char* name = pcObject->getNameInDocument(); @@ -199,4 +199,4 @@ std::string ViewProviderPlane::getRole() } return ""; -} \ No newline at end of file +} diff --git a/src/Gui/ViewProviderPlane.h b/src/Gui/ViewProviderPlane.h index eeb142a9ce..7246a9fe91 100644 --- a/src/Gui/ViewProviderPlane.h +++ b/src/Gui/ViewProviderPlane.h @@ -43,9 +43,9 @@ public: void attach (App::DocumentObject*) override; - unsigned long getColor(std::string& role); - std::string getRole(); - std::string getLabelText(std::string& role); + unsigned long getColor(const std::string& role) const; + std::string getRole() const; + std::string getLabelText(const std::string& role) const; void setLabelVisibility(bool val); private: From 8d4ddac85e081c59062b9784a02abc6a64a3e93f Mon Sep 17 00:00:00 2001 From: wmayer Date: Mon, 30 Dec 2024 18:42:50 +0100 Subject: [PATCH 2/4] Gui: Reduce code duplication in ViewProviderCoordinateSystem --- src/Gui/ViewProviderCoordinateSystem.cpp | 25 ++++++++++++------------ src/Gui/ViewProviderCoordinateSystem.h | 7 +++++++ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/Gui/ViewProviderCoordinateSystem.cpp b/src/Gui/ViewProviderCoordinateSystem.cpp index 2d57a94c8e..1d3fb7eaf1 100644 --- a/src/Gui/ViewProviderCoordinateSystem.cpp +++ b/src/Gui/ViewProviderCoordinateSystem.cpp @@ -183,30 +183,31 @@ void ViewProviderCoordinateSystem::setPlaneLabelVisibility(bool val) } -void ViewProviderCoordinateSystem::setTemporaryScale(double factor) +void ViewProviderCoordinateSystem::applyDatumObjects(const DatumObjectFunc& func) { auto lcs = getObject(); - auto& objs = lcs->OriginFeatures.getValues(); + const auto& objs = lcs->OriginFeatures.getValues(); for (auto* obj : objs) { auto* vp = dynamic_cast( Gui::Application::Instance->getViewProvider(obj)); if (vp) { - vp->setTemporaryScale(factor); + func(vp); } } } +void ViewProviderCoordinateSystem::setTemporaryScale(double factor) +{ + applyDatumObjects([factor](ViewProviderDatum* vp) { + vp->setTemporaryScale(factor); + }); +} + void ViewProviderCoordinateSystem::resetTemporarySize() { - auto lcs = getObject(); - auto& objs = lcs->OriginFeatures.getValues(); - for (auto* obj : objs) { - auto* vp = dynamic_cast( - Gui::Application::Instance->getViewProvider(obj)); - if (vp) { - vp->resetTemporarySize(); - } - } + applyDatumObjects([](ViewProviderDatum* vp) { + vp->resetTemporarySize(); + }); } void ViewProviderCoordinateSystem::updateData(const App::Property* prop) { diff --git a/src/Gui/ViewProviderCoordinateSystem.h b/src/Gui/ViewProviderCoordinateSystem.h index 1f812a5a66..d8177b0869 100644 --- a/src/Gui/ViewProviderCoordinateSystem.h +++ b/src/Gui/ViewProviderCoordinateSystem.h @@ -24,6 +24,7 @@ #ifndef GUI_VIEWPROVIDER_ViewProviderOrigin_H #define GUI_VIEWPROVIDER_ViewProviderOrigin_H +#include #include #include "ViewProviderGeoFeatureGroup.h" @@ -32,6 +33,7 @@ namespace Gui { class Document; +class ViewProviderDatum; class GuiExport ViewProviderCoordinateSystem : public ViewProviderGeoFeatureGroup { @@ -84,10 +86,15 @@ public: // default color for origini: light-blue (50, 150, 250, 255 stored as 0xRRGGBBAA) static const uint32_t defaultColor = 0x3296faff; + protected: void updateData(const App::Property*) override; bool onDelete(const std::vector &) override; +private: + using DatumObjectFunc = std::function; + void applyDatumObjects(const DatumObjectFunc& func); + private: SoGroup *pcGroupChildren; From 63088a29a9d4d8a1a3bf1b6b765b21866dca72c0 Mon Sep 17 00:00:00 2001 From: wmayer Date: Mon, 30 Dec 2024 19:03:58 +0100 Subject: [PATCH 3/4] Gui: Reduce cognitive complexity of ViewProviderCoordinateSystem::setTemporaryVisibility --- src/Gui/ViewProviderCoordinateSystem.cpp | 40 +++++++++--------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/src/Gui/ViewProviderCoordinateSystem.cpp b/src/Gui/ViewProviderCoordinateSystem.cpp index 1d3fb7eaf1..8333b1e76e 100644 --- a/src/Gui/ViewProviderCoordinateSystem.cpp +++ b/src/Gui/ViewProviderCoordinateSystem.cpp @@ -97,51 +97,41 @@ void ViewProviderCoordinateSystem::setDisplayMode(const char* ModeName) ViewProviderDocumentObject::setDisplayMode(ModeName); } -void ViewProviderCoordinateSystem::setTemporaryVisibility(bool axis, bool plane, bool points) { - auto origin = static_cast( getObject() ); +void ViewProviderCoordinateSystem::setTemporaryVisibility(bool axis, bool plane, bool points) +{ + auto origin = getObject(); bool saveState = tempVisMap.empty(); try { // Remember & Set axis visibility for(App::DocumentObject* obj : origin->axes()) { - if (obj) { - Gui::ViewProvider* vp = Gui::Application::Instance->getViewProvider(obj); - if(vp) { - if (saveState) { - tempVisMap[vp] = vp->isVisible(); - } - vp->setVisible(axis); + if (auto vp = Gui::Application::Instance->getViewProvider(obj)) { + if (saveState) { + tempVisMap[vp] = vp->isVisible(); } + vp->setVisible(axis); } } // Remember & Set plane visibility for(App::DocumentObject* obj : origin->planes()) { - if (obj) { - Gui::ViewProvider* vp = Gui::Application::Instance->getViewProvider(obj); - if(vp) { - if (saveState) { - tempVisMap[vp] = vp->isVisible(); - } - vp->setVisible(plane); + if (auto vp = Gui::Application::Instance->getViewProvider(obj)) { + if (saveState) { + tempVisMap[vp] = vp->isVisible(); } + vp->setVisible(plane); } } // Remember & Set origin point visibility App::DocumentObject* obj = origin->getOrigin(); - if (obj) { - Gui::ViewProvider* vp = Gui::Application::Instance->getViewProvider(obj); - if (vp) { - if (saveState) { - tempVisMap[vp] = vp->isVisible(); - } - vp->setVisible(points); + if (auto vp = Gui::Application::Instance->getViewProvider(obj)) { + if (saveState) { + tempVisMap[vp] = vp->isVisible(); } + vp->setVisible(points); } - - } catch (const Base::Exception &ex) { Base::Console().Error ("%s\n", ex.what() ); From cbd116b4517c8cc0faed92b998dd3f29f1f4e8a2 Mon Sep 17 00:00:00 2001 From: wmayer Date: Tue, 31 Dec 2024 13:29:01 +0100 Subject: [PATCH 4/4] Gui: Use bitmask instead of three booleans in setTemporaryVisibility to improve readability --- src/Gui/ViewProviderCoordinateSystem.cpp | 8 ++++---- src/Gui/ViewProviderCoordinateSystem.h | 16 +++++++++++++++- src/Mod/PartDesign/Gui/TaskFeaturePick.cpp | 15 ++++----------- src/Mod/PartDesign/Gui/TaskHelixParameters.cpp | 2 +- .../Gui/TaskLinearPatternParameters.cpp | 2 +- .../PartDesign/Gui/TaskMirroredParameters.cpp | 2 +- .../Gui/TaskPolarPatternParameters.cpp | 2 +- .../PartDesign/Gui/TaskPrimitiveParameters.cpp | 2 +- .../PartDesign/Gui/TaskRevolutionParameters.cpp | 2 +- 9 files changed, 29 insertions(+), 22 deletions(-) diff --git a/src/Gui/ViewProviderCoordinateSystem.cpp b/src/Gui/ViewProviderCoordinateSystem.cpp index 8333b1e76e..32d1465ec1 100644 --- a/src/Gui/ViewProviderCoordinateSystem.cpp +++ b/src/Gui/ViewProviderCoordinateSystem.cpp @@ -97,7 +97,7 @@ void ViewProviderCoordinateSystem::setDisplayMode(const char* ModeName) ViewProviderDocumentObject::setDisplayMode(ModeName); } -void ViewProviderCoordinateSystem::setTemporaryVisibility(bool axis, bool plane, bool points) +void ViewProviderCoordinateSystem::setTemporaryVisibility(DatumElements elements) { auto origin = getObject(); @@ -110,7 +110,7 @@ void ViewProviderCoordinateSystem::setTemporaryVisibility(bool axis, bool plane, if (saveState) { tempVisMap[vp] = vp->isVisible(); } - vp->setVisible(axis); + vp->setVisible(elements.testFlag(DatumElement::Axes)); } } @@ -120,7 +120,7 @@ void ViewProviderCoordinateSystem::setTemporaryVisibility(bool axis, bool plane, if (saveState) { tempVisMap[vp] = vp->isVisible(); } - vp->setVisible(plane); + vp->setVisible(elements.testFlag(DatumElement::Planes)); } } @@ -130,7 +130,7 @@ void ViewProviderCoordinateSystem::setTemporaryVisibility(bool axis, bool plane, if (saveState) { tempVisMap[vp] = vp->isVisible(); } - vp->setVisible(points); + vp->setVisible(elements.testFlag(DatumElement::Origin)); } } catch (const Base::Exception &ex) { diff --git a/src/Gui/ViewProviderCoordinateSystem.h b/src/Gui/ViewProviderCoordinateSystem.h index d8177b0869..f18859fdd4 100644 --- a/src/Gui/ViewProviderCoordinateSystem.h +++ b/src/Gui/ViewProviderCoordinateSystem.h @@ -25,6 +25,7 @@ #define GUI_VIEWPROVIDER_ViewProviderOrigin_H #include +#include #include #include "ViewProviderGeoFeatureGroup.h" @@ -35,6 +36,17 @@ namespace Gui { class Document; class ViewProviderDatum; +enum class DatumElement +{ + // clang-format off + Origin = 1 << 0, + Axes = 1 << 1, + Planes = 1 << 2 + // clang-format on +}; + +using DatumElements = Base::Flags; + class GuiExport ViewProviderCoordinateSystem : public ViewProviderGeoFeatureGroup { PROPERTY_HEADER_WITH_OVERRIDE(Gui::ViewProviderCoordinateSystem); @@ -62,7 +74,7 @@ public: */ ///@{ /// Set temporary visibility of some of origin's objects e.g. while rotating or mirroring - void setTemporaryVisibility (bool axis, bool planes, bool points = false); + void setTemporaryVisibility (DatumElements elements); /// Returns true if the origin in temporary visibility mode bool isTemporaryVisibility (); /// Reset the visibility @@ -103,5 +115,7 @@ private: } // namespace Gui +ENABLE_BITMASK_OPERATORS(Gui::DatumElement) + #endif // GUI_VIEWPROVIDER_ViewProviderOrigin_H diff --git a/src/Mod/PartDesign/Gui/TaskFeaturePick.cpp b/src/Mod/PartDesign/Gui/TaskFeaturePick.cpp index 2a039750c7..022898cfdd 100644 --- a/src/Mod/PartDesign/Gui/TaskFeaturePick.cpp +++ b/src/Mod/PartDesign/Gui/TaskFeaturePick.cpp @@ -110,14 +110,8 @@ TaskFeaturePick::TaskFeaturePick(std::vector& objects, ui->listWidget->setSelectionMode(QAbstractItemView::ExtendedSelection); } - enum - { - axisBit = 0, - planeBit = 1 - }; - // NOTE: generally there shouldn't be more then one origin - std::map> originVisStatus; + std::map originVisStatus; auto statusIt = status.cbegin(); auto objIt = objects.begin(); @@ -144,10 +138,10 @@ TaskFeaturePick::TaskFeaturePick(std::vector& objects, App::Origin* origin = dynamic_cast(datum->getLCS()); if (origin) { if ((*objIt)->isDerivedFrom(App::Plane::getClassTypeId())) { - originVisStatus[origin].set(planeBit, true); + originVisStatus[origin].setFlag(Gui::DatumElement::Planes, true); } else if ((*objIt)->isDerivedFrom(App::Line::getClassTypeId())) { - originVisStatus[origin].set(axisBit, true); + originVisStatus[origin].setFlag(Gui::DatumElement::Axes, true); } } } @@ -160,8 +154,7 @@ TaskFeaturePick::TaskFeaturePick(std::vector& objects, Gui::ViewProviderCoordinateSystem* vpo = static_cast( Gui::Application::Instance->getViewProvider(origin)); if (vpo) { - vpo->setTemporaryVisibility(originVisStatus[origin][axisBit], - originVisStatus[origin][planeBit]); + vpo->setTemporaryVisibility(originVisStatus[origin]); vpo->setTemporaryScale(4.0); // NOLINT vpo->setPlaneLabelVisibility(true); origins.push_back(vpo); diff --git a/src/Mod/PartDesign/Gui/TaskHelixParameters.cpp b/src/Mod/PartDesign/Gui/TaskHelixParameters.cpp index e1b4b9aa2b..9d87827b13 100644 --- a/src/Mod/PartDesign/Gui/TaskHelixParameters.cpp +++ b/src/Mod/PartDesign/Gui/TaskHelixParameters.cpp @@ -174,7 +174,7 @@ void TaskHelixParameters::showCoordinateAxes() ViewProviderCoordinateSystem* vpOrigin; vpOrigin = static_cast( Gui::Application::Instance->getViewProvider(origin)); - vpOrigin->setTemporaryVisibility(true, false); + vpOrigin->setTemporaryVisibility(Gui::DatumElement::Axes); } catch (const Base::Exception& ex) { ex.ReportException(); diff --git a/src/Mod/PartDesign/Gui/TaskLinearPatternParameters.cpp b/src/Mod/PartDesign/Gui/TaskLinearPatternParameters.cpp index de62a2b1cd..13b7fffc1c 100644 --- a/src/Mod/PartDesign/Gui/TaskLinearPatternParameters.cpp +++ b/src/Mod/PartDesign/Gui/TaskLinearPatternParameters.cpp @@ -111,7 +111,7 @@ void TaskLinearPatternParameters::setupParameterUI(QWidget* widget) App::Origin* origin = body->getOrigin(); auto vpOrigin = static_cast( Gui::Application::Instance->getViewProvider(origin)); - vpOrigin->setTemporaryVisibility(true, false); + vpOrigin->setTemporaryVisibility(Gui::DatumElement::Axes); } catch (const Base::Exception& ex) { Base::Console().Error("%s\n", ex.what()); diff --git a/src/Mod/PartDesign/Gui/TaskMirroredParameters.cpp b/src/Mod/PartDesign/Gui/TaskMirroredParameters.cpp index d8cb8a5fb5..91bf091697 100644 --- a/src/Mod/PartDesign/Gui/TaskMirroredParameters.cpp +++ b/src/Mod/PartDesign/Gui/TaskMirroredParameters.cpp @@ -92,7 +92,7 @@ void TaskMirroredParameters::setupParameterUI(QWidget* widget) App::Origin* origin = body->getOrigin(); auto vpOrigin = static_cast( Gui::Application::Instance->getViewProvider(origin)); - vpOrigin->setTemporaryVisibility(false, true); + vpOrigin->setTemporaryVisibility(Gui::DatumElement::Planes); } catch (const Base::Exception& ex) { Base::Console().Error("%s\n", ex.what()); diff --git a/src/Mod/PartDesign/Gui/TaskPolarPatternParameters.cpp b/src/Mod/PartDesign/Gui/TaskPolarPatternParameters.cpp index 8f55d94990..61eb9dc368 100644 --- a/src/Mod/PartDesign/Gui/TaskPolarPatternParameters.cpp +++ b/src/Mod/PartDesign/Gui/TaskPolarPatternParameters.cpp @@ -113,7 +113,7 @@ void TaskPolarPatternParameters::setupParameterUI(QWidget* widget) App::Origin* origin = body->getOrigin(); auto vpOrigin = static_cast( Gui::Application::Instance->getViewProvider(origin)); - vpOrigin->setTemporaryVisibility(true, false); + vpOrigin->setTemporaryVisibility(Gui::DatumElement::Axes); } catch (const Base::Exception& ex) { Base::Console().Error("%s\n", ex.what()); diff --git a/src/Mod/PartDesign/Gui/TaskPrimitiveParameters.cpp b/src/Mod/PartDesign/Gui/TaskPrimitiveParameters.cpp index db6ac08739..539ed47128 100644 --- a/src/Mod/PartDesign/Gui/TaskPrimitiveParameters.cpp +++ b/src/Mod/PartDesign/Gui/TaskPrimitiveParameters.cpp @@ -262,7 +262,7 @@ TaskBoxPrimitives::TaskBoxPrimitives(ViewProviderPrimitive* vp, QWidget* parent) App::Origin *origin = body->getOrigin(); Gui::ViewProviderCoordinateSystem* vpOrigin {}; vpOrigin = static_cast(Gui::Application::Instance->getViewProvider(origin)); - vpOrigin->setTemporaryVisibility(true, true); + vpOrigin->setTemporaryVisibility(Gui::DatumElement::Planes | Gui::DatumElement::Axes); } catch (const Base::Exception &ex) { Base::Console().Error ("%s\n", ex.what () ); } diff --git a/src/Mod/PartDesign/Gui/TaskRevolutionParameters.cpp b/src/Mod/PartDesign/Gui/TaskRevolutionParameters.cpp index 2ba59e549b..316abc9fb2 100644 --- a/src/Mod/PartDesign/Gui/TaskRevolutionParameters.cpp +++ b/src/Mod/PartDesign/Gui/TaskRevolutionParameters.cpp @@ -100,7 +100,7 @@ TaskRevolutionParameters::TaskRevolutionParameters(PartDesignGui::ViewProvider* // show the parts coordinate system axis for selection try { if (auto vpOrigin = getOriginView()) { - vpOrigin->setTemporaryVisibility(true, false); + vpOrigin->setTemporaryVisibility(Gui::DatumElement::Axes); } } catch (const Base::Exception &ex) {