kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Mathias,

On 9/11/2017 3:38 PM, Mathias Grimmberger wrote:
> 
> Hi all,
> 
> 
> you may remember that I complained about the inconsistent behaviour of
> several dialogs some time ago.
> 
> Below is what I have come up with to improve that. The main purpose is
> to improve consistency of dialogs, although it may occasionally simplify
> the code too.
> 
> Basically it is a kind of bracket around what makes up a typical input in
> a dialog, namely a label, a text entry, an optional spin button and an
> optional unit indicator, kind of an extended WX_UNIT_BINDER.

I'm OK with this conceptually although how it works in practice remains
to be seen.  If it simplifies and improves entering coordinates then
that is a good thing.

I have a few comments on your class names.  Please do not use WX_ as a
prefix.  This makes it seem like it's something from wxWidgets.
Typically we use the EDA_ or no prefix.  Also, DIMENSION implies the
distance between two points.  COORDINATE would be a more accurate class
name.

> 
> The idea is that the code for the dialog should only deal with that
> object and use the individual controls only in special circumstances.
> 
> Below is the declaration of WX_DIMENSION_INPUT, this is for inputting
> everything that ends up as a value in internal units. Also attached is
> the code for 2 (simple) dialogs using it to show what that looks like
> (please note that I'm not sure the values used for design rule
> validation are actually the right ones, this is more for demonstration).

Please do *not* handle dialog button commands (wxID_OK, wxID_CANCEL,
etc.) in your dialogs for transferring data from controls.  wxDialog (as
well as DIALOG_SHIM) handles this automatically.  You should override
TransferData{To/From}Window to do this as required.  If you pass the
pointers to dialog member data objects to your validators, you do not
even need to override TransferData{To/From}Window unless you need to
test for combinations of control values that are not valid.

Also, you should *never* call EndModal() or EndQuasiModal() from within
the dialog.  You cannot be sure how the user is going to open the
dialog.  If you don't think this is an issue, open the dialog you have
supplied by calling ShowQuasiModal() and see what happens when you
dismiss it.  I know this poor design choice is used in a lot our dialogs
but that doesn't mean we should continue to add to the problem.

> 
> What WX_DIMENSION_INPUT provides for now:
> 
> - disabling/enabling of all controls belonging to the input as a group
> - setting/getting the input value directly in internal units
> - allowing input of optional unit suffixes (mm, in, mi etc.)
> - validating that a formally correct value was entered (meaning no more
> inputting "mimimi" and getting 0 as value without error message)
> - validating that the value won't overflow internal units limits
> - validating the value against settable minimum/maximum limits (default
>> 0, <= INT_MAX)
> - error popups with consistent error messages if validation fails
> (e.g. "Value of 'Track width' invalid: design rules specify a minimum of
> 0.2 mm."), with the value name automatically derived from the label
> - after the error popup is dismissed the text entry will get focus and
> the text in it will be selected
> - the value can be accessed either by explicit functions or by the
> usual data transfer mechanism of wxWidgets

All of this makes sense to me.  You may want to include Doxygen comments
on how to use these objects to bind the controls so other devs will know
how to use them.

> 
> Similar (but simpler) classes can be created for input of arbitrary
> floating point numbers and for integers.

wxFloatingPointValidator and wxIntegerValidator already exist for this.
There is even a wxNumericPropertyValidator that can be used for numbers
that are not base 10.  If these classes do not provide the exact
behavior you need, please try to derive from them rather than create a
new object if at all possible.  We have enough unnecessary duplicate
functionality in KiCad.

Cheers,

Wayne

> 
> Some more features would need to be added to be able to use this for
> complicated dialogs like the pad properties.
> 
> So, my question is, what do you think? Is it worth continuing in this
> direction (identifiers and other details can be discussed if this ever
> gets to the stage of being considered for inclusion)?
> 
> 
> Have fun,
> 
> MGri
> 
> 
> 
> 
> 
> _______________________________________________
> 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
>