kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

After the various fixes for the file extension handling in the recent
weeks, another small bug was introduced for GTK this time, specifically
that the extension displayed to the user in the file selector box included
the full regex that was used to match the extension.

This patch fixes that issue, but also reorganizes the extension handling to
clean it up. Specifically it makes the file dialog creation use the
standard functions for creating the dialog wildcard strings, and creates a
new common function performs file extension matching (this is then
referenced by the plugin manager to clean up the code there).

The 5.1 branch also shows this issue, so it would be good to backport this
fix to 5.1 as well.

-Ian

From d13c0996c0179c7aac22bd34e093edf99ca51973 Mon Sep 17 00:00:00 2001
From: Ian McInerney <Ian.S.McInerney@xxxxxxxx>
Date: Thu, 26 Sep 2019 22:09:09 +0200
Subject: [PATCH] pcbnew: Clean up extension handling in graphics plugins

* Fix wildcard display in the file selector dialog (on GTK
  it would show the regex to the user)
* Move the file extension comparison into a common function
---
 common/wildcards_and_files_ext.cpp         | 27 +++++++++++-
 include/wildcards_and_files_ext.h          | 22 +++++++++-
 pcbnew/import_gfx/dialog_import_gfx.cpp    |  6 +--
 pcbnew/import_gfx/dxf_import_plugin.h      |  6 +--
 pcbnew/import_gfx/graphics_import_mgr.cpp  |  9 +---
 pcbnew/import_gfx/graphics_import_plugin.h |  7 +--
 pcbnew/import_gfx/svg_import_plugin.h      |  6 +--
 qa/common/test_wildcards_and_files_ext.cpp | 51 ++++++++++++++++++++++
 qa/pcbnew/test_graphics_import_mgr.cpp     |  2 +-
 9 files changed, 114 insertions(+), 22 deletions(-)

diff --git a/common/wildcards_and_files_ext.cpp b/common/wildcards_and_files_ext.cpp
index 37fab403f..5eb77757f 100644
--- a/common/wildcards_and_files_ext.cpp
+++ b/common/wildcards_and_files_ext.cpp
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2018 Jean-Pierre Charras, jp.charras at wanadoo.fr
  * Copyright (C) 2008 Wayne Stambaugh <stambaughw@xxxxxxxxx>
- * Copyright (C) 1992-2018 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2019 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -27,9 +27,34 @@
  * @file wildcards_and_files_ext.cpp
  * Definition of file extensions used in Kicad.
  */
+#include <regex>
 #include <wildcards_and_files_ext.h>
 
 
