kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxxxx>]

On 11/21/2013 2: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.

I don't have any issues with silently changing the file names in order
be able to write them to the pretty format.

> 
> 
> 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.

This should always be the case.  The user should not be able to enter
illegal characters.  I'll modify the footprint name text edit controls
to complain when an illegal character is entered.

> 
> 
> 
>> 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 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.

This certainly would be a lot cleaner than what we have to do now to get
wxString to play nice with std::string.  I'm hoping this new class would
make using the FROM_UTF8() and TO_UTF8() macros obsolete.

> 
> 
> Dick
> 
> 
> 
> 
> 
>>
>>>
>>>
>>> Dick
>>>
>>>
>>>>
>>>>>
>>>>> The advertised names are converted before even using them, from FootprintLoad() or cacheLib().
>>>>>
>>>>> This is the chosen path for EAGLE anyways.
>>>>>
>>>>> Note that it was not good enough to do a character to character swap.  That lead to a
>>>>> duplicate footprint name.  No foolin', murphy is always in charge.
>>>>>
>>>>>
>>>>>    We were swapping '/'  with say '-'.
>>>>>
>>>>>
>>>>> But then ran into a library with say both  'XYZ/' and 'XYZ-' in the same library!
>>>>
>>>> Nobody ever said this was going to be easy :)
>>>>
>>>>>
>>>>> Back to the drawing board.  What's now in place is a URL encoding like substitution.
>>>>> Nobody would ever name their footprint 'XYZ%2f'  would they?
>>>>
>>>> It seems like it would be safe but I wouldn't bet on it.  There is
>>>> always that one person who will confound your logic.
>>>>
>>>> Wayne
>>>>
>>>>>
>>>>> Please say no, because finding a unique name is the goal.  After one read / write cycle, a
>>>>> person can always use the module editor to rename anything with a % in it.  Or in the case
>>>>> of pretty, just a file manager.
>>>>>
>>>>>
>>>>> Dick
>>>>>
>>>>
>>>>