kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Hi Seth,

On 11/7/19 2:30 PM, Wayne Stambaugh wrote:
> 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.

Did you miss the previous check verifies the size of the draw items list
and returns non-zero if the list sizes are different?  The right hand
side list end check would be redundant unless we are worried thread
safety.  I suppose the check wouldn't hurt.

>> - Same with the footprint check.  Ideally, these would be the same style
>> (iterators vs. dereference) but that's minor

Same as above.

>> - Do we expect these containers to remain sorted?

I hadn't considered that but it is possible that you could have
identical objects in a different order which would fail the comparison.
 I don't see where the MULTIVECTOR is sorted any where but maybe I
missed it.  If it is not sorted than I will have to fix this.

Everything else has been fixed.  Let me know if you find anything else.

Cheers,

Wayne

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