+bool compareFileExtensions( const std::string& aExtension,
+        const std::vector<std::string>& aReference, bool aCaseSensitive )
+{
+    // Form the regular expression string by placing all possible extensions into it as alternatives
+    std::string regexString = "(";
+    bool        first = true;
+    for( auto ext : aReference )
+    {
+        // The | separate goes between the extensions
+        if( !first )
+            regexString += "|";
+        else
+            first = false;
+
+        regexString += ext;
+    }
+    regexString += ")";
+
+    // Create the regex and see if it matches
+    std::regex extRegex( regexString, aCaseSensitive ? std::regex::ECMAScript : std::regex::icase );
+    return std::regex_match( aExtension, extRegex );
+}
+
+
 wxString formatWildcardExt( const wxString& aWildcard )
 {
     wxString wc;
diff --git a/include/wildcards_and_files_ext.h b/include/wildcards_and_files_ext.h
index 907a684f8..c1c70d4f7 100644
--- a/include/wildcards_and_files_ext.h
+++ b/include/wildcards_and_files_ext.h
@@ -4,7 +4,7 @@
  * Copyright (C) 2018 Jean-Pierre Charras, jp.charras at wanadoo.fr
  * Copyright (C) 2007-2012 SoftPLC Corporation, Dick Hollenbeck <dick@xxxxxxxxxxx>
  * Copyright (C) 2008 Wayne Stambaugh <stambaughw@xxxxxxxxx>
- * Copyright (C) 1992-2018 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2019 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -49,6 +49,26 @@
  * @{
  */
 
+
+/**
+ * Compare the given extension against the reference extensions to see if it matches any
+ * of the reference extensions.
+ *
+ * This function uses the C++ regular expression functionality to perform the comparison,
+ * so the reference extensions can be regular expressions of their own right. This means
+ * that partial searches can be made, for example ^g.* can be used to see if the first
+ * character of the extension is g. The reference extensions are concatenated together
+ * as alternatives when doing the evaluation (e.g. (dxf|svg|^g.*) ).
+ *
+ * @param aExtension is the extension to test
+ * @param aReference is a vector containing the extensions to test against
+ * @param aCaseSensitive says if the comparison should be case sensitive or not
+ *
+ * @return if the extension matches any reference extensions
+ */
+bool compareFileExtensions( const std::string& aExtension,
+        const std::vector<std::string>& aReference, bool aCaseSensitive = false );
+
 /**
  * Build the wildcard extension file dialog wildcard filter to add to the base message dialog.
  *
diff --git a/pcbnew/import_gfx/dialog_import_gfx.cpp b/pcbnew/import_gfx/dialog_import_gfx.cpp
index 90029fe5c..36a8d9533 100644
--- a/pcbnew/import_gfx/dialog_import_gfx.cpp
+++ b/pcbnew/import_gfx/dialog_import_gfx.cpp
@@ -241,10 +241,10 @@ void DIALOG_IMPORT_GFX::onBrowseFiles( wxCommandEvent& event )
     for( auto pluginType : m_gfxImportMgr->GetImportableFileTypes() )
     {
         auto       plugin = m_gfxImportMgr->GetPlugin( pluginType );
-        const auto wildcards = plugin->GetWildcards();
+        const auto extensions = plugin->GetFileExtensions();
 
-        wildcardsDesc += "|" + plugin->GetName() + " (" + wildcards + ")|" + wildcards;
-        allWildcards += wildcards + ";";
+        wildcardsDesc += "|" + plugin->GetName() + AddFileExtListToFilter( extensions );
+        allWildcards += plugin->GetWildcards() + ";";
     }
 
     wildcardsDesc = _( "All supported formats|" ) + allWildcards + wildcardsDesc;
diff --git a/pcbnew/import_gfx/dxf_import_plugin.h b/pcbnew/import_gfx/dxf_import_plugin.h
index e774ed2c8..d738f6e9a 100644
--- a/pcbnew/import_gfx/dxf_import_plugin.h
+++ b/pcbnew/import_gfx/dxf_import_plugin.h
@@ -146,10 +146,10 @@ public:
         return "AutoCAD DXF";
     }
 
-    const wxArrayString GetFileExtensions() const override
+    const std::vector<std::string> GetFileExtensions() const override
     {
-        static wxString wildcardExt = formatWildcardExt( "dxf" );
-        return wxArrayString( 1, &wildcardExt );
+        static std::vector<std::string> exts = { "dxf" };
+        return exts;
     }
 
     bool Load( const wxString& aFileName ) override;
diff --git a/pcbnew/import_gfx/graphics_import_mgr.cpp b/pcbnew/import_gfx/graphics_import_mgr.cpp
index 9be929fb9..2d969fdc3 100644
--- a/pcbnew/import_gfx/graphics_import_mgr.cpp
+++ b/pcbnew/import_gfx/graphics_import_mgr.cpp
@@ -70,13 +70,8 @@ std::unique_ptr<GRAPHICS_IMPORT_PLUGIN> GRAPHICS_IMPORT_MGR::GetPluginByExt(
         auto plugin = GetPlugin( fileType );
         auto fileExtensions = plugin->GetFileExtensions();
 
-        for( const auto& fileExt : fileExtensions )
-        {
-            wxRegEx extensions( fileExt, wxRE_ICASE );
-
-            if( extensions.Matches( aExtension ) )
-                return plugin;
-        }
+        if( compareFileExtensions( aExtension.ToStdString(), fileExtensions ) )
+            return plugin;
     }
 
     return {};
diff --git a/pcbnew/import_gfx/graphics_import_plugin.h b/pcbnew/import_gfx/graphics_import_plugin.h
index 548a88724..2ef00c5ee 100644
--- a/pcbnew/import_gfx/graphics_import_plugin.h
+++ b/pcbnew/import_gfx/graphics_import_plugin.h
@@ -26,6 +26,7 @@
 #ifndef GRAPHICS_IMPORT_PLUGIN_H
 #define GRAPHICS_IMPORT_PLUGIN_H
 
+#include <wildcards_and_files_ext.h>
 #include <wx/arrstr.h>
 
 class GRAPHICS_IMPORTER;
@@ -56,9 +57,9 @@ public:
     virtual const wxString GetName() const = 0;
 
     /**
-     * Return a string array of the file extensions handled by this plugin.
+     * Return a vector of the file extensions handled by this plugin.
      */
-    virtual const wxArrayString GetFileExtensions() const = 0;
+    virtual const std::vector<std::string> GetFileExtensions() const = 0;
 
     /**
      * Return a list of wildcards that contains the file extensions
@@ -76,7 +77,7 @@ public:
             else
                 ret += ";";
 
-            ret += "*." + extension;
+            ret += "*." + formatWildcardExt( extension );
         }
 
         return ret;
diff --git a/pcbnew/import_gfx/svg_import_plugin.h b/pcbnew/import_gfx/svg_import_plugin.h
index 78e49b8b3..708f6aeca 100644
--- a/pcbnew/import_gfx/svg_import_plugin.h
+++ b/pcbnew/import_gfx/svg_import_plugin.h
@@ -43,10 +43,10 @@ public:
         return "Scalable Vector Graphics";
     }
 
-    const wxArrayString GetFileExtensions() const override
+    const std::vector<std::string> GetFileExtensions() const override
     {
-        static wxString wildcardExt = formatWildcardExt( "svg" );
-        return wxArrayString( 1, &wildcardExt );
+        static std::vector<std::string> exts = { "svg" };
+        return exts;
     }
 
     /**
diff --git a/qa/common/test_wildcards_and_files_ext.cpp b/qa/common/test_wildcards_and_files_ext.cpp
index 6efdd5336..fd2a0dbbc 100644
--- a/qa/common/test_wildcards_and_files_ext.cpp
+++ b/qa/common/test_wildcards_and_files_ext.cpp
@@ -70,6 +70,57 @@ static constexpr bool should_use_regex_filters()
 }
 
 
+// Structure used to store the extensions to test and the expected comparison result
+struct testExtensions
+{
+    std::string ext;
+    bool        insense_result;
+    bool        sense_result;
+};
+
+const static std::vector<std::string>    extensionList = { "dxf", "svg", "SCH", "^g.*" };
+const static std::vector<testExtensions> testExtensionList = {
+    {
+            "dxf",
+            true, // Case insensitive comparison result
+            true  // Case sensitive comparison result
+    },
+    {
+            "sch",
+            true, // Case insensitive comparison result
+            false // Case sensitive comparison result
+    },
+    {
+            "gbr",
+            true, // Case insensitive comparison result
+            true  // Case sensitive comparison result
+    },
+    {
+            "pcb",
+            false, // Case insensitive comparison result
+            false  // Case sensitive comparison result
+    }
+};
+
+/**
+ * Check correct comparison of file names
+ */
+BOOST_AUTO_TEST_CASE( FileNameComparison )
+{
+    for( const auto& testExt : testExtensionList )
+    {
+        bool extPresent_insense = compareFileExtensions( testExt.ext, extensionList, false );
+        bool extPresent_sense = compareFileExtensions( testExt.ext, extensionList, true );
+
+        BOOST_TEST_INFO( "Case insensitive test for extension " + testExt.ext );
+        BOOST_CHECK_EQUAL( extPresent_insense, testExt.insense_result );
+
+        BOOST_TEST_INFO( "Case sensitive test for extension " + testExt.ext );
+        BOOST_CHECK_EQUAL( extPresent_sense, testExt.sense_result );
+    }
+}
+
+
 /**
  * Check correct handling of filter strings (as used by WX)
  */
diff --git a/qa/pcbnew/test_graphics_import_mgr.cpp b/qa/pcbnew/test_graphics_import_mgr.cpp
index bec3a75de..ae437a37b 100644
--- a/qa/pcbnew/test_graphics_import_mgr.cpp
+++ b/qa/pcbnew/test_graphics_import_mgr.cpp
@@ -42,7 +42,7 @@ static bool pluginHandlesExt( const GRAPHICS_IMPORT_PLUGIN& aPlugin, const std::
 
     for( auto ext : exts )
     {
-        std::regex ext_reg( ext.ToStdString() );
+        std::regex ext_reg( ext );
 
         if( std::regex_match( aExt, ext_reg ) )
             return true;
-- 
2.21.0