From a36a4b04e5e3967c0fd8c19d04d603621cff0fa1 Mon Sep 17 00:00:00 2001 From: Kevin Martin Date: Mon, 23 Sep 2024 11:28:00 -0400 Subject: [PATCH] DXF: Fix Import related behavior reported in issues #13597 and #16068 (#16511) * Fix double-import on exception in ImportGUI.readDXF The python code has a try/catch block intended to detect if the ImportGUI module is present and if so use its readDXF method, otherwise use Import.readDXF. The block was incorrectly structured so that Import.readDXF would also be called if ImportGUI.readDXF raised an exception, causing the DXF file contents to be loaded twice into the drawing (at least, up to the point where the exception occurred) * Make ImpExpDxfRead::MakeLayer use centralized accessor for Draft module The importer class already had a method to find the python Draft module, but MakeLayer was doing the work itself. Now it uses the centralized accessor, which has also been improved to generate a message if the module can't be loaded. * Give compounded objects names related to the containing layer name If "Use Layers" is set and also "Group layers into blocks" the names of the generated compound objects will be based on the name of the containing layer. If "Use Layers" is not set this will not occur because in general objects from several layers would be compounded together. Fixes (partially) #16068, the remaining fix is likely just an explanation of the interaction of the options. closes #13597 * Use correct (new) property name OverrideShapeAppearanceChildren This corrects a bug created by commit 3aea798 which renamed the layer property "OverrideShapeColorChildren" to "OverrideShapeAppearanceChildren" but missed this particular reference to the property by its old name. The code here called PyObject_SetAttrString which, due to the wrong attribute name, set an expection state in the Python engine, ultimately causing the next attempt at importing a module to fail, with various consequences depending on why the module is being imported. * Wrap PyObject_SetAttrString and PyObject_GetAttrString with error-checkers In DEBUG compiles these methods are wrapped to report errors which can occur if the property cannot be referenced. * Make some error-printers static instead of const They don't need the CDxfRead object to work * Display exceptions raised by ReadEntity Although the DXF reader has an ignore-errors flag, it should still report any errors it encounters. The flag is deemed to mean that, after an error, the reader should continue to try to read the file rather than quitting on the first error. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- src/Mod/Draft/importDXF.py | 6 ++++-- src/Mod/Import/App/dxf/ImpExpDxf.cpp | 12 ++++++++---- src/Mod/Import/App/dxf/ImpExpDxf.h | 4 ++++ src/Mod/Import/App/dxf/dxf.cpp | 4 ++++ src/Mod/Import/App/dxf/dxf.h | 25 +++++++++++++++++++++++-- 5 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/Mod/Draft/importDXF.py b/src/Mod/Draft/importDXF.py index 8baee72332..9868aa1077 100644 --- a/src/Mod/Draft/importDXF.py +++ b/src/Mod/Draft/importDXF.py @@ -2832,10 +2832,11 @@ def open(filename): FreeCAD.setActiveDocument(doc.Name) try: import ImportGui - ImportGui.readDXF(filename) except Exception: import Import Import.readDXF(filename) + else: + ImportGui.readDXF(filename) Draft.convert_draft_texts() # convert annotations to Draft texts doc.recompute() @@ -2874,10 +2875,11 @@ def insert(filename, docname): else: try: import ImportGui - ImportGui.readDXF(filename) except Exception: import Import Import.readDXF(filename) + else: + ImportGui.readDXF(filename) Draft.convert_draft_texts() # convert annotations to Draft texts doc.recompute() diff --git a/src/Mod/Import/App/dxf/ImpExpDxf.cpp b/src/Mod/Import/App/dxf/ImpExpDxf.cpp index 24c49d492d..931c45abfe 100644 --- a/src/Mod/Import/App/dxf/ImpExpDxf.cpp +++ b/src/Mod/Import/App/dxf/ImpExpDxf.cpp @@ -103,7 +103,10 @@ bool ImpExpDxfRead::ReadEntitiesSection() // TODO: We do end-to-end joining or complete merging as selected by the options. for (auto& shapeSet : ShapesToCombine) { m_entityAttributes = shapeSet.first; - CombineShapes(shapeSet.second, "Compound"); + CombineShapes(shapeSet.second, + m_entityAttributes.m_Layer == nullptr + ? "Compound" + : m_entityAttributes.m_Layer->Name.c_str()); } } else { @@ -644,7 +647,7 @@ ImpExpDxfRead::MakeLayer(const std::string& name, ColorIndex_t color, std::strin App::Color appColor = ObjectColor(color); PyObject* draftModule = nullptr; PyObject* layer = nullptr; - draftModule = PyImport_ImportModule("Draft"); + draftModule = getDraftModule(); if (draftModule != nullptr) { // After the colours, I also want to pass the draw_style, but there is an intervening // line-width parameter. It is easier to just pass that parameter's default value than @@ -667,12 +670,13 @@ ImpExpDxfRead::MakeLayer(const std::string& name, ColorIndex_t color, std::strin appColor.b, 2.0, "Solid"); - Py_DECREF(draftModule); } auto result = new Layer(name, color, std::move(lineType), layer); if (result->DraftLayerView != nullptr) { PyObject_SetAttrString(result->DraftLayerView, "OverrideLineColorChildren", Py_False); - PyObject_SetAttrString(result->DraftLayerView, "OverrideShapeColorChildren", Py_False); + PyObject_SetAttrString(result->DraftLayerView, + "OverrideShapeAppearanceChildren", + Py_False); } // We make our own layer class even if we could not make a layer. MoveToLayer will ignore diff --git a/src/Mod/Import/App/dxf/ImpExpDxf.h b/src/Mod/Import/App/dxf/ImpExpDxf.h index 5c0478c936..3c71aa1f29 100644 --- a/src/Mod/Import/App/dxf/ImpExpDxf.h +++ b/src/Mod/Import/App/dxf/ImpExpDxf.h @@ -122,7 +122,11 @@ protected: PyObject* getDraftModule() { if (DraftModule == nullptr) { + static int times = 0; DraftModule = PyImport_ImportModule("Draft"); + if (DraftModule == nullptr && times++ == 0) { + ImportError("Unable to locate \"Draft\" module"); + } } return DraftModule; } diff --git a/src/Mod/Import/App/dxf/dxf.cpp b/src/Mod/Import/App/dxf/dxf.cpp index ba8161cfae..8dab411337 100644 --- a/src/Mod/Import/App/dxf/dxf.cpp +++ b/src/Mod/Import/App/dxf/dxf.cpp @@ -2910,7 +2910,11 @@ bool CDxfRead::ReadEntitiesSection() return false; } } + catch (const Base::Exception& e) { + e.ReportException(); + } catch (...) { + ImportError("CDxfRead::ReadEntity raised unknown exception\n"); } } else { diff --git a/src/Mod/Import/App/dxf/dxf.h b/src/Mod/Import/App/dxf/dxf.h index a661e39b19..b9a82e123f 100644 --- a/src/Mod/Import/App/dxf/dxf.h +++ b/src/Mod/Import/App/dxf/dxf.h @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -685,12 +686,12 @@ protected: // notification popup "Log" goes to a log somewhere and not to the screen/user at all template - void ImportError(const char* format, args&&... argValues) const + static void ImportError(const char* format, args&&... argValues) { Base::ConsoleSingleton::Instance().Warning(format, std::forward(argValues)...); } template - void ImportObservation(const char* format, args&&... argValues) const + static void ImportObservation(const char* format, args&&... argValues) { Base::ConsoleSingleton::Instance().Message(format, std::forward(argValues)...); } @@ -922,5 +923,25 @@ public: return m_entityAttributes.m_LineType[0] == 'h' || m_entityAttributes.m_LineType[0] == 'H'; } static App::Color ObjectColor(ColorIndex_t colorIndex); // as rgba value + +#ifdef DEBUG +protected: + static PyObject* PyObject_GetAttrString(PyObject* o, const char* attr_name) + { + PyObject* result = ::PyObject_GetAttrString(o, attr_name); + if (result == nullptr) { + ImportError("Unable to get Attribute '%s'\n", attr_name); + PyErr_Clear(); + } + return result; + } + static void PyObject_SetAttrString(PyObject* o, const char* attr_name, PyObject* v) + { + if (::PyObject_SetAttrString(o, attr_name, v) != 0) { + ImportError("Unable to set Attribute '%s'\n", attr_name); + PyErr_Clear(); + } + } +#endif }; #endif