kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

Hi,

This patch makes SVG import a CMake option. This allows interested
developers to work on this feature without having to maintain a patch
on top to disable the define. While possible with Git, it is certainly
a bit fiddly.

The CMake option is KICAD_EXPERIMENTAL_IMPORT_SVG. Set to ON to use SVG import.

I have documented this in the Pcbnew CMake, rather than the top level
CMake, as it's a developer option, and should not be set by general
packagers. This keeps it near the set_property call that sets the
define in the file needed (which will be removed, along with the
compile flag when SVG import is done).

Now for a proposal:

I think ideally, this would not even be a #define, but instead a run
time option, to eliminate any dead code in the master branch that is
not truly required. Dead code is vulnerable to bit rot. I think we
should have a way to define and set experimental and advanced options
for fine configuration. Somewhat like Firefox's about:config options,
but without the UI (unless someone wants to make that effort, of
course!)

Shipping development features in the main branch, as built code, but
protected by run-time feature flags is a common practice, as it not
only makes it easier for developers to work on the code, but it gets
the code, as-is, into the CI, analysis and testing systems ASAP, and
bugs can be found sooner rather than during a big code dump from a
private branch, when it's easy to miss one bug out of many.

This will allow to use custom configs for control of things that
currently need recompilation to achieve. It will also hopefully
encourage people to write parametrisable code, which is likely to be
more testable code. I suggest perhaps placing them in a separate
"advanced" config file, so that you can use XDG_CONFIG_DIR to switch
between multiple configs if wanted during testing. This is VERY
helpful for A/B testing.

The implication of these advanced configs is that if you set them for
development, or for or advanced use only, and non-default settings are
not officially supported. If a setting is deemed useful enough to
become supported, it should be given UI and put into the existing
config files. Perhaps any set advanced config should be included in
the version info dump.ki

Cheers,

John

From a38059bc60daec5af906b85830b6ed6b74234ee6 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Tue, 4 Dec 2018 13:48:47 +0000
Subject: [PATCH] Make SVG import a CMake option (experimental)

This allows too turn it on for work without an additional patch.

Also fix a bug in the disable code - when the SVG is enabled,
allow to select SVG files.
---
 pcbnew/CMakeLists.txt                   | 20 ++++++++++++++++++++
 pcbnew/import_gfx/dialog_import_gfx.cpp | 14 +-------------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/pcbnew/CMakeLists.txt b/pcbnew/CMakeLists.txt
index 6c1eb44fd..8cac3a110 100644
--- a/pcbnew/CMakeLists.txt
+++ b/pcbnew/CMakeLists.txt
@@ -852,3 +852,23 @@ if( APPLE )
             )
     endif()
 endif()
+
+# Experimental feature flags
+#
+
+# Currently (Version 5.1) SVG import is disabled by default, to avoid issues
+# SVG needs some enhancements.
+# Especially, all SVG shapes are imported as curves and converted to a lot of segments.
+# A better approach is to convert to polylines (not yet existing in Pcbnew) and keep
+# arcs and circles as primitives (not yet possible with tinysvg library.
+# So, until these issues are solved, disable SVG import option.
+# Warning: enable svg import is currently only for developers.
+option( KICAD_EXPERIMENTAL_ENABLE_SVG "Experimental: Enable SVG import" OFF)
+
+if( NOT KICAD_EXPERIMENTAL_ENABLE_SVG )
+    set_property(
+        SOURCE import_gfx/dialog_import_gfx.cpp
+        PROPERTY COMPILE_DEFINITIONS DISABLE_SVG_IMPORT
+        APPEND
+    )
+endif()
\ No newline at end of file
diff --git a/pcbnew/import_gfx/dialog_import_gfx.cpp b/pcbnew/import_gfx/dialog_import_gfx.cpp
index 819856cbc..48f098876 100644
--- a/pcbnew/import_gfx/dialog_import_gfx.cpp
+++ b/pcbnew/import_gfx/dialog_import_gfx.cpp
@@ -49,22 +49,12 @@
 #define IMPORT_GFX_LINEWIDTH_UNITS_KEY          "GfxImportLineWidthUnits"
 #define IMPORT_GFX_LINEWIDTH_KEY                "GfxImportLineWidth"
 
-// Currently (Version 5.1) SVG import is disabled by default, to avoid issues
-// SVG needs some enhancements.
-// Especially, all SVG shapes are imported as curves and converted to a lot of segments.
-// A better approach is to convert to polylines (not yet existing in Pcbnew) and keep
-// arcs and circles as primitives (not yet possible with tinysvg library.
-// So, until these issues are solved, disable SVG import option.
-// Warning: enable svg import is currently only for developers.
-#define DISABLE_SVG_IMPORT
-
 // Static members of DIALOG_IMPORT_GFX, to remember
 // the user's choices during the session
 wxString DIALOG_IMPORT_GFX::m_filename;
 bool DIALOG_IMPORT_GFX::m_placementInteractive = true;
 LAYER_NUM DIALOG_IMPORT_GFX::m_layer = Dwgs_User;
-double DIALOG_IMPORT_GFX::m_scaleImport = 1.0;  // Do not change the imported items siaz
-
+double DIALOG_IMPORT_GFX::m_scaleImport = 1.0;  // Do not change the imported items size
 
 DIALOG_IMPORT_GFX::DIALOG_IMPORT_GFX( PCB_BASE_FRAME* aParent, bool aImportAsFootprintGraphic )
     : DIALOG_IMPORT_GFX_BASE( aParent )
@@ -250,8 +240,6 @@ void DIALOG_IMPORT_GFX::onBrowseFiles( wxCommandEvent& event )
 
     for( auto pluginType : GRAPHICS_IMPORT_MGR::GFX_FILE_TYPES )
     {
-        if( !pluginType == GRAPHICS_IMPORT_MGR::GFX_FILE_T::DXF )
-            continue;
         auto plugin = GRAPHICS_IMPORT_MGR::GetPlugin( pluginType );
         const auto wildcards = plugin->GetWildcards();
 
-- 
2.19.1