Improved validation of library and entry names

Symbol/footprint library and entry have the same set of forbidden
characters with a single exception, space character. To accommodate for
this difference, LIB_ID validation and fix methods have been extended to
specify the LIB_ID type that is checked (schematic/board).

LIB_ID::HasIllegalChars() and LIB_ID::FixIllegalChars() had two different
sets of characters treated as invalid in LIB_IDs. The set has been
factored out to another function to avoid duplication.

common/lib_id.cpp

@@ -359,21 +359,11 @@ int LIB_ID::compare( const LIB_ID& aLibId ) const
 }
 
 
-bool LIB_ID::HasIllegalChars( const UTF8& aLibItemName )
+bool LIB_ID::HasIllegalChars( const UTF8& aLibItemName, LIB_ID_TYPE aType )
 {
     for( auto ch : aLibItemName )
     {
-        switch( ch )
+        if( !isLegalChar( ch, aType ) )
-        {
-        case '\t':
-        case '\n':
-        case '\r':
-        case ':':
-        case '/':
-            return true;
-        }
-
-        if( !wxIsascii( ch ) )
             return true;
     }
 
@@ -381,41 +371,56 @@ bool LIB_ID::HasIllegalChars( const UTF8& aLibItemName )
 }
 
 
-UTF8 LIB_ID::FixIllegalChars( const UTF8& aLibItemName )
+UTF8 LIB_ID::FixIllegalChars( const UTF8& aLibItemName, LIB_ID_TYPE aType )
 {
     UTF8 fixedName;
 
     for( UTF8::uni_iter chIt = aLibItemName.ubegin(); chIt < aLibItemName.uend(); ++chIt )
     {
         auto ch = *chIt;
-
+        fixedName += isLegalChar( ch, aType ) ? ch : '_';
-        if( !wxIsascii( ch ) )
-        {
-            fixedName += '_';
-        }
-        else
-        {
-            switch( ch )
-            {
-            case '\t':
-            case '\n':
-            case '\r':
-            case ':':
-            case '/':
-            case '\\':
-                fixedName += '_';
-                break;
-
-            default:
-                fixedName += ch;
-            }
-        }
     }
 
     return fixedName;
 }
 
 
+unsigned LIB_ID::FindIllegalChar( const UTF8& aNickname, LIB_ID_TYPE aType )
+{
+    for( unsigned ch : aNickname )
+    {
+        if( !isLegalChar( ch, aType ) )
+            return ch;
+    }
+
+    return 0;
+}
+
+
+///> Set of characters not accepted in library and entry names
+#define BASE_ILLEGAL_CHARS '\t', '\n', '\r', ':', '/', '\\'
+const unsigned schIllegalChars[] = { BASE_ILLEGAL_CHARS, ' ' };
+const unsigned pcbIllegalChars[] = { BASE_ILLEGAL_CHARS, 0 };
+#define ILL_CHAR_SIZE (sizeof(schIllegalChars) / sizeof(int))
+
+bool LIB_ID::isLegalChar( unsigned aChar, LIB_ID_TYPE aType )
+{
+    const unsigned (&illegalChars)[ILL_CHAR_SIZE] =
+        aType == ID_SCH ? schIllegalChars : pcbIllegalChars;
+
+    for( const unsigned ch : illegalChars )
+    {
+        if( ch == aChar )
+            return false;
+    }
+
+    if( !wxIsascii( aChar ) )
+        return false;
+
+    return true;
+}
+
+
 #if 0 && defined(DEBUG)
 
 // build this with Debug CMAKE_BUILD_TYPE

eeschema/dialogs/dialog_sym_lib_table.cpp

