kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 11/21/2013 01:36 PM, Dick Hollenbeck wrote:
> On 11/21/2013 09:13 AM, Wayne Stambaugh wrote:
>> On 11/21/2013 9:25 AM, Dick Hollenbeck wrote:
>>>
>>>>>> Using path separators in module names was not an issue in the legacy
>>>>>> file format.  With the pretty format, it will cause the file path to be
>>>>>> incorrect.  I have it on my todo list to add filtering to prevent new
>>>>>> modules names from containing illegal file name and path characters.  I
>>>>>> will have to add this test when converting other library formats to the
>>>>>> pretty format.  My guess is this is what happened in this case.  For
>>>>>> now, we will have the fix them by hand as we come across them.
>>>>>> Hopefully there wont be too many of them.
>>>>>
>>>>>
>>>>> We ran into that in the EAGPLE_PLUGIN.  The simplest solution was to simply lie about the
>>>>> name encountered when loading the footprint.
>>>>>
>>>>> This is done in function
>>>>>
>>>>> static bool fix_eagle_package_name( string* aName )
>>>>>
>>>>> in the eagle plugin.  But it now looks like the strategy could have general applicability,
>>>>> including in LEGACY_PLUGIN.
>>>>
>>>> It might be a good idea to make this public for use in other plugins
>>>> when attempting to save pretty libraries.
>>>
>>>
>>>
>>> As to its use, I wish to add clarification since the last part of your sentence confounds
>>> me a little.  The name change happens during the eagle plugin's footprint name caching
>>> operation.  So it is a modification during loading, i.e. *reading* (not saving) of a
>>> library containing potentially "illegal" footprint names.
>>>
>>> It is only indirectly tied to "saving pretty libraries", and here's how:  Since the all
>>> names are legal after loading, there is nothing special to do for pretty save, its already
>>> been done, all names in RAM are legal already.
>>>
>>> Some plugins hold footprints in individual files, others do not.  Only for the latter
>>> case, there is potential for an illegal filename problem. That's where a footprint name
>>> translation function was used, in EAGLE_PLUGIN.  But we see the same situation now in
>>> LEGACY, not sure about Geda.  Pretty code at most might only get an assert, but it would
>>> not be necessary to change anything there.
>>
>> This would apply to any library plugin that stores multiple footprints
>> in a single file.  Although it might be confusing to the user to see
>> that a footprint name with an "illegal" file name characters has been
>> changed unexpectedly.  It might be more prudent to check for illegal
>> names and warn the user that footprint will be renamed when saving to
>> the pretty format rather than changing them at load time.  
> 
> 
> No thanks to nag warnings on footprint saves.  Jean-Pierre and I both sort of worked on
> the current plugin solution.  (Eeschema is different, it is usage by reference so a rename
> has larger consequences.)
> 
> 
> In this case, I think its easiest just to rename illegal footprints on load, otherwise I
> would not have wasted 15 minutes saying so in my last email.  If it bothers you, make it a
> PLUGIN option and teach LEGACY_PLUGIN to do it only in the presence of a particular
> option/property during FootprintLoad() or FootprintEnumerate().  It does not bother me.
> 
> 
> Carl could tune his python script with the option set when he converts everything over to
> github and pretty.  But this is needless work in my view.  The KiCad footprint libraries
> are rarely something I use in any case, probably less than 10% of any given board.  A
> footprint rename would not spook me or any one else IMO.
> 
> I think I will be using mostly the Eagle ones moving forward, and I have no prior
> expectation on how footprints should be spelled, so a rename there is tantamount to the
> original author assigning the same name that we assign to it.  Or I use footprints I have
> made.
> 
> Whatever the footprint naming policy is, I would think of it as "KiCad wide", not specific
> to pretty format.
> 
> "Yes" to nagging on keyboard entry of new illegal footprint names at any UI prompt.
> 
> 
> 
>> Geda is like pretty in that each footprint is stored in a separate file.
>>
>>>
>>> EAGLE_PLUGIN starts with 8 bit strings, and the name conversion when it happens is an
>>> exception, not the norm for speed reasons.  The "fix eagle package name" function I have
>>> works on 8 bit strings, I don't know if it is useful to a plugin working with puffy
>>> wxString footprint names.  If so, it could be sensible to make it a static member function
>>> of PLUGIN.
>>>
>>> Obviously naming policy needs to be focused into fewest control points.  Also, we're
>>> implicitly assuming that any legal base filename is also a legal footprint name, and that
>>> assumption needs verification.
>>
>> I don't recall ever seeing anything that would limit the characters used
>> for footprint names in Pcbnew but then again I've never really looked at
>> it that closely.  It probably makes sense to enforce the pretty naming
>> policy even when creating new footprints in legacy libraries to prevent
>> issues when converting them to the pretty format.  That only leaves
>> verifying legacy and third party libraries for illegal names when
>> converting to the pretty format.
>>
>>>
>>>
>>> (BTW, I admit to be coming down with an ever increasing distaste for wxString.  I don't
>>> like broccoli anymore either, and yet it is good for me.  wxString is only good for me to
>>> the extend that is eases use of wxWidgets.  I don't see where it makes programming any
>>> easier however.)
>>
>> This issue should go away for the most part once wxWidgets 3 becomes the
>> minimum requirement to build KiCad.
> 
> That is not what I see, unless you decide to use 8 bit strings in the current locale.  And
> I have had a reversal of opinion on that.  I think 8 bit strings in KiCad might be better
> held in UTF8 format in any case.  This makes the new wxString constructor not useable:
> 
> 	wxString( const char* )
> 	wxString( std::string )
> 
> since these both convert from current locale encoding, not explicitly UTF8.
> 
> (And although current locale on linux is typically UTF8, you cannot assume this.)
> 
> So it will take a lot of augmentation to come up with a sound future strategy.
> 
> I would recommend our 


own

> class derived from wxString() so we can override the above two
> constructors, but still pass them around to wx APIs as if they were really wxStrings.
> 
> We could do that now, even with wx 2.8, and get on with it.
> 
> 
> Dick
>