On 7/3/20 7:31 PM, Reece R. Pollack wrote: > On 7/3/20 5:42 PM, Jeff Young wrote: >> Hi Reece, >> >>> On 3 Jul 2020, at 21:32, Reece R. Pollack <re...@his.com >>> <mailto:re...@his.com>> wrote: >>> >>> Noting that the PCB_BASE_FRAME class is derived from the >>> EDA_DRAW_FRAME class, is it acceptable to assume that the >>> EDA_DRAW_FRAME pointer parameters passed to functions in Pcbnew >>> classes are actually pointers to a PCB_BASE_FRAME? >> >> Yes, but it would be considered bad practice. > > I would argue that it could easily be defined that code in the pcbnew > directory is dealing with a PCB_BASE_FRAME rather than an > EDA_DRAW_FRAME. Common code should not make that assumption, of course, > and would not use anything beyond the base class.
Why not just provide a virtual method in the EDA_DRAW_FRAME and override the method in the derived frame objects to do implement the frame specific behavior? This way you don't have to care about the draw frame type > >>> >>> Specifically: >>> >>> * The UNIT_BINDER class constructor >>> * The classes implementing the GetMsgPanelInfo function >>> * The classes implementing the GetSelectMenuText function >>> >>> The reason I'm asking is that to implement origin transforms these >>> functions need access to the user's display option that chooses the >>> display origin. This needs access to a function defined in the >>> PCB_BASE_FRAME class. I can make that a common function defined in >>> EDU_DRAW_FRAME and overridden in PCB_BASE_FRAME, or the code needs to >>> know that parameter is really a pointer to a PCB_BASE_FRAME object. >>> >> Ask yourself this: is there anything PCB-specific about a settable >> origin? Is there any reason a settable origin wouldn’t also be >> useful, say, a schematic? If the answer is “no”, then you should push >> the origin (along with its getters and setters) down in to EDA_DRAW_FRAME. > > My submission last year was intended to allow any or all of the KiCad > subsystems (eeschema, pcbnew, gerbview, etc.) make use of origin > transforms. Gerbview could benefit, because if you set the place and > drill origin to the lower left of you design when you generate Gerbers, > Gerbview displays the page border below the displayed board. I'd thought > about how to do this in eeschema, considering an origin at any of the > four corners or at the aux origin (which is defined in eeschema but is > not settable). But I did a full implementation on pcbnew because that > was what I felt really needed it.That implementation added a parameter > to the functions I mentioned above, which resulted in the eeschema > functions also having that parameter even though they didn't use it. > > When Wayne sent out his last call for comments before he merged the > changes, Seth complained that since I didn't implement support in > eeschema then these changes should not affect any code in the eeschema > directory. His suggestion was to use function overloading, but that > would result in having two interfaces visible in the both eeschema and > pcbnew, one that worked in that context and one that didn't. You > wouldn't know that the code called the wrong function until you noticed > broken behavior. I don't consider that an acceptable situation just to > avoid a trivial parameter list change. > > My approach this year is that code in the pcbnew directory knows it's > dealing with a PCB_BASE_FRAME because it's PCB-specific code in the > first place. Then it's easy to access the PCB-specific interfaces and > data. Rather than changing the UNIT_BINDER class I've implemented a > PCB_UNIT_BINDER class derived from UNIT_BINDER. It knows that it's > getting a PCB_BASE_FRAME rather than an EDA_DRAW_FRAME, and passes that > to the base class constructor. It's more tricky with the functions that > are called indirectly through the EDA_DRAW_FRAME, such as > GetMsgPanelInfo and GetSelectMenuText. The implementations are specific > to pcbnew and live in the pcbnew directory. > > I can do it with dynamic casts, but I'm an embedded systems guy and I > hate to waste CPU cycles checking things that are defined to be true. > And dynamic_cast is slow: my tests of the Icarus Verilog simulator > appear to show VVP spends about 20% of its CPU cycles just resolving > dynamic casts. > >> If this really is PCB-specific, then you can either push just the >> origin getters/setters down into EDA_DRAW_FRAME and override them in >> PCB_BASE_FRAME, or do a dynamic_cast to PCB_BASE_FRAME and if non-null >> call the getters/setters. > > I believe there will need to be subsystem-specific implementations of > portions of the origin transform code, based on what display options we > offer to the user. However, I think a lot of the interfaces can be > common. I'm willing to do either a PCB-only version or one that has > support for pcbnew and latent support for the other subsystems. What I'm > not willing to do is invest a lot of effort, only for someone to veto it > at the last minute over a stylistic difference of opinion again. We, > meaning myself and the core developers collectively, need to come to a > consensus on how this should be implemented. > > -Reece > > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
