From c0df6ec4754c5f45edf2d3254722c83fbf9bc327 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 16 Sep 2019 17:45:17 +0200 Subject: [PATCH 1/3] Merge two wxDataViewWidgetImpl::Remove() overloads into one Having 2 different overloads might have been useful for Carbon implementation, but as they do exactly the same thing in the Cocoa version, leave only one of them -- and don't pass it the item, or items, being deleted as they're not used anyhow. No real changes. --- include/wx/osx/cocoa/dataview.h | 5 +---- include/wx/osx/core/dataview.h | 3 +-- src/osx/cocoa/dataview.mm | 11 +---------- src/osx/dataview_osx.cpp | 6 +++--- 4 files changed, 6 insertions(+), 19 deletions(-) diff --git a/include/wx/osx/cocoa/dataview.h b/include/wx/osx/cocoa/dataview.h index b4203ca6ee..169fe7e754 100644 --- a/include/wx/osx/cocoa/dataview.h +++ b/include/wx/osx/cocoa/dataview.h @@ -486,10 +486,7 @@ public: virtual wxDataViewItem GetTopItem() const; virtual bool IsExpanded(const wxDataViewItem& item) const; virtual bool Reload(); - virtual bool Remove(const wxDataViewItem& parent, - const wxDataViewItem& item); - virtual bool Remove(const wxDataViewItem& parent, - const wxDataViewItemArray& item); + virtual bool Remove(const wxDataViewItem& parent); virtual bool Update(const wxDataViewColumn* columnPtr); virtual bool Update(const wxDataViewItem& parent, const wxDataViewItem& item); diff --git a/include/wx/osx/core/dataview.h b/include/wx/osx/core/dataview.h index 30a91816e3..8fd0292287 100644 --- a/include/wx/osx/core/dataview.h +++ b/include/wx/osx/core/dataview.h @@ -68,8 +68,7 @@ public: virtual wxDataViewItem GetTopItem (void) const = 0; // get top-most visible item virtual bool IsExpanded (wxDataViewItem const& item) const = 0; // checks if the passed item is expanded in the native control virtual bool Reload (void) = 0; // clears the native control and reloads all data - virtual bool Remove (wxDataViewItem const& parent, wxDataViewItem const& item) = 0; // removes an item from the native control - virtual bool Remove (wxDataViewItem const& parent, wxDataViewItemArray const& item) = 0; // removes items from the native control + virtual bool Remove (wxDataViewItem const& parent) = 0; // removes one or more items under the given parent from the native control virtual bool Update (wxDataViewColumn const* columnPtr) = 0; // updates the items in the passed column of the native control virtual bool Update (wxDataViewItem const& parent, wxDataViewItem const& item) = 0; // updates the passed item in the native control virtual bool Update (wxDataViewItem const& parent, wxDataViewItemArray const& items) = 0; // updates the passed items in the native control diff --git a/src/osx/cocoa/dataview.mm b/src/osx/cocoa/dataview.mm index 4b881b6315..b47ad642bd 100644 --- a/src/osx/cocoa/dataview.mm +++ b/src/osx/cocoa/dataview.mm @@ -2339,16 +2339,7 @@ bool wxCocoaDataViewControl::Reload() return true; } -bool wxCocoaDataViewControl::Remove(const wxDataViewItem& parent, const wxDataViewItem& WXUNUSED(item)) -{ - if (parent.IsOk()) - [m_OutlineView reloadItem:[m_DataSource getDataViewItemFromBuffer:parent] reloadChildren:YES]; - else - [m_OutlineView reloadData]; - return true; -} - -bool wxCocoaDataViewControl::Remove(const wxDataViewItem& parent, const wxDataViewItemArray& WXUNUSED(item)) +bool wxCocoaDataViewControl::Remove(const wxDataViewItem& parent) { if (parent.IsOk()) [m_OutlineView reloadItem:[m_DataSource getDataViewItemFromBuffer:parent] reloadChildren:YES]; diff --git a/src/osx/dataview_osx.cpp b/src/osx/dataview_osx.cpp index 30107475e3..60e57b70f2 100644 --- a/src/osx/dataview_osx.cpp +++ b/src/osx/dataview_osx.cpp @@ -170,7 +170,7 @@ bool wxOSXDataViewModelNotifier::ItemDeleted(wxDataViewItem const& parent, wxDat // to prevent the control trying to ask the model to update an already deleted item the control is informed that currently a deleting process // has been started and that variables can currently not be updated even when requested by the system: m_DataViewCtrlPtr->SetDeleting(true); - noFailureFlag = m_DataViewCtrlPtr->GetDataViewPeer()->Remove(parent,item); + noFailureFlag = m_DataViewCtrlPtr->GetDataViewPeer()->Remove(parent); // enable automatic updating again: m_DataViewCtrlPtr->SetDeleting(false); @@ -179,7 +179,7 @@ bool wxOSXDataViewModelNotifier::ItemDeleted(wxDataViewItem const& parent, wxDat return noFailureFlag; } -bool wxOSXDataViewModelNotifier::ItemsDeleted(wxDataViewItem const& parent, wxDataViewItemArray const& items) +bool wxOSXDataViewModelNotifier::ItemsDeleted(wxDataViewItem const& parent, wxDataViewItemArray const& WXUNUSED(items)) { bool noFailureFlag; @@ -190,7 +190,7 @@ bool wxOSXDataViewModelNotifier::ItemsDeleted(wxDataViewItem const& parent, wxDa // has been started and that variables can currently not be updated even when requested by the system: m_DataViewCtrlPtr->SetDeleting(true); // delete all specified items: - noFailureFlag = m_DataViewCtrlPtr->GetDataViewPeer()->Remove(parent,items); + noFailureFlag = m_DataViewCtrlPtr->GetDataViewPeer()->Remove(parent); // enable automatic updating again: m_DataViewCtrlPtr->SetDeleting(false); From 3a9869b4c441c4fcce1e8e8a72f772580a982e13 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 16 Sep 2019 18:04:44 +0200 Subject: [PATCH 2/3] Fix crash when deleting item being edited in wxOSX wxDataViewCtrl Avoid touching the model while updating the control due to the items being deleted from it, as doing this can easily result in dereferencing pointers to the already deleted objects and crashing. This is rather ugly, but the original code seems to have been written with this approach in mind (see preexisting comments in ItemDeleted()) and, to be fair, there doesn't seem any other solution with the existing API, as it allows (or maybe even requires?) deleting the items _before_ notifying the control about their removal and so not giving it any opportunity to stop editing the item while it's still alive. Closes #18337. --- src/osx/cocoa/dataview.mm | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/osx/cocoa/dataview.mm b/src/osx/cocoa/dataview.mm index b47ad642bd..e13521b042 100644 --- a/src/osx/cocoa/dataview.mm +++ b/src/osx/cocoa/dataview.mm @@ -568,6 +568,14 @@ outlineView:(NSOutlineView*)outlineView { wxUnusedVar(outlineView); + if ( implementation->GetDataViewCtrl()->IsDeleting() ) + { + // Note that returning "NO" here would result in currently expanded + // branches not being expanded any more, while returning "YES" doesn't + // seem to have any ill effects, even though this is clearly bogus. + return YES; + } + wxCHECK_MSG( model, 0, "Valid model in data source does not exist." ); return model->IsContainer(wxDataViewItemFromItem(item)); } @@ -597,6 +605,15 @@ outlineView:(NSOutlineView*)outlineView { wxUnusedVar(outlineView); + // We ignore any calls to this function happening while items are being + // deleted, as they can (only?) come from -[NSTableView textDidEndEditing:] + // called from our own textDidEndEditing, which is called by + // -[NSOutlineView reloadItem:reloadChildren:], and if the item being + // edited is one of the items being deleted, then trying to use it would + // attempt to use already freed memory and crash. + if ( implementation->GetDataViewCtrl()->IsDeleting() ) + return nil; + wxCHECK_MSG( model, nil, "Valid model in data source does not exist." ); wxDataViewColumn* const @@ -623,6 +640,11 @@ outlineView:(NSOutlineView*)outlineView { wxUnusedVar(outlineView); + // See the comment in outlineView:objectValueForTableColumn:byItem: above: + // this function can also be called in the same circumstances. + if ( implementation->GetDataViewCtrl()->IsDeleting() ) + return; + wxDataViewColumn* const col([static_cast(tableColumn) getColumnPointer]); @@ -1937,6 +1959,10 @@ outlineView:(NSOutlineView*)outlineView { // call method of superclass (otherwise editing does not work correctly - // the outline data source class is not informed about a change of data): + // + // Note that we really, really need to do this, as otherwise we would be + // left with an orphan text control -- even though doing this forces us to + // have the checks for IsDeleting() in several other methods of this class. [super textDidEndEditing:notification]; // under OSX an event indicating the end of an editing session can be sent From b9338b6130ad0f4d447d38e55f96045227b3062c Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 17 Sep 2019 00:04:04 +0200 Subject: [PATCH 3/3] Call wxDataViewCustomRenderer::ActivateCell() in Mac version too Do this for consistency with the other ports. Closes #17746. --- interface/wx/dataview.h | 4 ---- src/osx/cocoa/dataview.mm | 27 ++++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/interface/wx/dataview.h b/interface/wx/dataview.h index 90bb1c2aaf..085fa2a7c7 100644 --- a/interface/wx/dataview.h +++ b/interface/wx/dataview.h @@ -2501,10 +2501,6 @@ public: corresponding event. Is @NULL otherwise (for keyboard activation). Mouse coordinates are adjusted to be relative to the cell. - @note Currently support for this method is not implemented in the - native macOS version of the control, i.e. it will be never called - there. - @since 2.9.3 @note Do not confuse this method with item activation in wxDataViewCtrl diff --git a/src/osx/cocoa/dataview.mm b/src/osx/cocoa/dataview.mm index e13521b042..78c153c088 100644 --- a/src/osx/cocoa/dataview.mm +++ b/src/osx/cocoa/dataview.mm @@ -1677,10 +1677,35 @@ outlineView:(NSOutlineView*)outlineView // and setDoubleAction: seems to be wrong as this action message is always // sent whether the cell is editable or not wxDataViewCtrl* const dvc = implementation->GetDataViewCtrl(); + wxDataViewModel * const model = dvc->GetModel(); const wxDataViewItem item = wxDataViewItemFromItem([self itemAtRow:[self clickedRow]]); + + const NSInteger col = [self clickedColumn]; + wxDataViewColumn* const dvCol = implementation->GetColumn(col); + + // Check if we need to activate a custom renderer first. + if ( wxDataViewCustomRenderer* const + renderer = wxDynamicCast(dvCol->GetRenderer(), wxDataViewCustomRenderer) ) + { + if ( renderer->GetMode() == wxDATAVIEW_CELL_ACTIVATABLE && + model->IsEnabled(item, dvCol->GetModelColumn()) ) + { + const wxRect rect = implementation->GetRectangle(item, dvCol); + + wxMouseEvent mouseEvent(wxEVT_LEFT_DCLICK); + wxPoint pos = dvc->ScreenToClient(wxGetMousePosition()); + pos -= rect.GetPosition(); + mouseEvent.m_x = pos.x; + mouseEvent.m_y = pos.y; + + renderer->ActivateCell(rect, model, item, col, &mouseEvent); + } + } + + // And then send the ACTIVATED event in any case. wxDataViewEvent event(wxEVT_DATAVIEW_ITEM_ACTIVATED, dvc, item); - event.SetColumn( [self clickedColumn] ); + event.SetColumn(col); dvc->GetEventHandler()->ProcessEvent(event); }