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.sumin...@cern.ch>
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.sumin...@cern.ch>
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.sumin...@cern.ch>
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.sumin...@cern.ch>
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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
