It would be preferable to make it possible to call GetSelectMenuText (or really, any facility that needs access to the current units and coordinate transform) without a frame.
We have recently been putting a lot of effort into separating the logic code from the UI / frame code in KiCad, and it would be a shame to introduce something that ties them together. What about bundling together the units selection and the origin transforms into a single object that can be passed into GetSelectMenuText instead of the EDA_DRAW_FRAME? That would make it easier to write testcases and other standalone chunks of code that can pass in arbitrary units and transforms without having to construct a frame. -Jon On Fri, Jul 10, 2020 at 3:33 PM Reece R. Pollack <re...@his.com> wrote: > > The units selection and the origin transforms are both available through > the EDA_DRAW_FRAME. I haven't looked at every call in detail (there are > a combined 118 calls and function declarations), but it appears that > when the caller has access to an EDA_DRAW_FRAME then the call is some > variant of: > > GetSelectMenuText( m_editFrame->GetUserUnits() ); > > Where there isn't access to an EDA_DRAW_FRAME, it's coded as: > > GetSelectMenuText( EDA_UNITS::MILLIMETRES ); > > It seems to me that if we're going to change the parameters to this > function, we should pass a pointer to an EDA_DRAW_FRAME as a single > parameter. If this parameter is nullptr, then we assume > EDA_UNITS::MILLIMETRES and null transforms, otherwise we use the getter > functions in EDA_DRAW_FRAME to get the correct object or data. > > I'd need to see more than a few nodding heads before I made either of > these changes. > > -Reece > > On 7/10/20 3:00 PM, Jeff Young wrote: > > I agree on both points: units and transform should be saved in the project > > file, and they should be passed to GetSelectMenuText as parameters. > > > > Cheers, > > Jeff. > > > > > >> On 10 Jul 2020, at 19:35, Jon Evans <j...@craftyjon.com> wrote: > >> > >> OK, I see. > >> > >> I have mostly stayed out of this conversation before so I will > >> probably step back, but I would suggest that I think that display > >> units and origin choice should be a project-level setting, not > >> system-level. > >> > >> Probably it makes sense to change GetSelectMenuText so that it has > >> this context available somehow (whether by passing in an > >> EDA_DRAW_FRAME or by just passing in the ORIGIN_TRANSFORMS or > >> whatever) > >> > >> -Jon > >> > >> On Fri, Jul 10, 2020 at 2:29 PM Reece R. Pollack <re...@his.com> wrote: > >>> Jon, > >>> > >>> The alternate origins themselves (Place & Drill / Aux origin, and Grid > >>> origin) in Pcbnew are stored in the BOARD_DESIGN_SETTINGS class and > >>> saved in the board file. Of course, the Page origin location doesn't > >>> need to be stored; it's always (0,0). > >>> > >>> My initial thought last year was to store the user's choice of display > >>> origin in the board file, but after some discussion we reached consensus > >>> that the choice of display origin was more like millimeters/inches and > >>> thus should be a user option rather than a board property. In the > >>> proposed implementation, the user's preference for which origin is used > >>> for display is stored in the PCB_DISPLAY_OPTIONS class and saved in the > >>> pcbnew.json file. I think a case could be made that this should be saved > >>> per-project, but no one has made a good argument for that. > >>> > >>> > >>> The primary user of the display origin transforms is the UNIT_BINDER > >>> class. This class has a pointer to the EDA_DRAW_FRAME object. Since > >>> UNIT_BINDER is common, I added a virtual function > >>> "GetOriginTransforms()" to the EDA_DRAW_FRAME class. This function > >>> returns a reference to an ORIGIN_TRANSFORMS class. The base > >>> implementation of the ORIGIN_TRANSFORMS class contains functions that > >>> return their coordinate arguments unchanged. This way none of the KiCad > >>> applications see a change in behavior unless they override the > >>> GetOriginTransforms() function. > >>> > >>> In the case of Pcbnew, the EDA_DRAW_FRAME parameter is actually a > >>> pointer to a PCB_BASE_FRAME object. In this class, the > >>> GetOriginTransforms() function is overridden and returns a reference to > >>> a PCB_ORIGIN_TRANSFORMS object. This object's functions know how to > >>> access the BOARD_DESIGN_SETTINGS and PCB_DISPLAY_OPTIONS objects through > >>> the PCB_BASE_FRAME object and perform the needed transformations. > >>> > >>> This works for everything that can find the EDA_DRAW_FRAME object, like > >>> UNIT_BINDER. The GetMsgPanelInfo functions take an EDA_DRAW_FRAME > >>> pointer as a parameter, so this isn't a problem. However, the > >>> GetSelectMenuText() functions take only an EDA_UNITS parameter. Thus my > >>> problem. > >>> > >>> -Reece > >>> > >>> On 7/10/20 1:25 PM, Jon Evans wrote: > >>>> Shouldn't the origin be stored in the design data (BOARD / SCHEMATIC) > >>>> rather than the base frame? > >>>> > >>>> It seems like all data about objects, including their coordinates in > >>>> the coordinate space defined by the user, should be available just by > >>>> parsing a board or schematic file (which should be independent of the > >>>> EDA_BASE_FRAME) > >>>> > >>>> -Jon > >>>> > >>>> On Fri, Jul 10, 2020 at 1:18 PM Reece R. Pollack <re...@his.com> wrote: > >>>>> Jeff, > >>>>> > >>>>> Thanks for the follow-up. > >>>>> > >>>>> I'm finding some of the GetSelectMenuText() implementations format > >>>>> coordinate addresses for display. That means they need to use display > >>>>> origin transforms so the displayed coordinate matches what the user sees > >>>>> on the status line and elsewhere. However, there does not appear to be a > >>>>> path from these functions to the EDA_BASE_FRAME class where these > >>>>> transforms are currently available. > >>>>> > >>>>> Might someone more familiar with the data structures and calling > >>>>> sequences could suggest how this can best be accomplished? > >>>>> > >>>>> Seth, I'd appreciate it if you could bring your knowledge and expertise > >>>>> to bear. > >>>>> > >>>>> -Reece > >>>>> > >>>>> On 7/10/20 6:35 AM, Jeff Young wrote: > >>>>>> No, the DRC re-write won’t affect GetSelectMenuText(). It originated > >>>>>> for describing items in the Clarify Selection menu, but it’s now used > >>>>>> whenever we want to describe an item to the user. > >>>>>> > >>>>>>> On 10 Jul 2020, at 00:51, Reece R. Pollack <re...@his.com> wrote: > >>>>>>> > >>>>>>> On 7/9/20 7:09 PM, Tomasz Wlostowski wrote: > >>>>>>>> On 10/07/2020 00:58, Reece R. Pollack wrote: > >>>>>>>>> I'm looking at display origin transformations for DRC reports. > >>>>>>>>> > >>>>>>>>> With the 5.1.x branch Pcbnew, the DRC report dialog box message > >>>>>>>>> texts > >>>>>>>>> contained the Cartesian coordinates of each flagged item. It appears > >>>>>>>>> that the 5.99 branch no longer displays the coordinates of DRC > >>>>>>>>> items. > >>>>>>>>> However, the Cartesian coordinates are still listed in the report > >>>>>>>>> file. > >>>>>>>>> Unlike the display in a dialog box, this report is persistent and > >>>>>>>>> could > >>>>>>>>> be passed to someone using different display origin settings. > >>>>>>>>> > >>>>>>>>> 1. Should these coordinates be reported relative to the page > >>>>>>>>> origin, or > >>>>>>>>> transformed per the user-selected origin and axis directions? > >>>>>>>>> 2. If the coordinates are transformed, should the report include > >>>>>>>>> the > >>>>>>>>> user settings? > >>>>>>>>> > >>>>>>>>> -Reece > >>>>>>>>> > >>>>>>>> Reece, > >>>>>>>> > >>>>>>>> I wouldn't introduce any changes to the current DRC code, we're > >>>>>>>> designing a new DRC engine for KiCad V6 and many things in DRC > >>>>>>>> interface > >>>>>>>> will likely change. > >>>>>>>> > >>>>>>>> IMHO the DRC coordinate transform belongs to the UI, not the DRC > >>>>>>>> itself: > >>>>>>>> - the DRC engine generates an internal report with coordinates in > >>>>>>>> board > >>>>>>>> coordinate space > >>>>>>>> - whatever displays the report to the user (i.e. the DRC dialog) > >>>>>>>> converts the board coordinates to UI coordinates. > >>>>>>>> > >>>>>>>> - my 5 cents > >>>>>>>> Tom > >>>>>>>> > >>>>>>>> > >>>>>>> Tom, > >>>>>>> > >>>>>>> I'm not hard to convince, especially when it means doing less work. > >>>>>>> > >>>>>>> This sounds like the RC_ITEM::ShowCoord function will be removed or > >>>>>>> replaced. It's one of the two troublesome function groups. > >>>>>>> > >>>>>>> The other troublesome function group is the GetSelectMenuText > >>>>>>> function. Last year I knew what they did but I've forgotten in the > >>>>>>> interim. Will this DRC rewrite replace these? > >>>>>>> > >>>>>>> -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 > _______________________________________________ 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
