kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxxxx>]

On 9/14/2010 2:54 AM, jean-pierre charras wrote:
> Le 13/09/2010 22:16, Wayne Stambaugh a écrit :
> ....
> 
>>
>> I'ld rather not get off topic but I think I may have found one as I have
>> wrestled with a problem for several weeks and with at least two false starts.
>> The problem stems from the current design of CMP_LIBRARY and it's ownership of
>> LIB_ALIAS and LIB_COMPONENT objects and the interdependency between them.  My
>> original attempt was to make CMP_LIBRARY own LIB_COMPONENT objects and
>> LIB_COMPONENT own it's associated LIB_ALIAS objects.  This made both searching,
>> indexing, and modifying the library and editing components much more complex
>> than the current design so I abandoned it as one of my goals was to simplify
>> the current design.  I finally decided that CMP_LIBRARY should only own
>> LIB_ALIAS objects.  I created an addition LIB_ALIAS object for the root
>> LIB_COMPONENT(more on this below).  I added a boost::shared_ptr for the
>> LIB_COMPONENT pointer in each LIB_ALIAS that references it and I added code in
>> the LIB_ALIAS destructor to keep the LIB_COMPONENT name up to date.  When the
>> last LIB_ALIAS that references the shared LIB_COMPONENT is deleted, the
>> LIB_COMPONENT object goes out of scope and is automatically deleted.  This
>> makes sense to me as LIB_ALIAS objects no meaning without an associated
>> LIB_COMPONENT.  It also made the code for updating the CMP_LIBRARY object very
>> simple.  Access to the underlying LIB_COMPONENT object is always done though
>> LIB_ALIAS::GetComponent().  I know your thinking why create an alias with the
>> same name as the component?  This change will allow for future expansion, if we
>> want to support custom fields in aliases for the so called heavy weight
>> libraries (if I am correctly understanding what the library development folks
>> are requesting).  This will probably make more sense when I submit the
>> blueprint for the new component library file format which should be in the next
>> week or so.  Except for the additional boost header files, the changes to the
>> current BZR version are pretty minimal in keeping with my preference to making
>> small incremental changes to the existing code.  I could submit a patch or
>> publish my branch if you would like to take a look at it.  If you have any
>> suggestions that can improve this design, I would be more than happy take a
>> look at it.
>>
>> Wayne
>>
> 
> Here is just an idea:
> Because handle aliases is tricky, remove them.
> 
> I believe the library could handle only 2 kind of items in memory:
> - A LIB_COMPONENT (currently like a LIB_ALIAS: having its name, fields, key
> words, doc) but no graphic info
> _ A lIB_SHAPE (currently the graphic items section of a LIB_COMPONENT) having a
> shape_name
>      and handling all graphic data (body and pins, number of parts per package)
>      but no fields, key words, doc ..  which are only in a LIB_COMPONENT
>          lIB_SHAPE have no link to LIB_COMPONENT, just their name that appear
> in some LIB_COMPONENT.
> 
> The complexity of the current code is due to the link between a root component
> (which can be see as LIB_COMPONENT plus a lIB_SHAPE) and aliases, which is
> removed here.

I used something very similar in my proposal for the new file format.  I use
definition in the file and LIB_DEFINITION in the source ( I also considered
symbol/LIB_SYMBOL ) which maps to DEF/ENDDEF in the current file format.  I
replaced current alias/LIB_ALIAS with component/LIB_COMPONENT.  For the rest of
this discussion I will used the current LIB_ALIAS/LIB_COMPONENT design to avoid
confusion.

> 
> Each component could have a link to a LIB_SHAPE.
> Mainly the name of the LIB_SHAPE, and a pointer on it just to quickly draw the
> component.
> (Could be NULL, and initialized only on request, the first time it is used)

Does creating a LIB_ALIAS object without an associated LIB_COMPONENT object
make sense?  I could not think of one so I opted to use a boost::shared_ptr to
ensure that aliases always have a valid component.  After looking at Dick's
suggestions, I may reference count the LIB_COMPONENT object in the constructor
and destructor of LIB_ALIAS.  This is essentially what boost::shared_ptr does.
 It even has a use_count() method which would suggest that it is indeed
reference counting.  I could have implemented reference counting in
LIB_COMPONENT myself but I figured why reinvent the wheel.

> When created a new LIB_COMPONENT the corresponding LIB_SHAPE name could have
> the name of the new component.
> Adding an ALIAS is just creating a new component sharing the loaded shape.
> Removing an ALIAS is just deleting a component (not the shape).
> 
> In libedit, creating the list of alias in just collect LIB_COMPONENT items
> sharing the same shape.
> (a simple linear sweep of the library), and load the shared shape in the editor.
> Removing an ALIAS is easy because there is no dependence between components,
> and saving a list is not tricky.
> (components have no dependency)

The library editor seems to have an lot of code that directly manipulates the
library being edited.  It seems to me that this could be greatly simplified as
well.  It is on my radar of things to take a look at since it will require
changes to support new library file format.

> 
> A special case is removing the component having the same name as the shape (
> the first created component )
> Just find the next component sharing the shape, and rename the shape with its
> name,
> and update all components sharing this shape.
> But this is also a very simple linear sweep.
> 
> A shape will be deleted only if there is no component "pointing" this shape.
> (After deleting a component, the linear sweep of the library can easily handle
> this)

It is even easier to do in the destructor of the LIB_ALIAS object.  Just
compare name of the alias being deleted to the associated component and if they
are the same, remove the name from the component alias list and rename the
component to next alias in the list.  Conversely, in the alias constructor you
add or replace a duplicate alias name to the component alias list.

> 
> I believe this is not a lot of changes inside the Eeschema code and a lot of
> tricky code could be removed.

I agree that it would only take some minor refactoring to make a big different
in simplifying the code not only in EESchema but many other parts of Kicad as
well.  Thanks for the input.

Wayne

> 
>