From ec07635801e2c980f8abe62b514b8adc19a4a548 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 20 Jan 2020 13:16:41 +0100 Subject: [PATCH] Don't recurse upwards when updating pending focus in wxMSW This is unnecessary, we only need to update the pending focus in the immediate parent window to prevent a wrong radio button from being focused (and hence selected) when it regains focus, there is no good reason at all to interfere with the focus in the grandparent (and higher) windows. Doing this was not only useless, but actually harmful, as it overrode explicit calls to SetFocus() in the user code, so just stop doing it. This also allows to avoid having 2 functions related to this and keep just a single virtual WXSetPendingFocus() one. Closes #18653. --- include/wx/containr.h | 2 +- include/wx/msw/toplevel.h | 2 +- include/wx/msw/window.h | 11 +++++------ src/msw/radiobut.cpp | 28 +++++++++++++++++++++++----- src/msw/window.cpp | 19 ------------------- 5 files changed, 30 insertions(+), 32 deletions(-) diff --git a/include/wx/containr.h b/include/wx/containr.h index aa5ffd625c..1c32c01f6d 100644 --- a/include/wx/containr.h +++ b/include/wx/containr.h @@ -265,7 +265,7 @@ public: } WXDLLIMPEXP_INLINE_CORE - virtual void WXDoUpdatePendingFocus(wxWindow* win) wxOVERRIDE + virtual void WXSetPendingFocus(wxWindow* win) wxOVERRIDE { return m_container.SetLastFocus(win); } diff --git a/include/wx/msw/toplevel.h b/include/wx/msw/toplevel.h index cf6356754a..d32764bb1f 100644 --- a/include/wx/msw/toplevel.h +++ b/include/wx/msw/toplevel.h @@ -103,7 +103,7 @@ public: // called from wxWidgets code itself only when the pending focus, i.e. the // element which should get focus when this TLW is activated again, changes - virtual void WXDoUpdatePendingFocus(wxWindow* win) wxOVERRIDE + virtual void WXSetPendingFocus(wxWindow* win) wxOVERRIDE { m_winLastFocused = win; } diff --git a/include/wx/msw/window.h b/include/wx/msw/window.h index 2ee4558b94..230d099b27 100644 --- a/include/wx/msw/window.h +++ b/include/wx/msw/window.h @@ -585,12 +585,11 @@ public: static bool MSWClickButtonIfPossible(wxButton* btn); // This method is used for handling wxRadioButton-related complications, - // see wxRadioButton::SetValue(). It calls WXDoUpdatePendingFocus() for - // this window and all its parents up to the enclosing TLW, recursively. - void WXSetPendingFocus(wxWindow* win); - - // Should be overridden by all classes storing the "last focused" window. - virtual void WXDoUpdatePendingFocus(wxWindow* WXUNUSED(win)) {} + // see wxRadioButton::SetValue(). + // + // It should be overridden by all classes storing the "last focused" + // window to avoid focusing an unset radio button when regaining focus. + virtual void WXSetPendingFocus(wxWindow* WXUNUSED(win)) {} // Called from WM_DPICHANGED handler for all windows to let them update // any sizes and fonts used internally when the DPI changes and generate diff --git a/src/msw/radiobut.cpp b/src/msw/radiobut.cpp index ef3d7ec15f..513e734edb 100644 --- a/src/msw/radiobut.cpp +++ b/src/msw/radiobut.cpp @@ -181,11 +181,29 @@ void wxRadioButton::SetValue(bool value) } else { - // Don't change focus right now, this could be unexpected, but do - // ensure that when our parent regains focus, it goes to this button - // and not another one, which would result in this one losing its - // checked status. - GetParent()->WXSetPendingFocus(this); + // We don't need to change the focus right now, so avoid doing it as it + // could be unexpected (and also would result in focus set/kill events + // that wouldn't be generated under the other platforms). + if ( !GetParent()->IsDescendant(FindFocus()) ) + { + // However tell the parent to focus this radio button when it + // regains the focus the next time, to avoid focusing another one + // and this changing the value. Note that this is not ideal: it + // would be better to only change pending focus if it currently + // points to a radio button, but this is much simpler to do, as + // otherwise we'd need to not only check if the pending focus is a + // radio button, but also if we'd give the focus to a radio button + // when there is no current pending focus, so don't bother with it + // until somebody really complains about it being a problem in + // practice. + GetParent()->WXSetPendingFocus(this); + } + //else: don't overwrite the pending focus if the window has the focus + // currently, as this would make its state inconsistent and would be + // useless anyhow, as the problem we're trying to solve here won't + // happen in this case (we know that our focused sibling is not a radio + // button in the same group, otherwise we would have set shouldSetFocus + // to true above). } } diff --git a/src/msw/window.cpp b/src/msw/window.cpp index 957b185bd7..f207a8afd7 100644 --- a/src/msw/window.cpp +++ b/src/msw/window.cpp @@ -1024,25 +1024,6 @@ void wxWindowMSW::MSWUpdateUIState(int action, int state) ::SendMessage(GetHwnd(), WM_CHANGEUISTATE, MAKEWPARAM(action, state), 0); } -void wxWindowMSW::WXSetPendingFocus(wxWindow* win) -{ - wxWindow * const focus = FindFocus(); - - for ( wxWindow* parent = this; parent; parent = parent->GetParent() ) - { - // We shouldn't overwrite the pending focus if the window has the focus - // currently, as this would make its state inconsistent. And this would - // be useless anyhow, as we only remember pending focus in order to - // restore it properly when the window gets the focus back -- which is - // unnecessary if it has the focus already. - if ( !parent->IsDescendant(focus) ) - parent->WXDoUpdatePendingFocus(win); - - if ( parent->IsTopLevel() ) - break; - } -} - // --------------------------------------------------------------------------- // scrolling stuff // ---------------------------------------------------------------------------