From c369c792a355ec1e422bf6f423131af1444a8809 Mon Sep 17 00:00:00 2001 From: ikamakj Date: Sat, 30 Jun 2018 23:05:57 +0200 Subject: [PATCH 1/8] Fix selection event generation in wxMac wxListBox Prevents deselecting the selected item in single-selection listbox. Also generate correct events in the multi-selection case by reusing the existing wxListBoxBase::CalcAndSendEvent() method. Closes #15603. --- include/wx/osx/listbox.h | 6 ++++++ src/osx/cocoa/listbox.mm | 40 +++++++++++++++++++++++++--------------- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/include/wx/osx/listbox.h b/include/wx/osx/listbox.h index e96b186d7a..f716bf1ae8 100644 --- a/include/wx/osx/listbox.h +++ b/include/wx/osx/listbox.h @@ -125,6 +125,12 @@ public: bool MacGetBlockEvents() const { return m_blockEvents; } virtual void HandleLineEvent( unsigned int n, bool doubleClick ); + + // These are called by wxNSTableView + using wxListBoxBase::DoChangeSingleSelection; + using wxListBoxBase::CalcAndSendEvent; + int GetOldSelection() const { return m_oldSelections.empty() ? wxNOT_FOUND : m_oldSelections[0]; } + protected: // callback for derived classes which may have to insert additional data // at a certain line - which cannot be predetermined for sorted list data diff --git a/src/osx/cocoa/listbox.mm b/src/osx/cocoa/listbox.mm index 2078be5035..402de57c04 100644 --- a/src/osx/cocoa/listbox.mm +++ b/src/osx/cocoa/listbox.mm @@ -297,24 +297,34 @@ protected: int row = [self selectedRow]; - if (row == -1) - { - // no row selected - } - else - { - wxWidgetCocoaImpl* impl = (wxWidgetCocoaImpl* ) wxWidgetImpl::FindFromWXWidget( self ); - wxListBox *list = static_cast ( impl->GetWXPeer()); - wxCHECK_RET( list != NULL , wxT("Listbox expected")); + wxWidgetCocoaImpl* impl = (wxWidgetCocoaImpl* ) wxWidgetImpl::FindFromWXWidget( self ); + wxListBox *list = static_cast ( impl->GetWXPeer()); + wxCHECK_RET( list != NULL , wxT("Listbox expected")); - if ((row < 0) || (row > (int) list->GetCount())) // OS X can select an item below the last item - return; - - if ( !list->MacGetBlockEvents() ) - list->HandleLineEvent( row, false ); + // Correct notification events for multiselection list, like in Carbon version + if (list->HasMultipleSelection() && !list->MacGetBlockEvents()) + { + list->CalcAndSendEvent(); + return; } -} + if ( !list->MacGetBlockEvents() ) + { + // OS X can select an item below the last item. In that case keep the old selection because + // in wxWidgets API there is no notification event for removing the selection from a single-selection list box. + // Otherwise call DoChangeSingleSelection so GetOldSelection() will return the correct value if row < 0 later. + if ((row < 0) || (row > (int) list->GetCount())) + { + int oldsel = list->GetOldSelection(); + if (oldsel >= 0) + list->SetSelection(oldsel); + return; + } + if ( !list->DoChangeSingleSelection(row) ) + return ; + list->HandleLineEvent( row, false ); + } +} - (void)setFont:(NSFont *)aFont { From 0084fb94beab7e45caeb2a069a5b678c686758f1 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Jun 2018 23:09:06 +0200 Subject: [PATCH 2/8] Use wxDynamicCast() instead of static_cast<> This is safer as it really checks if the pointer is of the correct type. --- src/osx/cocoa/listbox.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/osx/cocoa/listbox.mm b/src/osx/cocoa/listbox.mm index 402de57c04..7e0b32cd7e 100644 --- a/src/osx/cocoa/listbox.mm +++ b/src/osx/cocoa/listbox.mm @@ -298,7 +298,7 @@ protected: int row = [self selectedRow]; wxWidgetCocoaImpl* impl = (wxWidgetCocoaImpl* ) wxWidgetImpl::FindFromWXWidget( self ); - wxListBox *list = static_cast ( impl->GetWXPeer()); + wxListBox* const list = wxDynamicCast(impl->GetWXPeer(), wxListBox); wxCHECK_RET( list != NULL , wxT("Listbox expected")); // Correct notification events for multiselection list, like in Carbon version From 1eee7a8a3c414c6b9dda291f37f611c4d2e2aaf4 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Jun 2018 23:10:02 +0200 Subject: [PATCH 3/8] Slightly improve the assertion failure message Also remove the unnecessary wxT(). --- src/osx/cocoa/listbox.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/osx/cocoa/listbox.mm b/src/osx/cocoa/listbox.mm index 7e0b32cd7e..1fe0d48499 100644 --- a/src/osx/cocoa/listbox.mm +++ b/src/osx/cocoa/listbox.mm @@ -299,7 +299,7 @@ protected: wxWidgetCocoaImpl* impl = (wxWidgetCocoaImpl* ) wxWidgetImpl::FindFromWXWidget( self ); wxListBox* const list = wxDynamicCast(impl->GetWXPeer(), wxListBox); - wxCHECK_RET( list != NULL , wxT("Listbox expected")); + wxCHECK_RET( list != NULL , "Associated control should be a wxListBox" ); // Correct notification events for multiselection list, like in Carbon version if (list->HasMultipleSelection() && !list->MacGetBlockEvents()) From dde6f662fcf537e6f3efcb309446ce3db9aba32f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Jun 2018 23:13:33 +0200 Subject: [PATCH 4/8] Move wxListBox selection event generation to wxListBox itself This allows to avoid making DoChangeSingleSelection() and CalcAndSendEvent() public. No real changes, this is just a refactoring. --- include/wx/osx/listbox.h | 6 ++---- src/osx/cocoa/listbox.mm | 24 +----------------------- src/osx/listbox_osx.cpp | 30 ++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 27 deletions(-) diff --git a/include/wx/osx/listbox.h b/include/wx/osx/listbox.h index f716bf1ae8..2b84ad463c 100644 --- a/include/wx/osx/listbox.h +++ b/include/wx/osx/listbox.h @@ -126,10 +126,8 @@ public: virtual void HandleLineEvent( unsigned int n, bool doubleClick ); - // These are called by wxNSTableView - using wxListBoxBase::DoChangeSingleSelection; - using wxListBoxBase::CalcAndSendEvent; - int GetOldSelection() const { return m_oldSelections.empty() ? wxNOT_FOUND : m_oldSelections[0]; } + // This is called by wxNSTableView + void MacHandleSelectionChange(int row); protected: // callback for derived classes which may have to insert additional data diff --git a/src/osx/cocoa/listbox.mm b/src/osx/cocoa/listbox.mm index 1fe0d48499..d8847e5ae3 100644 --- a/src/osx/cocoa/listbox.mm +++ b/src/osx/cocoa/listbox.mm @@ -301,29 +301,7 @@ protected: wxListBox* const list = wxDynamicCast(impl->GetWXPeer(), wxListBox); wxCHECK_RET( list != NULL , "Associated control should be a wxListBox" ); - // Correct notification events for multiselection list, like in Carbon version - if (list->HasMultipleSelection() && !list->MacGetBlockEvents()) - { - list->CalcAndSendEvent(); - return; - } - - if ( !list->MacGetBlockEvents() ) - { - // OS X can select an item below the last item. In that case keep the old selection because - // in wxWidgets API there is no notification event for removing the selection from a single-selection list box. - // Otherwise call DoChangeSingleSelection so GetOldSelection() will return the correct value if row < 0 later. - if ((row < 0) || (row > (int) list->GetCount())) - { - int oldsel = list->GetOldSelection(); - if (oldsel >= 0) - list->SetSelection(oldsel); - return; - } - if ( !list->DoChangeSingleSelection(row) ) - return ; - list->HandleLineEvent( row, false ); - } + list->MacHandleSelectionChange(row); } - (void)setFont:(NSFont *)aFont diff --git a/src/osx/listbox_osx.cpp b/src/osx/listbox_osx.cpp index f824848ec0..75b1ad9750 100644 --- a/src/osx/listbox_osx.cpp +++ b/src/osx/listbox_osx.cpp @@ -420,6 +420,36 @@ void wxListBox::HandleLineEvent( unsigned int n, bool doubleClick ) HandleWindowEvent(event); } +void wxListBox::MacHandleSelectionChange(int row) +{ + // Correct notification events for multiselection list, like in Carbon version + if ( HasMultipleSelection() && !MacGetBlockEvents() ) + { + CalcAndSendEvent(); + return; + } + + if ( !MacGetBlockEvents() ) + { + // OS X can select an item below the last item. In that case keep the old selection because + // in wxWidgets API there is no notification event for removing the selection from a single-selection list box. + // Otherwise call DoChangeSingleSelection so GetOldSelection() will return the correct value if row < 0 later. + if ((row < 0) || (row > (int) GetCount())) + { + if ( !m_oldSelections.empty() ) + { + const int oldsel = m_oldSelections[0]; + if (oldsel >= 0) + SetSelection(oldsel); + } + return; + } + if ( !DoChangeSingleSelection(row) ) + return ; + HandleLineEvent( row, false ); + } +} + // // common list cell value operations // From eadcd93bf9326dcd61e8c5586d4392715c8f4c3e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Jun 2018 23:14:36 +0200 Subject: [PATCH 5/8] Remove wxListBox::MacGetBlockEvents() Just test m_blockEvents directly, there doesn't seem to be any gain in using an accessor here. Also test it only once instead of doing it twice in MacHandleSelectionChange(). --- include/wx/osx/listbox.h | 2 -- src/osx/listbox_osx.cpp | 32 ++++++++++++++++---------------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/include/wx/osx/listbox.h b/include/wx/osx/listbox.h index 2b84ad463c..2882c2eff9 100644 --- a/include/wx/osx/listbox.h +++ b/include/wx/osx/listbox.h @@ -122,8 +122,6 @@ public: wxListWidgetImpl* GetListPeer() const; - bool MacGetBlockEvents() const { return m_blockEvents; } - virtual void HandleLineEvent( unsigned int n, bool doubleClick ); // This is called by wxNSTableView diff --git a/src/osx/listbox_osx.cpp b/src/osx/listbox_osx.cpp index 75b1ad9750..51cd293ca6 100644 --- a/src/osx/listbox_osx.cpp +++ b/src/osx/listbox_osx.cpp @@ -422,32 +422,32 @@ void wxListBox::HandleLineEvent( unsigned int n, bool doubleClick ) void wxListBox::MacHandleSelectionChange(int row) { + if ( m_blockEvents ) + return; + // Correct notification events for multiselection list, like in Carbon version - if ( HasMultipleSelection() && !MacGetBlockEvents() ) + if ( HasMultipleSelection() ) { CalcAndSendEvent(); return; } - if ( !MacGetBlockEvents() ) + // OS X can select an item below the last item. In that case keep the old selection because + // in wxWidgets API there is no notification event for removing the selection from a single-selection list box. + // Otherwise call DoChangeSingleSelection so GetOldSelection() will return the correct value if row < 0 later. + if ((row < 0) || (row > (int) GetCount())) { - // OS X can select an item below the last item. In that case keep the old selection because - // in wxWidgets API there is no notification event for removing the selection from a single-selection list box. - // Otherwise call DoChangeSingleSelection so GetOldSelection() will return the correct value if row < 0 later. - if ((row < 0) || (row > (int) GetCount())) + if ( !m_oldSelections.empty() ) { - if ( !m_oldSelections.empty() ) - { - const int oldsel = m_oldSelections[0]; - if (oldsel >= 0) - SetSelection(oldsel); - } - return; + const int oldsel = m_oldSelections[0]; + if (oldsel >= 0) + SetSelection(oldsel); } - if ( !DoChangeSingleSelection(row) ) - return ; - HandleLineEvent( row, false ); + return; } + if ( !DoChangeSingleSelection(row) ) + return ; + HandleLineEvent( row, false ); } // From f51cb52004d0d72fe253712867363163b0f2faaf Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Jun 2018 23:17:03 +0200 Subject: [PATCH 6/8] Reformat comments and code in MacHandleSelectionChange() No real changes, just trying to make the code more clear. --- src/osx/listbox_osx.cpp | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/osx/listbox_osx.cpp b/src/osx/listbox_osx.cpp index 51cd293ca6..01b6ef7f0d 100644 --- a/src/osx/listbox_osx.cpp +++ b/src/osx/listbox_osx.cpp @@ -425,17 +425,21 @@ void wxListBox::MacHandleSelectionChange(int row) if ( m_blockEvents ) return; - // Correct notification events for multiselection list, like in Carbon version + // Correct notification events for multiselection list. if ( HasMultipleSelection() ) { CalcAndSendEvent(); return; } - // OS X can select an item below the last item. In that case keep the old selection because - // in wxWidgets API there is no notification event for removing the selection from a single-selection list box. - // Otherwise call DoChangeSingleSelection so GetOldSelection() will return the correct value if row < 0 later. - if ((row < 0) || (row > (int) GetCount())) + // OS X can select an item below the last item. In that case keep the old + // selection because in wxWidgets API there is no notification event for + // removing the selection from a single-selection list box. + // + // Otherwise call DoChangeSingleSelection so GetOldSelection() will return + // the correct value if row < 0 later. + const int count = static_cast(GetCount()); + if ( row < 0 || row > count ) { if ( !m_oldSelections.empty() ) { @@ -443,11 +447,11 @@ void wxListBox::MacHandleSelectionChange(int row) if (oldsel >= 0) SetSelection(oldsel); } - return; } - if ( !DoChangeSingleSelection(row) ) - return ; - HandleLineEvent( row, false ); + else if ( DoChangeSingleSelection(row) ) + { + HandleLineEvent( row, false ); + } } // From 30c798b70a96636f865b38d896787342150cec3b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Jun 2018 23:17:37 +0200 Subject: [PATCH 7/8] Fix row validity check in MacHandleSelectionChange() Valid indices must be in [0, count[ half-open interval. --- src/osx/listbox_osx.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/osx/listbox_osx.cpp b/src/osx/listbox_osx.cpp index 01b6ef7f0d..b00514c94f 100644 --- a/src/osx/listbox_osx.cpp +++ b/src/osx/listbox_osx.cpp @@ -439,7 +439,7 @@ void wxListBox::MacHandleSelectionChange(int row) // Otherwise call DoChangeSingleSelection so GetOldSelection() will return // the correct value if row < 0 later. const int count = static_cast(GetCount()); - if ( row < 0 || row > count ) + if ( row < 0 || row >= count ) { if ( !m_oldSelections.empty() ) { From c53524a59ba39871269dfacbcee2187ab8776d69 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Jun 2018 23:18:28 +0200 Subject: [PATCH 8/8] Also check the old selection index for validity It's not totally clear if the old selection is always guaranteed to be valid so prefer to check it explicitly. --- src/osx/listbox_osx.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/osx/listbox_osx.cpp b/src/osx/listbox_osx.cpp index b00514c94f..ff41b53a37 100644 --- a/src/osx/listbox_osx.cpp +++ b/src/osx/listbox_osx.cpp @@ -444,7 +444,7 @@ void wxListBox::MacHandleSelectionChange(int row) if ( !m_oldSelections.empty() ) { const int oldsel = m_oldSelections[0]; - if (oldsel >= 0) + if ( oldsel >= 0 && oldsel < count ) SetSelection(oldsel); } }