kicad-developers team mailing list archive

[Oliver Walters <oliver.henry.walters@xxxxxxxxx>]

On Thu, Sep 15, 2016 at 6:19 AM, Nick Østergaard <oe.nick@xxxxxxxxx> wrote:

> 2016-09-14 14:09 GMT+02:00 Oliver Walters <oliver.henry.walters@xxxxxxxxx
> >:
> > Hi all,
> >
> > First time submitting a patch, so here goes....
> >
>
> Welcome. Please be aware that your patches contain tabs, as per the
> coding style policy; the indentation level for the KiCad source code
> is defined as four spaces. Please do not use tabs.


Ok, will fix.


>
> > The attached patch deals with a number of issues with regards to the
> > footprint wizards manager. It started off as what I imagined was a fairly
> > simple task to improve the UX of the FP wizards interface, but it evolved
> > into something a bit more complex as I delved deeper into the source!
> >
>
> Lovely to see some improments. I think there are great improvements in
> here and I have tried to add some more comments inline.
>
> > Improvements are as follows:
> >
> > 1. I have done away with the use of a leading asterisk to designate the
> > "units" of a wizard parameter. Multiple parameter types can now be
> defined
> > (integer, float, mm, mils, bool, etc..)
> >
> > 2. Input validation. Each type of parameter is now validated properly
> within
> > the wizards screen. Integer parameters can only be set to integers,
> > dimensions can only be floating point, etc.
> >
> > 3. Boolean values are now treated properly. Simply click the cell to
> toggle,
> > rather than awkwardly typing "True" or "False"
> >
>
> I this this works a bit strange right now because it switches between
> showing the True/False text, but when clicking it will show the
> checkbox. Why this switching between content?
>
> To make it easer than manually typing true or false as before I would
> have expected either a combobox or a checkbox that is constantly
> there.


It looks like using a wxGridCellBoolRenderer will fix this. I'll look into
this.


> > 4. Multiple choice options available - If the python script specifies a
> list
> > of options, then a drop-down box will be displayed for that parameter
> >
>
> Is there an example script that uses lists? I could not find any of
> the default wizards proviing this option.


I'll add an example script.


>
> > 5. Param designators. Instead of the row numbers being shown, each param
> can
> > optionally be assigned a designator (such as 'e' for pitch) which will be
> > show to the left of that row.
> >
>
> I think this makes sense.
>
> It would be cool if we in the future could render dimension arrows on
> the plot for some things using those references.
>

That is a very cool idea. I had been planning on implementing the
GetImage() method which would require a dimensional drawing for each
wizard. However, with the ability to draw on the screen, you could easily
show the dimensions only for the current page of parameters, which would
de-clutter it a lot. I'm not sure how much work this would be, though.


>
> > 6. "Reset to default" - A new button in the top toolbar which resets all
> > wizard parameters to their default values
> >
> >
> > 7. More logical parameter checking within the python wizard helpers.
> > Currently each parameter needs to be explicitly checked e.g. "CheckInt"
> to
> > make sure the value is a valid integer. This patch defines a Parameter
> > holder class that automatically checks values based on their specified
> type.
> > Additionally, parameters can have other checks specified when they are
> > added, e.g:
> >
> > AddParam("Pads","width",uMM, 2.5, min_value=0.1, max_value=5.5)
> >
> > 8. Fixed script import errors in the case of "bad" scripts. Currently if
> a
> > wizard contains any errors, the LoadPlugins functions fail and no
> subsequent
> > wizards are loaded. A simple try-except block has been added around the
> > specific file loading so that one bad plugin file does not break the
> others
> >
> > 9. Auto-sizing of the parameter grid. Not a huge deal but the grid width
> now
> > expands to fill the available space.
> >
> > General Notes:
> >
> > a) To provide the functionality for multiple unit types I have had to
> > slightly break compatibility with the way that parameter types were
> defined
> > in the current wizards. However, I have designed the new Parameter class
> (in
> > kicadplugins.i) to be as close-to-compatible as it can be.
> >
>
> Maybe some notes are required on how to update the older scripts to
> ease a transition.
>
> > I have updated each of the default plugins to be compatible with the new
> > system. Only minor changes were required.
> >
> > b) I have also consolidated the helper classes
> > (HelpfulFootprintWizardPlugin) into the simpler FootprintWizard base
> class.
> >
> > c) There is now a GitHub repository for FootprintWizards -
> > https://github.com/KiCad/Footprint_Wizards - should this patch be
> accepted I
> > propose that the default wizards be further improved, and removed from
> the
> > source files. Instead, provide a link to the GitHub page or a
> > download-helper for the scripts. This way the community can contribute
> > quality wizards, and I shall endeavour to add some good documentation to
> the
> > wiki page for wizard creation.
> >
> > .diff is attached - hopefully this is the right way of doing this?
> >
>
> Patches that can be applied with git am is good. In this case it seems
> like all newlines in commit comments are stripped which is not so
> good.
>

Any suggestions on how I could fix this? Should I be submitting one patch
per commit, or a single patch as I have done here? Also, can I submit a
pull-request straight from git or is the patch-email method preferred?


>
> I wonder why you make a construct like this in 0001.
> #if defined( __MINGW32__ )
>     path = A
> #elif defined( __WXMAC__ )
>     path = B
> #else
>     path = A
> #endif
>
> Why not like this?
> #if defined( __WXMAC__ )
>     path = B
> #else
>     path = A
> #endif


I can simplify this as above.


>
> > Please let me know what I can do to help this process along.
> >
> > Regards,
> >
> > Oliver
> >
> >
> > _______________________________________________
> > 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
> >
>