kicad-developers team mailing list archive

[Nick Østergaard <oe.nick@xxxxxxxxx>]

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.

> 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.

> 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.

> 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.

> 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.

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

> 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
>