@@ -257,6 +257,7 @@ bool DIALOG_SYMBOL_LIB_TABLE::verifyTables()
         {
             wxString nick = model.GetValue( r, COL_NICKNAME ).Trim( false ).Trim();
             wxString uri  = model.GetValue( r, COL_URI ).Trim( false ).Trim();
+            unsigned illegalCh = 0;
 
             if( !nick || !uri )
             {
@@ -266,11 +267,11 @@ bool DIALOG_SYMBOL_LIB_TABLE::verifyTables()
                 // button.
                 model.DeleteRows( r, 1 );
             }
-            else if( nick.find( ':' ) != size_t( -1 ) || nick.find( ' ' ) != size_t( -1 ) )
+            else if( ( illegalCh = LIB_ID::FindIllegalChar( nick, LIB_ID::ID_SCH ) ) )
             {
                 wxString msg = wxString::Format(
-                    _( "Illegal character \"%s\" found in Nickname: \"%s\" in row %d" ),
+                    _( "Illegal character \"%c\" found in Nickname: \"%s\" in row %d" ),
-                    ( nick.find( ':' ) != size_t( -1 ) ) ? ":" : " ", GetChars( nick ), r + 1 );
+                    illegalCh, GetChars( nick ), r + 1 );
 
                 // show the tabbed panel holding the grid we have flunked:
                 if( &model != cur_model() )

eeschema/project_rescue.cpp

@@ -260,11 +260,11 @@ void RESCUE_CACHE_CANDIDATE::FindRescues( RESCUER& aRescuer,
             // and the symbol name does not have any illegal characters.
             if( ( ( cache_match && lib_match
                   && !cache_match->PinsConflictWith( *lib_match, true, true, true, true, false ) )
-                || (!cache_match && lib_match ) ) && !LIB_ID::HasIllegalChars( part_name ) )
+                || (!cache_match && lib_match ) ) && !LIB_ID::HasIllegalChars( part_name, LIB_ID::ID_SCH ) )
                 continue;
 
             // Check if the symbol has already been rescued.
-            wxString new_name = LIB_ID::FixIllegalChars( part_name );
+            wxString new_name = LIB_ID::FixIllegalChars( part_name, LIB_ID::ID_SCH );
 
             RESCUE_CACHE_CANDIDATE candidate( part_name, new_name, cache_match, lib_match );
 
@@ -382,11 +382,11 @@ void RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::FindRescues(
             if( ( ( cache_match && lib_match
                   && !cache_match->PinsConflictWith( *lib_match, true, true, true, true, false ) )
                 || (!cache_match && lib_match ) )
-              && !LIB_ID::HasIllegalChars( part_id.GetLibItemName() ) )
+              && !LIB_ID::HasIllegalChars( part_id.GetLibItemName(), LIB_ID::ID_SCH ) )
                 continue;
 
             // Fix illegal LIB_ID name characters.
-            wxString new_name = LIB_ID::FixIllegalChars( part_id.GetLibItemName() );
+            wxString new_name = LIB_ID::FixIllegalChars( part_id.GetLibItemName(), LIB_ID::ID_SCH );
 
             // Differentiate symbol name in the resue library by appending the symbol library
             // table nickname to the symbol name to prevent name clashes in the rescue library.

eeschema/sch_eagle_plugin.cpp

@@ -131,7 +131,8 @@ wxString SCH_EAGLE_PLUGIN::getLibName()
             m_libName = "noname";
 
         m_libName += "-eagle-import";
-        m_libName = LIB_ID::FixIllegalChars( m_libName );
+        // use ID_SCH as it is more restrictive
+        m_libName = LIB_ID::FixIllegalChars( m_libName, LIB_ID::ID_SCH );
     }
 
     return m_libName;
@@ -1073,7 +1074,7 @@ void SCH_EAGLE_PLUGIN::loadInstance( wxXmlNode* aInstanceNode )
         package = p->second;
     }
 
-    wxString kisymbolname = LIB_ID::FixIllegalChars( symbolname );
+    wxString kisymbolname = LIB_ID::FixIllegalChars( symbolname, LIB_ID::ID_SCH );
 
     LIB_ALIAS* alias = m_pi->LoadSymbol( getLibFileName().GetFullPath(), kisymbolname,
                                          m_properties.get() );
