kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Hey Seth,

On 11/7/19 12:22 PM, Seth Hillbrand wrote:
> On 2019-11-06 12:34, Wayne Stambaugh wrote:
>> I finally finish up the initial attempt at simple inheritance for the
>> symbol library code.  The change is non-trivial and will likely have
>> some unexpected side affects that I missed.  I pushed the branch to my
>> personal repo[1] and I would like a few pair of extra eyes on it before
>> I merge it into master.  If you prefer, I can set up a merge request
>> from this branch in Launchpad.
>>
>> The biggest user facing change is that the concept of aliases is gone
>> which means internally, the LIB_ALIAS object has been removed and all of
>> the information it managed was moved into LIB_PART.  This actually
>> simplified some of the UI code but made the LIB_MANAGER and LIB_PART
>> object code more complicated.  The symbol library editor works
>> significantly different now.  When you select a derived part (formerly a
>> LIB_ALIAS), it will show the parent symbol that it was derived from
>> darkened and all of the edit features disabled.  Until the new symbol
>> file format is done and writing to the legacy symbol file format is
>> deprecated, this restriction will have to remain in place.  The
>> properties editor no longer has an aliases panel and is limited to what
>> it can change in the derived or parent symbols depending on which symbol
>> is currently being edited.  I suspect this will be the biggest stumbling
>> block for existing users so if you can think of a better way to handle
>> this, I'm open to suggestion.  The other thing that I am concerned about
>> is symbol library editing.  There were a lot of LIB_MANAGER object
>> changes which I am not 100% confident in.
>>
>> Just a couple of other internal changes to be aware of:
>>
>> A flattened copy of the symbol is used in SCH_COMPONENT instead of
>> relying on the weak reference to the library symbol.  This way we don't
>> have to worry about the shared pointer disappearing and causing issues.
>> However, this will require that we be diligent about updating modified
>> symbol libraries in the schematic.  Otherwise, the schematic could
>> change the next time it is loaded.  I'm open to changing this back if we
>> think it's going to be an issue.
>>
>> There is now a compare function for the LIB_PART object which can be
>> rather tricky.  I created a new test suite inside the current LIB_PART
>> test file so if you change anything, please run the test to ensure
>> nothing gets broken.  I also added a bunch of other new tests for the
>> LIB_PART object.
>>
>> I added code to DIALOG_SHIM to allow the caller to reset the last dialog
>> size when hiding dialog control state changes between dialog instances.
>> We have some dialog windows that are not the correct default size
>> depending on which controls are shown so there is now a convenient way
>> to address this.
>>
>> Please let me know if you find anything so I can get it fixed and merged
>> into master.  The next step is to convert the schematic internal units
>> from 1mil to 10nm.  Once that step is complete, I will knock out the new
>> symbol library format.  Thanks in advance for the help.
>>
>> Cheers,
>>
>> Wayne
>>
>> [1]:https://code.launchpad.net/~stambaughw/kicad/+git/kicad-dev/+ref/lib-alias-merge
>>
>>
> 
> Hi Wayne-
> 
> I'm psyched to see this progress!  Congrats.  I'll have a chance to
> fully test after next week but I wanted to get you the code read
> feedback before then.
> 
> 1) Compare function:
> - It looks like rhs isn't checked for end() in the loop.  While size
> gets checked ahead of time, we've had a couple cases where code gets
> added between checks and loops in the past, so it's probably safer to
> put the check in the loop.
> - Same with the footprint check.  Ideally, these would be the same style
> (iterators vs. dereference) but that's minor
> - Do we expect these containers to remain sorted?

I'm not sure thats strictly necessary but I will take a look at it.

> 
> 2) You have a few C-style casts in Flatten()>
> 3) The LIB_PART() copy constructor should take a const reference rather
> than the existing one, that should remove the requirement of using
> const_cast in Flatten()

Gaaa!  How did I miss that?

> 
> 4) That ternary in GetNextDrawItem() is breaking my brain a bit.

This will only get worse with multiple inheritance.  Draw items from
each parent will have to be recused to create a final image of the symbol.

> 
> 5) In footprint_info.h, if we are returning the wxString itself (instead
> of the reference), it shouldn't be const.  (addendum: Also loadAliases())

Missed another one.

> 
> 6) sch_view.cpp has a couple c-style casts that can probably be
> compressed into our dyn_cast routine.
> 
> 7) DeleteSymbol mixes up it and it1 in the while() loop.

I'll take a look at it.

> 
> I can't wait to test it out!  On the usability issue for aliases, I
> suspect that this is a time limited constraint, so there shouldn't be
> large issues as people should not be building aliased libraries with the
> master branch at this point.  We may want to clarify that in a list
> message.  Overall, I am very excited for the new formats.  Thanks again
> for taking on this huge job!

Thanks for the review, I should have most of the issues above fixed by
in the next day or so.  There are lot more big changes coming.  I
desperately want to have the new file formats in place by FOSDEM.

Cheers,

Wayne

> 
> Best-
> Seth
> 
> Seth Hillbrand
> KiCad Services Corporation
> https://www.kipro-pcb.com
> +1 530 302 5483 | +1 212 603 9372