Callers of the form:
GetSelectMenuText( EDA_UNITS::MILLIMETRES );
are most likely test cases.
Dialogs also know what the user units are, so you can get it from there too if
you don’t have a frame.
But yes, I spent a lot of time divorcing GetSelectMenuText() from global
variables and the frame; I’d hate to see it lost.
On the topic Jon and Ian were discussing (one parameter or two) I don’t have
strong feelings.
Cheers,
Jeff.
> On 10 Jul 2020, at 21:30, Jon Evans <j...@craftyjon.com> wrote:
>
> The way I see it, there is one package of information that we should
> be delivering: how to turn internal units into real text for display.
> That information includes both a units selection and whatever
> transform to apply in case there are coordinates involved.
>
> I see no benefit into splitting that into two arguments.
>
> On Fri, Jul 10, 2020 at 4:27 PM Ian McInerney <ian.s.mciner...@ieee.org>
> wrote:
>>
>> Doesn't the origin transform operate on the IU and not use the units
>> directly (so the units are only needed by the actual menu text generation
>> function)? If they are separate, then we should pass them as two individual
>> arguments to the GetSelectMenuText function.
>>
>> -Ian
>>
>> On Fri, Jul 10, 2020 at 8:40 PM Jon Evans <j...@craftyjon.com> wrote:
>>>
>>> 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
>
> _______________________________________________
> 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
