kicad-developers team mailing list archive

[Seth Hillbrand <seth@xxxxxxxxxxxxx>]

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!

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?

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.

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.

Best-
Seth

On 2019-05-25 22:57, Seth Hillbrand wrote:
> Hi Reece-
>
> I've had a chance to test this a bit.  It works really nicely.  Thanks
> for the good idea here.
>
> I only noticed one spot where it wasn't transformed so far: the
> Measurement tool.  When used, it displays a sign with the distance,
> which doesn't match the increasing/decreasing convention.
>
> The second part is mostly a question.  Where do I set this in Eeschema
> and page layout?  The setting is in the panel under pcbnew, so I would
> assume it is a per-application process.  However, there is no other
> application that has that panel for setting.
>
> I don't have a strong opinion on whether this should be a KiCad-wide
> preference or not.  I can't imagine someone wanting to set it one way
> in Eeschema and another in pcbnew.  If that was your intention, the
> panel should be at the top level rather than under pcbnew.  If it
> wasn't, can you give some more insight into why it would be good to
> split between the applications?
>
> Lastly, and this is a bit fundamental, I have reservations about
> passing this parameter around when it is not needed.  This is more of
> a C-style convention.  Where functions inherit the frame with the
> preference, that should be used by a Get() method rather than passing
> down in a parameter chain.
>
> In some cases (UNIT_HELPER), this should either be incorporated into
> UNIT_HELPER or written as a class that inherits UNIT_HELPER.  The
> class then gets the current setting (as Unit helper does with the
> units) and applies it in one place only.
>
> The problem with the current solution is that it becomes very
> difficult to maintain as the parameter count increases.
>
> Overall, this is an excellent piece of work.  I look forward to using
> it and flipping my perspective around. :)
>
> Best-
> Seth
>
>
>
> On 2019-05-25 09:08, Reece R. Pollack wrote:
> > The Zip file attachment contains the complete set of patches
> > implementing Display Origin Transforms, now squashed and rebased for
> > your merging pleasure!
> >
> > They should apply cleanly atop this commit from JP Charras:
> >
> > > b8e2054 Activate context menu in LIB_VIEW_FRAME canvas.
> >  Folks, please resist the urge to commit another 110 patches before
> > Wayne has a chance to merge these onto master. :-)
> >
> > -Reece
> > _______________________________________________
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp