kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

I am trying to find a reasonable way to handle symbol and components
with invalid characters in their LIB_IDs (see bug report #1752419 [1]).
While now it is impossible to create such LIB_IDs, we need to handle
documents that had been created before the restriction was introduced.

LIB_IDs in SCH_COMPONENTS will have illegal characters replaced on load,
but it is not the case for LIB_{PART,ALIAS} classes. Due to that:

- Symbol-component links are broken (e.g. component with LIB_ID
'Test/Name' will change to 'Test_Name', but the corresponding LIB_PART
will remain 'Test/Name'.

- It is impossible to place/edit such symbols.

The simplest solution is to process LIB_{PART,ALIAS} LIB_IDs in the same
way as id done for SCH_COMPONENTs, so they become valid names that match
SCH_COMPONENTs using them (see the attached patches). There are two
drawbacks:

- Changing names that user had previously assigned, which might be
annoying if LIB_IDs are used e.g. to create BOMs. Personally I would use
the value field that accepts all characters for this purpose.

- Naming conflicts may occur (e.g. both 'Test/Name' and 'Test:Name' will
be changed to 'Test_Name', but I believe it is a rare case.

Any thoughts? I believe a more elegant solution will be implemented once
the new schematic file format is ready.

Cheers,
Orson

1. https://bugs.launchpad.net/kicad/+bug/1752419

From b3061c2f8c4ec2723b93e58c7a6c0d09ef15298b Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Tue, 6 Mar 2018 10:11:54 +0100
Subject: [PATCH 1/4] Added ReplaceIllegalFileNameChars() for wxString&

---
 common/string.cpp      | 37 ++++++++++++++++++++++++++++++++++---
 include/kicad_string.h |  1 +
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/common/string.cpp b/common/string.cpp
index 8f35b83d6..8932fbc18 100644
--- a/common/string.cpp
+++ b/common/string.cpp
@@ -482,8 +482,9 @@ wxString GetIllegalFileNameWxChars()
 
 bool ReplaceIllegalFileNameChars( std::string* aName, int aReplaceChar )
 {
-    bool              changed = false;
-    std::string       result;
+    bool changed = false;
+    std::string result;
+    result.reserve( aName->length() );
 
     for( std::string::iterator it = aName->begin();  it != aName->end();  ++it )
     {
@@ -503,7 +504,37 @@ bool ReplaceIllegalFileNameChars( std::string* aName, int aReplaceChar )
     }
 
     if( changed )
-        *aName =  result;
+        *aName = result;
+
+    return changed;
+}
+
+
+bool ReplaceIllegalFileNameChars( wxString& aName, int aReplaceChar )
+{
+    bool changed = false;
+    wxString result;
+    result.reserve( aName.Length() );
+
+    for( wxString::iterator it = aName.begin();  it != aName.end();  ++it )
+    {
+        if( strchr( illegalFileNameChars, *it ) )
+        {
+            if( aReplaceChar )
+                result += aReplaceChar;
+            else
+                result += wxString::Format( "%%%02x", *it );
+
+            changed = true;
+        }
+        else
+        {
+            result += *it;
+        }
+    }
+
+    if( changed )
+        aName = result;
 
     return changed;
 }
diff --git a/include/kicad_string.h b/include/kicad_string.h
index abf6a8200..3a4f45fbe 100644
--- a/include/kicad_string.h
+++ b/include/kicad_string.h
@@ -172,6 +172,7 @@ wxString GetIllegalFileNameWxChars();
  * @return true if any characters have been replaced in \a aName.
  */
 bool ReplaceIllegalFileNameChars( std::string* aName, int aReplaceChar = 0 );
+bool ReplaceIllegalFileNameChars( wxString& aName, int aReplaceChar = 0 );
 
 #ifndef HAVE_STRTOKR
 // common/strtok_r.c optionally:
-- 
2.13.3

From f7a0ee82a6ea32d0c68c97c26079feed2ada61d3 Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Tue, 6 Mar 2018 11:09:11 +0100
Subject: [PATCH 2/4] Replace illegal characters in LIB_{ALIAS,PART} LIB_IDs

Schematic components have illegal characters replaced during load,
leading to broken component-symbol links. To avoid this, library symbols
should have their names fixed in the same way.
---
 eeschema/class_libentry.cpp | 23 ++++++++++++++++-------
 eeschema/class_libentry.h   |  2 +-
 eeschema/lib_field.cpp      | 16 ++++++++++------
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/eeschema/class_libentry.cpp b/eeschema/class_libentry.cpp
index 4f7f8807b..09bab9467 100644
--- a/eeschema/class_libentry.cpp
+++ b/eeschema/class_libentry.cpp
@@ -35,6 +35,7 @@
 #include <gr_basic.h>
 #include <sch_screen.h>
 #include <richio.h>
+#include <kicad_string.h>
 
 #include <general.h>
 #include <template_fieldnames.h>
@@ -67,7 +68,7 @@ LIB_ALIAS::LIB_ALIAS( const wxString& aName, LIB_PART* aRootPart ):
     EDA_ITEM( LIB_ALIAS_T ),
     shared( aRootPart )
 {
-    name = aName;
+    SetName( aName );
 }
 
 
@@ -118,6 +119,13 @@ PART_LIB* LIB_ALIAS::GetLib()
 }
 
 
+void LIB_ALIAS::SetName( const wxString& aName )
+{
+    name = aName;
+    ReplaceIllegalFileNameChars( name, '_' );
+}
+
+
 bool LIB_ALIAS::operator==( const wxChar* aName ) const
 {
     return name == aName;
@@ -275,21 +283,22 @@ const wxString& LIB_PART::GetName() const
 
 void LIB_PART::SetName( const wxString& aName )
 {
-    m_libId.SetLibItemName( aName, false );
-
     // The LIB_ALIAS that is the LIB_PART name has to be created so create it.
-    if( m_aliases.size() == 0 )
+    if( m_aliases.empty() )
         m_aliases.push_back( new LIB_ALIAS( aName, this ) );
     else
         m_aliases[0]->SetName( aName );
 
+    // LIB_ALIAS validates the name, reuse it instead of validating the name again
+    wxString validatedName( m_aliases[0]->GetName() );
+    m_libId.SetLibItemName( validatedName, false );
+
     LIB_FIELD& valueField = GetValueField();
 
     // LIB_FIELD::SetText() calls LIB_PART::SetName(),
     // the following if-clause is to break an infinite loop
-    if( valueField.GetText() != aName )
-        valueField.SetText( aName );
-
+    if( valueField.GetText() != validatedName )
+        valueField.SetText( validatedName );
 }
 
 
diff --git a/eeschema/class_libentry.h b/eeschema/class_libentry.h
index 79cfeecce..307969025 100644
--- a/eeschema/class_libentry.h
+++ b/eeschema/class_libentry.h
@@ -125,7 +125,7 @@ public:
 
     const wxString& GetName() const         { return name; }
 
-    void SetName( const wxString& aName )   { name = aName; }
+    void SetName( const wxString& aName );
 
     void SetDescription( const wxString& aDescription )
     {
diff --git a/eeschema/lib_field.cpp b/eeschema/lib_field.cpp
index 7acbf7328..a4af0c6ff 100644
--- a/eeschema/lib_field.cpp
+++ b/eeschema/lib_field.cpp
@@ -504,26 +504,30 @@ void LIB_FIELD::SetText( const wxString& aText )
     if( aText == GetText() )
         return;
 
-    wxString oldName = m_Text;
+    wxString oldValue( m_Text );
+    wxString newValue( aText );
 
     if( m_id == VALUE && m_Parent != NULL )
     {
-        LIB_PART*      parent = GetParent();
+        LIB_PART* parent = GetParent();
 
         // Set the parent component and root alias to the new name.
         if( parent->GetName().CmpNoCase( aText ) != 0 )
-            parent->SetName( aText );
+        {
+            ReplaceIllegalFileNameChars( newValue, '_' );
+            parent->SetName( newValue );
+        }
     }
 
     if( InEditMode() )
     {
-        m_Text = oldName;
-        m_savedText = aText;
+        m_Text = oldValue;
+        m_savedText = newValue;
         m_updateText = true;
     }
     else
     {
-        m_Text = aText;
+        m_Text = newValue;
     }
 }
 
-- 
2.13.3

From bfb2e2ba78e130b9bbed72ba386e25c33ef7b6b7 Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Tue, 6 Mar 2018 11:38:54 +0100
Subject: [PATCH 3/4] SCH_LEGACY_PLUGIN_CACHE: Do not add the root alias for
 loaded symbols

The root alias is added in the loop iterating through all aliases.
---
 eeschema/sch_legacy_plugin.cpp | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/eeschema/sch_legacy_plugin.cpp b/eeschema/sch_legacy_plugin.cpp
index 0ba39f79e..02f9a1d87 100644
--- a/eeschema/sch_legacy_plugin.cpp
+++ b/eeschema/sch_legacy_plugin.cpp
@@ -2370,7 +2370,6 @@ void SCH_LEGACY_PLUGIN_CACHE::Load()
         {
             // Read one DEF/ENDDEF part entry from library:
             loadPart( reader );
-
         }
     }
 
@@ -2629,10 +2628,7 @@ LIB_PART* SCH_LEGACY_PLUGIN_CACHE::loadPart( FILE_LINE_READER& aReader )
             loadFootprintFilters( part, aReader );
         else if( strCompare( "ENDDEF", line, &line ) )   // End of part description
         {
-            // Now all is good, Add the root alias to the cache alias list.
-            m_aliases[ part->GetName() ] = part->GetAlias( part->GetName() );
-
-            // Add aliases when exist
+            // Add aliases
             for( size_t ii = 0; ii < part->GetAliasCount(); ++ii )
                 m_aliases[ part->GetAlias( ii )->GetName() ] = part->GetAlias( ii );
 
-- 
2.13.3

From 2f7ed76d83a75d6b2d0f86e699343841e9c7c9e4 Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Tue, 6 Mar 2018 11:40:03 +0100
Subject: [PATCH 4/4] SCH_LEGACY_PLUGIN_CACHE: Warn about alias name conflicts

---
 eeschema/sch_legacy_plugin.cpp | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/eeschema/sch_legacy_plugin.cpp b/eeschema/sch_legacy_plugin.cpp
index 02f9a1d87..cab657d64 100644
--- a/eeschema/sch_legacy_plugin.cpp
+++ b/eeschema/sch_legacy_plugin.cpp
@@ -2630,7 +2630,25 @@ LIB_PART* SCH_LEGACY_PLUGIN_CACHE::loadPart( FILE_LINE_READER& aReader )
         {
             // Add aliases
             for( size_t ii = 0; ii < part->GetAliasCount(); ++ii )
-                m_aliases[ part->GetAlias( ii )->GetName() ] = part->GetAlias( ii );
+            {
+                const wxString& aliasName = part->GetAlias( ii )->GetName();
+                auto it = m_aliases.find( aliasName );
+
+                if( it != m_aliases.end() )
+                {
+                    wxLogWarning( "Symbol name conflict ('%s') in library %s\n",
+                                aliasName, m_fileName );
+
+                    LIB_ALIAS* confAlias = it->second;
+
+                    // Remove the conflicting alias and its parent part
+                    // if there are not other aliases
+                    if( !confAlias->GetPart()->RemoveAlias( it->second ) )
+                        delete confAlias;
+                }
+
+                m_aliases[ aliasName ] = part->GetAlias( ii );
+            }
 
             return part.release();
         }
-- 
2.13.3

Attachment: signature.asc
Description: OpenPGP digital signature