← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] Hotkey list dialog

 

Hi Wayne,

In wxSizer, there is an option to use the "default border", which is
preferred to hardcoding a pixel value. This is preferred
apparently[1]. The WX method to do this,
wxSizerFlags::GetDefaultBorder(), is here.[2] (GNOME-HIG compliant 6px
on GTK, 5px on OSX, scaled otherwise).

If we stick with a hardcoded value (as all the wxFB stuff cannot use
the DPI-aware defaults, it just specifies '5' on all platforms and
displays), setting the wxSizerFlags is not very concise:

     wxSizerFlags flags = KIUI::GetStdSizerFlags(); // this can't set
a border as we don't know the directions yet

     sizer->Add( widget1, flags.Border( wxLEFT | wxTOP, 5 ) ); //
still need to feed the '5' in
     sizer->Add( widget2, flags.Border( wxRIGHT | wxTOP, 5 ) ); //
can't reuse the '5', have to call Border() again with new directions

Because you can't "pre-set" the "5", a putative
KIUI::GetStdSizerFlags() can't actually help us to avoid specifying
"5" for every subsequent use, and we'd still need a way to get the
"5". What would you like the API for this to look like?

Regardless of this, the first 3 commits stand alone, and this is not
required for them to be perfectly functional.

Cheers,

John

[1] https://docs.wxwidgets.org/3.1/classwx_sizer_flags.html#a2dc000b863b8580ae3f183c0e11c9944
[2] https://github.com/wxWidgets/wxWidgets/blob/master/include/wx/sizer.h#L107
On Wed, Oct 3, 2018 at 6:53 PM John Beard <john.j.beard@xxxxxxxxx> wrote:
>
> Hi Wayne,
>
> I know the policy is not to convert wxFB dialogs, but in this case, I
> don't see how that dialog could be written neatly in wxFB without
> having the derived class tear half of the components down again if
> m_read_only is true. The alternative is to duplicate the entire
> dialog, which is a common tactic with making similar dialogs wxFB, but
> it's a shame to multiply code when you could divide it!
>
> Especially if you use a helper like the button row widget, you'd have
> two placeholders, so the wxFB project would be essentially a blank
> dialog with a single filter box at the top, and you'd have to have two
> workarounds like InstallOnPanel to place the widgets you actually
> wanted onto them.
>
> Re the common UI thing - that makes sense, I had forgotten about
> wxSizerFlags, even though I have done something similar in the past!
> Patch coming up.
>
> Cheers,
>
> John
>
> On Wed, Oct 3, 2018 at 6:40 PM Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
> >
> > John,
> >
> > Your patch set looks good and works as advertised.  I have a few
> > nit-pick comments.
> >
> > I personally prefer making the hotkey list panel hard coded as you did
> > rather than using wxFB but not all developers share my preference in
> > this regard.  Since this makes the hotkey list panel more reusable, I'm
> > hoping no one will protest too loudly.
> >
> > Your common UI code (patch 4) probably should include support for a
> > wxSizerFlag object for the default border.  You might want to consider
> > using term border instead of margin to prevent confusion as that is how
> > it is defined in the wxwidgets documentation.  I'm fine either way.
> >
> > I will merge your if there are not objections.  You can just create
> > another patch.
> >
> > Cheers,
> >
> > Wayne
> >
> > On 10/3/2018 11:20 AM, John Beard wrote:
> > > Hi,
> > >
> > > Here is a patch sequence for using the hotkey editor widget in a
> > > read-only mode to provide a filterable (and slightly prettier) hotkey
> > > list. This is a 5.1 milestone.
> > >
> > > Major details other than the main aim of the list widget:
> > >
> > > * There are some "common UI" elements introduced here that should be
> > > generally re-usable:
> > > ** BUTTON_ROW_PANEL provides an easy way to construct a row of buttons
> > > spaced out nicely
> > > ** A place to put "generic" UI stuff like the 5px constant very often
> > > used for margins
> > > * The editor/list panel is no longer a wxFB project, as I couldn't get
> > > it to play nice with optional elements: undoing half the construction
> > > in the derived class, unbinding events and changing various stuff in
> > > response to editabilty was not very tidy. It's substantially less code
> > > now, partly due to the re-usable widgets above.
> > >
> > > Also adds tooltips for the HK editor buttons via the new BUTTON_ROW_PANEL class.
> > >
> > > Cheers,
> > >
> > > John
> > >
> > >
> > > _______________________________________________
> > > 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


Follow ups

References