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 _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