@@ -1296,7 +1297,7 @@ EAGLE_LIBRARY* SCH_EAGLE_PLUGIN::loadLibrary( wxXmlNode* aLibraryNode,
             if( gates_count == 1 && ispower )
                 kpart->SetPower();
 
-            wxString name = LIB_ID::FixIllegalChars( kpart->GetName() );
+            wxString name = LIB_ID::FixIllegalChars( kpart->GetName(), LIB_ID::ID_SCH );
             kpart->SetName( name );
             m_pi->SaveSymbol( getLibFileName().GetFullPath(), new LIB_PART( *kpart.get() ),
                               m_properties.get() );

include/lib_id.h

@@ -68,6 +68,9 @@ public:
 
     LIB_ID( const wxString& aId );
 
+    ///> Types of library identifiers
+    enum LIB_ID_TYPE { ID_SCH, ID_PCB };
+
     /**
      * This LIB_ID ctor is a special version which ignores the parsing due to symbol
      * names allowing '/' as a valid character.  This was causing the symbol names to
@@ -188,23 +191,37 @@ public:
      * Examine \a aLibItemName for invalid #LIB_ID item name characters.
      *
      * @param aLibItemName is the #LIB_ID name to test for illegal characters.
+     * @param aType is the library identifier type
      * @return true if \a aLibItemName contain illegal characters otherwise false.
      */
-    static bool HasIllegalChars( const UTF8& aLibItemName );
+    static bool HasIllegalChars( const UTF8& aLibItemName, LIB_ID_TYPE aType );
 
     /**
      * Replace illegal #LIB_ID item name characters with underscores '_'.
      *
      * @param aLibItemName is the #LIB_ID item name to replace illegal characters.
+     * @param aType is the library identifier type
      * @return the corrected version of \a aLibItemName.
      */
-    static UTF8 FixIllegalChars( const UTF8& aLibItemName );
+    static UTF8 FixIllegalChars( const UTF8& aLibItemName, LIB_ID_TYPE aType );
+
+    /**
+     * Looks for characters that are illegal in library and item names.
+     *
+     * @param aNickname is the logical library name to be tested.
+     * @param aType is the library identifier type
+     * @return Invalid character found in the name or 0 is the name is valid.
+     */
+    static unsigned FindIllegalChar( const UTF8& aNickname, LIB_ID_TYPE aType );
 
 #if defined(DEBUG)
     static void Test();
 #endif
 
 protected:
+    ///> Tests whether a character is a legal LIB_ID character
+    static bool isLegalChar( unsigned aChar, LIB_ID_TYPE aType );
+
     UTF8    nickname;       ///< The nickname of the library or empty.
     UTF8    item_name;      ///< The name of the entry in the logical library.
     UTF8    revision;       ///< The revision of the entry.

pcbnew/dialogs/dialog_fp_lib_table.cpp

@@ -38,6 +38,7 @@
 #include <project.h>
 #include <3d_viewer.h>      // for KISYS3DMOD
 #include <dialog_fp_lib_table_base.h>
+#include <lib_id.h>
 #include <fp_lib_table.h>
 #include <lib_table_lexer.h>
 #include <invoke_pcb_dialog.h>
@@ -290,6 +291,7 @@ private:
             {
                 wxString nick = model.GetValue( r, COL_NICKNAME ).Trim( false ).Trim();
                 wxString uri  = model.GetValue( r, COL_URI ).Trim( false ).Trim();
+                unsigned illegalCh = 0;
 
                 if( !nick || !uri )
                 {
@@ -299,11 +301,11 @@ private:
                     // button.
                     model.DeleteRows( r, 1 );
                 }
-                else if( nick.find( ':' ) != size_t( -1 ) )
+            else if( ( illegalCh = LIB_ID::FindIllegalChar( nick, LIB_ID::ID_PCB ) ) )
                 {
                     wxString msg = wxString::Format(
-                        _( "Illegal character \"%s\" found in Nickname: \"%s\" in row %d" ),
+                        _( "Illegal character \"%c\" found in Nickname: \"%s\" in row %d" ),
-                        ":", GetChars( nick ), r );
+                        illegalCh, GetChars( nick ), r );
 
                     // show the tabbed panel holding the grid we have flunked:
                     if( &model != cur_model() )