kicad-developers team mailing list archive

[Reece Pollack <reece@xxxxxxx>]

----- On May 26, 2019, at 10:53 AM, Seth Hillbrand <seth@xxxxxxxxxxxxx> wrote: 

> Hi Reece-

> Apologies for the double-tap here. I see I miss-read your intention in
> a couple of places! Sorry about that. I'll try to clean up my comments
> below.

> I also noticed a few other bits and knobs that would be good to clean in
> the final patchset. Let me know if you'd like a hand with any of the
> suggestions!

Suggestions are always good. The more eyes on this now, the fewer complaints we'll have when end-users get it. 

> 1) I see that this is intended to only be pcbnew. I was thrown by the
> changes to GetSelectMenuText() in the other applications but if I'm
> reading it correctly, that is groundwork for future patches, correct?

I'd originally intended to limit the changes to pcbnew. Then it was suggested that the Footprint editor really should get the same treatment, followed by people such as yourself saying "what about eeschema?" And some things are common across different apps in ways I did not expect; some of the pcbnew DRC code is used by the eeschema ERC. 

GetMsgPanelInfo() and GetSelectMenuText() are declared in EDA_ITEM. To ensure consistency across all uses I changed the declarations in EDA_ITEM, which required the change to ripple throughout all applications. 

I've already started promising follow-up patches to the Footprint editor. GerbView needs them too. If we can agree on look-and-feel, I suspect eeschema and the symbol editor will follow. I do NOT want to hold up these patches waiting on other apps or we'll never break the continuous rebase cycle. 

> 2) The headers in your new files in 0004 seem to keep the copyrights
> from the original files. The copyright on the file itself should not
> extend backwards from the creation date, so any files you create should
> just be (C) 2019.

> 3) Patch 0011 has tabs instead of spaces in a couple places.

Oh frell. If Wayne doesn't chime in saying he's about to merge this patch set I'll probably squash these fixes and issue yet another full patch set. 

> 4) In UNIT_BINDER (not UNIT_HELPER -- my mistake below), you pass the
> transform as the last parameter but the data is stored in
> PCB_BASE_FRAME, which is also passed as the first parameter in pcbnew.
> I think we should get this in a similar manner to m_eval in the
> UNIT_BINDER constructor. This would require moving the base definition
> of the origin transform class to EDA_DRAW_FRAME from PCB_BASE_FRAME.
> But I don't think that there's anything pcb-specific about offset/sign
> transform, so moving it to the shared class should not be problematic.

I think I addressed in my previous email why each instantiation of UNIT_BINDER needs to be given a parameter identifying the required transform. 

Originally, the origin transform classes had specific knowledge of how to fetch the user-selected origin. I expected this to be different for eeschema than for pcbnew, as the eeschema implementation might need to calculate the coordinates of the user-selected origin corner, while the pcbnew implementation chooses from page, aux, or grid origins. Much later I factored out this code and put it in PCB_BASE_FRAME. With that done what remains as pcbnew-specific in PCB_ORIGIN_TRANSFORMS is the axis inversion selections. 

It's still handy in certain circumstances to have null origin transform objects. Specifically, there are places in the code where diagnostic data is reported in millimeters, not subject to user-selected units conversion. I've intentionally chosen to use null origin transforms in these places so the diagnostic data isn't subject to user-selected origin transforms either. 

-Reece