From 97e217e175c0c4929e88efb3eed9ef56a52757ca Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 22 Jul 2018 19:12:17 +0200 Subject: [PATCH 1/6] Ensure that wxDatePickerCtrlGeneric does get focus events Revert 1de107c03706de5e1864f7e167d29eea93df066f and remove the focus event handler which duplicated and interfered with the handler inherited from this class to make sure that we get wxEVT_{SET,KILL}_FOCUS for the objects of this class. Unfortunately these events now come in pairs, due to an extra artificial event generated by wxComboBoxExtraInputHandler::OnFocus(), which is still wrong -- but arguably less wrong and more useful than not sending them at all. --- include/wx/generic/datectrl.h | 3 +-- src/generic/datectlg.cpp | 9 --------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/include/wx/generic/datectrl.h b/include/wx/generic/datectrl.h index 09d340739d..4276ff853a 100644 --- a/include/wx/generic/datectrl.h +++ b/include/wx/generic/datectrl.h @@ -20,7 +20,7 @@ class WXDLLIMPEXP_FWD_ADV wxCalendarCtrl; class WXDLLIMPEXP_FWD_ADV wxCalendarComboPopup; class WXDLLIMPEXP_ADV wxDatePickerCtrlGeneric - : public wxCompositeWindowSettersOnly< wxNavigationEnabled > + : public wxCompositeWindow< wxNavigationEnabled > { public: // creating the control @@ -80,7 +80,6 @@ private: void OnText(wxCommandEvent &event); void OnSize(wxSizeEvent& event); - void OnFocus(wxFocusEvent& event); #ifdef __WXOSX_COCOA__ virtual void OSXGenerateEvent(const wxDateTime& WXUNUSED(dt)) wxOVERRIDE { } diff --git a/src/generic/datectlg.cpp b/src/generic/datectlg.cpp index 767a7454df..d5abccfd3a 100644 --- a/src/generic/datectlg.cpp +++ b/src/generic/datectlg.cpp @@ -296,7 +296,6 @@ wxEND_EVENT_TABLE() wxBEGIN_EVENT_TABLE(wxDatePickerCtrlGeneric, wxDatePickerCtrlBase) EVT_TEXT(wxID_ANY, wxDatePickerCtrlGeneric::OnText) EVT_SIZE(wxDatePickerCtrlGeneric::OnSize) - EVT_SET_FOCUS(wxDatePickerCtrlGeneric::OnFocus) wxEND_EVENT_TABLE() #ifndef wxHAS_NATIVE_DATEPICKCTRL @@ -485,12 +484,4 @@ void wxDatePickerCtrlGeneric::OnText(wxCommandEvent &ev) m_popup->SendDateEvent(dt); } - -void wxDatePickerCtrlGeneric::OnFocus(wxFocusEvent& WXUNUSED(event)) -{ - m_combo->SetFocus(); -} - - #endif // wxUSE_DATEPICKCTRL - From 06d2b836a06a6b541fde8f7ada0af31cc6061e22 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 22 Jul 2018 23:41:53 +0200 Subject: [PATCH 2/6] Bind focus events to direct children only in wxCompositeWindow Previously, wxEVT_{SET,KILL}_FOCUS event handlers were connected to all children, even the indirect ones, which resulted in duplicate focus events if a grandchildren forward its focus event to its parent window, as happens e.g. with wxComboCtrl, which contains a text control that forwards its focus events to the containing control itself. This change seems globally sound, even if it might break some controls whose children don't get the expected focus events themselves, because the right thing to do is to fix those children. And it fixes duplicate focus events in wxDatePickerCtrlGeneric. --- include/wx/compositewin.h | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/include/wx/compositewin.h b/include/wx/compositewin.h index 8483414686..23ee6f738b 100644 --- a/include/wx/compositewin.h +++ b/include/wx/compositewin.h @@ -186,8 +186,21 @@ private: // support) to hook into its event processing. wxWindow *child = event.GetWindow(); - if ( child == this ) - return; // not a child, we don't want to bind to ourselves + + // Check that it's one of our children: it could also be this window + // itself (for which we don't need to handle focus at all) or one of + // its grandchildren and we don't want to bind to those as child + // controls are supposed to be well-behaved and get their own focus + // event if any of their children get focus anyhow, so binding to them + // would only result in duplicate events. + // + // Notice that we can't use GetCompositeWindowParts() here because the + // member variables that are typically used in its implementation in + // the derived classes would typically not be initialized yet, as this + // event is generated by "m_child = new wxChildControl(this, ...)" code + // before "m_child" is assigned. + if ( child->GetParent() != this ) + return; child->Bind(wxEVT_SET_FOCUS, &wxCompositeWindow::OnSetFocus, this); From fb71adab047a5285c6a94e4f59a95baa51dcfbae Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 23 Jul 2018 00:09:25 +0200 Subject: [PATCH 3/6] Add a unit test for wxDatePickerCtrl focus events Unfortunately it doesn't work with wxGTK, as usual, due to wxUIActionSimulator not working correctly there. --- tests/controls/datepickerctrltest.cpp | 48 +++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/controls/datepickerctrltest.cpp b/tests/controls/datepickerctrltest.cpp index abef3925f8..2a9094a6e7 100644 --- a/tests/controls/datepickerctrltest.cpp +++ b/tests/controls/datepickerctrltest.cpp @@ -16,9 +16,11 @@ #ifndef WX_PRECOMP #include "wx/app.h" + #include "wx/button.h" #endif // WX_PRECOMP #include "wx/datectrl.h" +#include "wx/uiaction.h" #include "testableframe.h" #include "testdate.h" @@ -35,12 +37,15 @@ private: CPPUNIT_TEST_SUITE( DatePickerCtrlTestCase ); CPPUNIT_TEST( Value ); CPPUNIT_TEST( Range ); + WXUISIM_TEST( Focus ); CPPUNIT_TEST_SUITE_END(); void Value(); void Range(); + void Focus(); wxDatePickerCtrl* m_datepicker; + wxButton* m_button; wxDECLARE_NO_COPY_CLASS(DatePickerCtrlTestCase); }; @@ -54,10 +59,12 @@ CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( DatePickerCtrlTestCase, "DatePickerCtrlTe void DatePickerCtrlTestCase::setUp() { m_datepicker = new wxDatePickerCtrl(wxTheApp->GetTopWindow(), wxID_ANY); + m_button = NULL; } void DatePickerCtrlTestCase::tearDown() { + delete m_button; delete m_datepicker; } @@ -115,4 +122,45 @@ void DatePickerCtrlTestCase::Range() CPPUNIT_ASSERT_EQUAL( dtBeforeEnd, m_datepicker->GetValue() ); } +#if wxUSE_UIACTIONSIMULATOR + +static wxPoint GetRectCenter(const wxRect& r) +{ + return (r.GetTopRight() + r.GetBottomLeft()) / 2; +} + +void DatePickerCtrlTestCase::Focus() +{ + // Create another control just to give focus to it initially. + m_button = new wxButton(wxTheApp->GetTopWindow(), wxID_OK); + m_button->Move(0, m_datepicker->GetSize().y * 3); + m_button->SetFocus(); + wxYield(); + + CHECK( !m_datepicker->HasFocus() ); + + EventCounter setFocus(m_datepicker, wxEVT_SET_FOCUS); + EventCounter killFocus(m_datepicker, wxEVT_KILL_FOCUS); + + wxUIActionSimulator sim; + + sim.MouseMove(GetRectCenter(m_datepicker->GetScreenRect())); + sim.MouseClick(); + wxYield(); + + REQUIRE( m_datepicker->HasFocus() ); + CHECK( setFocus.GetCount() == 1 ); + CHECK( killFocus.GetCount() == 0 ); + + sim.MouseMove(GetRectCenter(m_button->GetScreenRect())); + sim.MouseClick(); + wxYield(); + + CHECK( !m_datepicker->HasFocus() ); + CHECK( setFocus.GetCount() == 1 ); + CHECK( killFocus.GetCount() == 1 ); +} + +#endif // wxUSE_UIACTIONSIMULATOR + #endif // wxUSE_DATEPICKCTRL From 25a2bae83611e768403d4485fadb0f13053d9d38 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 23 Jul 2018 00:29:31 +0200 Subject: [PATCH 4/6] Test focus event generation for all pages of the widgets sample Previously this was done only for wxSearchCtrl, extend this now to all the widgets in order to be able to check whether the expected events are generated. --- samples/widgets/searchctrl.cpp | 20 -------------------- samples/widgets/widgets.cpp | 31 +++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/samples/widgets/searchctrl.cpp b/samples/widgets/searchctrl.cpp index a4607cafa7..ec295e87e9 100644 --- a/samples/widgets/searchctrl.cpp +++ b/samples/widgets/searchctrl.cpp @@ -88,9 +88,6 @@ protected: void OnSearch(wxCommandEvent& event); void OnSearchCancel(wxCommandEvent& event); - void OnSetFocus(wxFocusEvent& event); - void OnKillFocus(wxFocusEvent& event); - wxMenu* CreateTestMenu(); // (re)create the control @@ -179,9 +176,6 @@ void SearchCtrlWidgetsPage::CreateControl() m_srchCtrl = new wxSearchCtrl(this, -1, wxEmptyString, wxDefaultPosition, FromDIP(wxSize(150, -1)), style); - - m_srchCtrl->Bind(wxEVT_SET_FOCUS, &SearchCtrlWidgetsPage::OnSetFocus, this); - m_srchCtrl->Bind(wxEVT_KILL_FOCUS, &SearchCtrlWidgetsPage::OnKillFocus, this); } void SearchCtrlWidgetsPage::RecreateWidget() @@ -256,18 +250,4 @@ void SearchCtrlWidgetsPage::OnSearchCancel(wxCommandEvent& event) event.Skip(); } -void SearchCtrlWidgetsPage::OnSetFocus(wxFocusEvent& event) -{ - wxLogMessage("Search control got focus"); - - event.Skip(); -} - -void SearchCtrlWidgetsPage::OnKillFocus(wxFocusEvent& event) -{ - wxLogMessage("Search control lost focus"); - - event.Skip(); -} - #endif // wxUSE_SEARCHCTRL diff --git a/samples/widgets/widgets.cpp b/samples/widgets/widgets.cpp index 3cd4f6c6a3..5df2d6972c 100644 --- a/samples/widgets/widgets.cpp +++ b/samples/widgets/widgets.cpp @@ -210,6 +210,10 @@ protected: WidgetsPage *CurrentPage(); private: + void OnWidgetFocus(wxFocusEvent& event); + + void ConnectToWidgetEvents(); + // the panel containing everything wxPanel *m_panel; @@ -668,6 +672,19 @@ WidgetsPage *WidgetsFrame::CurrentPage() return wxStaticCast(page, WidgetsPage); } +void WidgetsFrame::ConnectToWidgetEvents() +{ + const Widgets& widgets = CurrentPage()->GetWidgets(); + + for ( Widgets::const_iterator it = widgets.begin(); + it != widgets.end(); + ++it ) + { + (*it)->Bind(wxEVT_SET_FOCUS, &WidgetsFrame::OnWidgetFocus, this); + (*it)->Bind(wxEVT_KILL_FOCUS, &WidgetsFrame::OnWidgetFocus, this); + } +} + WidgetsFrame::~WidgetsFrame() { #if USE_LOG @@ -723,7 +740,10 @@ void WidgetsFrame::OnPageChanged(WidgetsBookCtrlEvent& event) wxWindowUpdateLocker noUpdates(curPage); curPage->CreateContent(); curPage->Layout(); + + ConnectToWidgetEvents(); } + // re-apply the attributes to the widget(s) curPage->SetUpWidget(); @@ -890,6 +910,9 @@ void WidgetsFrame::OnSetBorder(wxCommandEvent& event) WidgetsPage *page = CurrentPage(); page->RecreateWidget(); + + ConnectToWidgetEvents(); + // re-apply the attributes to the widget(s) page->SetUpWidget(); } @@ -1172,6 +1195,14 @@ void WidgetsFrame::OnSetHint(wxCommandEvent& WXUNUSED(event)) #endif // wxUSE_MENUS +void WidgetsFrame::OnWidgetFocus(wxFocusEvent& event) +{ + wxLogMessage("Widgets %s focus", + event.GetEventType() == wxEVT_SET_FOCUS ? "got" : "lost"); + + event.Skip(); +} + // ---------------------------------------------------------------------------- // WidgetsPageInfo // ---------------------------------------------------------------------------- From c9cce7e07160644ca53763d96b9519803245fc07 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 24 Jul 2018 01:28:32 +0200 Subject: [PATCH 5/6] Refactor focus debug logging in wxGTK No real changes, just add a helper wxDumpWindow() function to make focus logging less verbose and slightly more readable (as the label is not shown any more if it's empty). --- src/gtk/window.cpp | 50 +++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/src/gtk/window.cpp b/src/gtk/window.cpp index e6889a3549..866eb2905f 100644 --- a/src/gtk/window.cpp +++ b/src/gtk/window.cpp @@ -308,6 +308,24 @@ static wxPoint gs_lastGesturePoint; // the trace mask used for the focus debugging messages #define TRACE_FOCUS wxT("focus") +// Function used to dump a brief description of a window. +static +wxString wxDumpWindow(wxWindow* win) +{ + if ( !win ) + return "(no window)"; + + wxString s = wxString::Format("%s(%p", + win->GetClassInfo()->GetClassName(), win); + + wxString label = win->GetLabel(); + if ( !label.empty() ) + s += wxString::Format(", \"%s\"", label); + s += ")"; + + return s; +} + // A handy function to run from under gdb to show information about the given // GtkWidget. Right now it only shows its type, we could enhance it to show // more information later but this is already pretty useful. @@ -4335,8 +4353,8 @@ bool wxWindowGTK::GTKHandleFocusIn() // GTK+ focus changed from this wxWindow back to itself, so don't // emit any events at all wxLogTrace(TRACE_FOCUS, - "filtered out spurious focus change within %s(%p, %s)", - GetClassInfo()->GetClassName(), this, GetLabel()); + "filtered out spurious focus change within %s", + wxDumpWindow(this)); gs_deferredFocusOut = NULL; return retval; } @@ -4349,8 +4367,8 @@ bool wxWindowGTK::GTKHandleFocusIn() wxLogTrace(TRACE_FOCUS, - "handling focus_in event for %s(%p, %s)", - GetClassInfo()->GetClassName(), this, GetLabel()); + "handling focus_in event for %s", + wxDumpWindow(this)); if (m_imContext) gtk_im_context_focus_in(m_imContext); @@ -4401,8 +4419,8 @@ bool wxWindowGTK::GTKHandleFocusOut() wxASSERT_MSG( gs_deferredFocusOut == NULL, "deferred focus out event already pending" ); wxLogTrace(TRACE_FOCUS, - "deferring focus_out event for %s(%p, %s)", - GetClassInfo()->GetClassName(), this, GetLabel()); + "deferring focus_out event for %s", + wxDumpWindow(this)); gs_deferredFocusOut = this; return retval; } @@ -4415,8 +4433,8 @@ bool wxWindowGTK::GTKHandleFocusOut() void wxWindowGTK::GTKHandleFocusOutNoDeferring() { wxLogTrace(TRACE_FOCUS, - "handling focus_out event for %s(%p, %s)", - GetClassInfo()->GetClassName(), this, GetLabel()); + "handling focus_out event for %s", + wxDumpWindow(this)); gs_lastFocus = this; @@ -4435,8 +4453,8 @@ void wxWindowGTK::GTKHandleFocusOutNoDeferring() // * or it goes to another control, in which case focus-in event will // follow immediately and it will set gs_currentFocus to the right // value - wxLogDebug("window %s(%p, %s) lost focus even though it didn't have it", - GetClassInfo()->GetClassName(), this, GetLabel()); + wxLogDebug("window %s lost focus even though it didn't have it", + wxDumpWindow(this)); } gs_currentFocus = NULL; @@ -4463,8 +4481,8 @@ void wxWindowGTK::GTKHandleDeferredFocusOut() gs_deferredFocusOut = NULL; wxLogTrace(TRACE_FOCUS, - "processing deferred focus_out event for %s(%p, %s)", - GetClassInfo()->GetClassName(), this, GetLabel()); + "processing deferred focus_out event for %s", + wxDumpWindow(this)); GTKHandleFocusOutNoDeferring(); } @@ -4493,15 +4511,15 @@ void wxWindowGTK::SetFocus() !gtk_widget_get_can_focus(widget) ) { wxLogTrace(TRACE_FOCUS, - wxT("Setting focus to a child of %s(%p, %s)"), - GetClassInfo()->GetClassName(), this, GetLabel().c_str()); + wxT("Setting focus to a child of %s"), + wxDumpWindow(this)); gtk_widget_child_focus(widget, GTK_DIR_TAB_FORWARD); } else { wxLogTrace(TRACE_FOCUS, - wxT("Setting focus to %s(%p, %s)"), - GetClassInfo()->GetClassName(), this, GetLabel().c_str()); + wxT("Setting focus to %s"), + wxDumpWindow(this)); gtk_widget_grab_focus(widget); } } From 4059f24cf5e6ba8eb5bed80bcb5db7ae8c41959b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 24 Jul 2018 01:36:37 +0200 Subject: [PATCH 6/6] Reset pending focus when losing focus in wxGTK It could happen that the window remained as the global "pending focus" even after it lost its focus, resulting in FindFocus() still returning it as the currently focused window, even though it didn't have focus any more. This notably broke focus logic of wxCompositeWindow and could result in missing wxEVT_KILL_FOCUS for windows using it, such as wxDatePickerCtrl, and quite likely was responsible for other focus problems in wxGTK as well. --- src/gtk/window.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/gtk/window.cpp b/src/gtk/window.cpp index 866eb2905f..9f17f19ebe 100644 --- a/src/gtk/window.cpp +++ b/src/gtk/window.cpp @@ -4374,7 +4374,13 @@ bool wxWindowGTK::GTKHandleFocusIn() gtk_im_context_focus_in(m_imContext); gs_currentFocus = this; - gs_pendingFocus = NULL; + + if ( gs_pendingFocus ) + { + wxLogTrace(TRACE_FOCUS, "Resetting pending focus %s on focus set", + wxDumpWindow(gs_pendingFocus)); + gs_pendingFocus = NULL; + } #if wxUSE_CARET // caret needs to be informed about focus change @@ -4406,6 +4412,15 @@ bool wxWindowGTK::GTKHandleFocusOut() // handler issues a repaint const bool retval = m_wxwindow ? true : false; + // If this window is still the pending focus one, reset that pointer as + // we're not going to have focus any longer and DoFindFocus() must not + // return this window. + if ( gs_pendingFocus == this ) + { + wxLogTrace(TRACE_FOCUS, "Resetting pending focus %s on focus loss", + wxDumpWindow(this)); + gs_pendingFocus = NULL; + } // NB: If a control is composed of several GtkWidgets and when focus // changes from one of them to another within the same wxWindow, we get