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