Cancelling some dialogs with invalid values fails on GTK and MSW

[eelik]

See https://bugs.launchpad.net/kicad/+bug/1800711. Quote from there: "Some dialogs check that a value is within bounds and give warning dialog [...]It's not possible to cancel that kind of dialog without first giving some valid value."

This still happens with Text Properties dialogs, for example non-footprint text and footprint value field text.

Application: kicad
Version: (6.0.0-rc1-dev-1238-gae6989f3a), release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.61.1 OpenSSL/1.1.1 (WinSSL) zlib/1.2.11 brotli/1.0.6 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) nghttp2/1.34.0
Platform: Windows 7 (build 7601, Service Pack 1), 64-bit edition, 64 bit, Little endian, wxMSW
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8)
    Boost: 1.68.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.61.1
    Compiler: GCC 8.2.0 with C++ ABI 1013

Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=OFF
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=OFF
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

[Jeff Young (jeyjey)]

@eelik, this doesn't reproduce on OSX, but I have a suspicion I know what's going wrong on MSW. If you get a chance please test this new version and see if it's fixed.

[KiCad Janitor (kicad-janitor)]

Fixed in revision 72f17ad7f0146647d5cd53d7ec3be621e2feefea
https://git.launchpad.net/kicad/patch/?id=72f17ad7f0146647d5cd53d7ec3be621e2feefea

[eelik (eelik)]

I tested on Linux and it still happens even after the latest commit. I noticed that closing the dialog window with titlebar X succeeds but Cancel makes that problem. What's worse, this happens also with some other dialogs, at least Track & Via Properties.

I can test Win nightly builds later.

Application: kicad
Version: (6.0.0-rc1-dev-1284-g72f17ad7f-dirty), debug build
Libraries:
    wxWidgets 3.0.3
Platform: Linux 4.13.0-46-generic x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.3 (wchar_t,wx containers,compatible with 2.8) GTK+ 2.24
    Boost: 1.62.0
    Compiler: GCC 7.2.0 with C++ ABI 1011

Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=OFF
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=OFF
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=OFF
    KICAD_USE_OCE=OFF
    KICAD_USE_OCC=OFF
    KICAD_SPICE=OFF

[eelik (eelik)]

Also with Footprint Properties -> Pad clearance = -1

[Jeff Young (jeyjey)]

The issue is with the way the window manager hands out KillFocus events to UNIT_BINDER controls (which is pretty much anything with a unit).

[Jeff Young (jeyjey)]

@Devs, the code that attempts to catch this case is UNIT_BINDER::onKillFocus(). Could someone put a breakpoint there and see why it's not working on GTK & MSW? Thanks!

[John Beard (john-j-beard)]

Here is a stacktrace of hitting it on GTK+ (3). I haven't got time right now to debug it, but in case it's useful on its own, here it is!

This is in the text tool, with width set to 0mm.

Also, this behaviour is particularly unfriendly when you have focus-follow-mouse enabled, as if you have 0 in a text box, if you move the mouse out of the dialog, it loses focus and throws the alert box. If you now mouse out and then click on the dialog again, the alert is pushed behind the dialog, and you can't do anything until you move the dialog out of the way, dismiss the alert and correct the field. Any stray out of the dialog in the meantime throws the dialog again.

[John Beard (john-j-beard)]

Here's a video of focus-follow-mouse misbehaving.

Regardless of this WM setting, perhaps the alert should not be a modal alert, but maybe outline the field in red and add an alert icon with a tooltip (and/or more explanation available on click)?

A modal dialog is suitable when the workflow cannot proceed (in this case, when the user clicks OK but a field is invalid). It is an impediment elsewhere - focus loss does not mean the user has to be forcibly context-switched *right now*. An experienced user would already realise the issue just from there being an indicator: forcing a modal dialog to navigate is not very friendly after the first few times!

Furthermore, an "invalid" indicator can be updated on text change, so the user can see there is an issue before even attempting to leave the control.

Think of the difference between a password field that tells you your password is unacceptable before you enter it twice and click submit vs one that dumps you back on the form and tells you to fix something, then lets you scan the page for the error.

[Jeff Young (jeyjey)]

@John, thanks. It is indeed getting a KillFocus when I thought it was. I've got code in there that attempts to see if the object getting focus is the Cancel button, but for some reason it's not working. (I can't test it on OSX because it doesn't generate a KillFocus at all on Cancel.)

The follow-focus stuff is interesting. I expect we want to turn off validation in that case. Anyone know if there's a cross-platform way to ask wxWidgets if follow-focus is on?

[John Beard (john-j-beard)]

This is on 8ca0fe672, by the way.

[eelik (eelik)]

Here, in Kubuntu 17.10, the Warning window is also the topmost of _all_ the windows in my desktop. It must be dismissed before doing anything else, in practice even with other applications.

John's idea of a non-intrusive alert is very good although it doesn't belong under this bug.

[John Beard (john-j-beard)]

@Jeff, I don't think you can tell you're in a focus-follows-mouse environment - that's just the WM changing window activation based on mouse position. I doubt the "victim" windows know what's happening other than just losing focus (as if the user alt-tabbed).

[jean-pierre charras (jp-charras)]

@Jeff,
Your commit 72f17ad7f fixes this issue on MSW.

On Gtk, aEvent.GetWindow() returns always nullptr, and Validate() is always called.

[Jeff Young (jeyjey)]

Right, as discussed on the dev email-list, I'm pulling the plug on kill-focus-based-validation. We'll have to wait for wxWidgets to provide a non-intrusive version.