From 95b7a2f1837d677d8ee9c79f78c79cd3bfc5f3fb Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 30 Nov 2021 23:03:28 +0000 Subject: [PATCH 1/6] Return false from PrepareForItem() if there is no value This corresponds to the behaviour described in the comment and is more useful than always returning true. --- src/common/datavcmn.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/common/datavcmn.cpp b/src/common/datavcmn.cpp index f17b761ffa..cd070d2870 100644 --- a/src/common/datavcmn.cpp +++ b/src/common/datavcmn.cpp @@ -913,16 +913,15 @@ wxDataViewRendererBase::PrepareForItem(const wxDataViewModel *model, // empty cells. SetEnabled(model->IsEnabled(item, column)); + return !value.IsNull(); } wxCATCH_ALL ( // There is not much we can do about it here, just log it and don't // show anything in this cell. wxLogDebug("Retrieving the value from the model threw an exception"); - SetValue(wxVariant()); + return false; ) - - return true; } From 9cb360a0aa076b0efea3e1a36c15a8b3a6172618 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Dec 2021 02:22:06 +0100 Subject: [PATCH 2/6] Don't call SetValue() with null value in wxOSX wxDVC We can't guarantee that GetValue() returns a valid value, even if HasValue() returned true, so avoid calling SetValue() if it returned an invalid one, as this risks triggering an assert failure and because this function is typically called when repainting the control, there is a good chance that we're going to reenter it while showing the assert dialog box, resulting in an abort, which is not the best way to handle GetValue() not returning a valid value -- especially when the generic version doesn't do this, so the problem risks only appearing when porting to Mac. --- src/osx/cocoa/dataview.mm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/osx/cocoa/dataview.mm b/src/osx/cocoa/dataview.mm index ac596c5114..06bbddf9cc 100644 --- a/src/osx/cocoa/dataview.mm +++ b/src/osx/cocoa/dataview.mm @@ -626,7 +626,8 @@ outlineView:(NSOutlineView*)outlineView { wxVariant value; model->GetValue(value,dataViewItem, colIdx); - col->GetRenderer()->SetValue(value); + if ( !value.IsNull() ) + col->GetRenderer()->SetValue(value); } return nil; From 55420130b5157c14b466be0d5e62c67401853251 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Dec 2021 01:32:22 +0000 Subject: [PATCH 3/6] Don't use items without values in generic wxDVC In particular, don't draw them, as this would reuse the value of the previously drawn item, which would be wrong -- just leave them blank if PrepareForItem() returned false, which happens if GetValue() returned a null value or a value of a wrong type. --- src/generic/datavgen.cpp | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 43c9facee3..2712368c6f 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -2463,14 +2463,15 @@ wxBitmap wxDataViewMainWindow::CreateItemBitmap( unsigned int row, int &indent ) width -= indent; wxDataViewItem item = GetItemByRow( row ); - cell->PrepareForItem(model, item, column->GetModelColumn()); + if ( cell->PrepareForItem(model, item, column->GetModelColumn()) ) + { + wxRect item_rect(x, 0, width, height); + item_rect.Deflate(PADDING_RIGHTLEFT, 0); - wxRect item_rect(x, 0, width, height); - item_rect.Deflate(PADDING_RIGHTLEFT, 0); - - // dc.SetClippingRegion( item_rect ); - cell->WXCallRender(item_rect, &dc, 0); - // dc.DestroyClippingRegion(); + // dc.SetClippingRegion( item_rect ); + cell->WXCallRender(item_rect, &dc, 0); + // dc.DestroyClippingRegion(); + } x += width; } @@ -2836,7 +2837,7 @@ void wxDataViewMainWindow::OnPaint( wxPaintEvent &WXUNUSED(event) ) cell->SetState(state); if (hasValue) - cell->PrepareForItem(model, dataitem, col->GetModelColumn()); + hasValue = cell->PrepareForItem(model, dataitem, col->GetModelColumn()); // draw the background if ( !selected ) @@ -3789,9 +3790,8 @@ int wxDataViewMainWindow::QueryAndCacheLineHeight(unsigned int row, wxDataViewIt wxDataViewRenderer *renderer = const_cast(column->GetRenderer()); - renderer->PrepareForItem(model, item, column->GetModelColumn()); - - height = wxMax(height, renderer->GetSize().y); + if ( renderer->PrepareForItem(model, item, column->GetModelColumn()) ) + height = wxMax(height, renderer->GetSize().y); } // ... and store the height in the cache @@ -5998,8 +5998,8 @@ public: if ( m_model->HasValue(item, GetColumn()) ) { - m_renderer->PrepareForItem(m_model, item, GetColumn()); - width += m_renderer->GetSize().x; + if ( m_renderer->PrepareForItem(m_model, item, GetColumn()) ) + width += m_renderer->GetSize().x; } UpdateWithWidth(width); @@ -6747,7 +6747,9 @@ wxAccStatus wxDataViewCtrlAccessible::GetName(int childId, wxString* name) continue; // Skip non-textual items wxDataViewRenderer* r = dvCol->GetRenderer(); - r->PrepareForItem(model, item, dvCol->GetModelColumn()); + if ( !r->PrepareForItem(model, item, dvCol->GetModelColumn()) ) + continue; + wxString vs = r->GetAccessibleDescription(); if ( !vs.empty() ) { @@ -6905,7 +6907,9 @@ wxAccStatus wxDataViewCtrlAccessible::GetDescription(int childId, wxString* desc model->GetValue(value, item, dvCol->GetModelColumn()); wxDataViewRenderer* r = dvCol->GetRenderer(); - r->PrepareForItem(model, item, dvCol->GetModelColumn()); + if ( !r->PrepareForItem(model, item, dvCol->GetModelColumn()) ) + continue; + wxString valStr = r->GetAccessibleDescription(); // Skip first textual item if ( !firstTextSkipped && !value.IsNull() && !value.IsType(wxS("bool")) && !valStr.empty() ) From 74e1c444fa4a66c1c5fe3091491665ea44e0b042 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Dec 2021 02:42:20 +0100 Subject: [PATCH 4/6] Don't show bogus value when there are none in wxGTK wxDVC neither This is similar to the previous change to the generic version and simply applies the same logic to the cells for which GetValue() returns null value as for those for which HasValue() returns false. --- src/gtk/dataview.cpp | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/gtk/dataview.cpp b/src/gtk/dataview.cpp index 767334e004..7d8140d153 100644 --- a/src/gtk/dataview.cpp +++ b/src/gtk/dataview.cpp @@ -3171,6 +3171,14 @@ gtk_dataview_header_button_press_callback( GtkWidget *WXUNUSED(widget), return FALSE; } +// Helper for wxGtkTreeCellDataFunc() below. +static void wxGtkTreeSetVisibleProp(GtkCellRenderer *renderer, gboolean visible) +{ + wxGtkValue gvalue( G_TYPE_BOOLEAN ); + g_value_set_boolean( gvalue, visible ); + g_object_set_property( G_OBJECT(renderer), "visible", gvalue ); +} + extern "C" { @@ -3200,16 +3208,22 @@ static void wxGtkTreeCellDataFunc( GtkTreeViewColumn *WXUNUSED(column), if (!wx_model->IsVirtualListModel()) { gboolean visible = wx_model->HasValue(item, column); - wxGtkValue gvalue( G_TYPE_BOOLEAN ); - g_value_set_boolean( gvalue, visible ); - g_object_set_property( G_OBJECT(renderer), "visible", gvalue ); + wxGtkTreeSetVisibleProp(renderer, visible); if ( !visible ) return; } cell->GtkSetCurrentItem(item); - cell->PrepareForItem(wx_model, item, column); + + if (!cell->PrepareForItem(wx_model, item, column)) + { + // We don't have any value in this cell, after all, so hide it. + if (!wx_model->IsVirtualListModel()) + { + wxGtkTreeSetVisibleProp(renderer, FALSE); + } + } } } // extern "C" From 0c944ff5bed80420c9916af355aee0593561c3f5 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Dec 2021 02:24:45 +0100 Subject: [PATCH 5/6] Test returning invalid value from GetValue() in dataview sample Add an item with unspecified year and update the custom model GetValue() to not return anything in this case to check, and confirm, that all implementations handle this properly by simply not showing anything in the cell in this case. --- samples/dataview/mymodels.cpp | 20 +++++++++++++++++--- samples/dataview/mymodels.h | 2 +- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/samples/dataview/mymodels.cpp b/samples/dataview/mymodels.cpp index 609c749fa0..d6a4149459 100644 --- a/samples/dataview/mymodels.cpp +++ b/samples/dataview/mymodels.cpp @@ -40,6 +40,8 @@ MyMusicTreeModel::MyMusicTreeModel() m_pop = new MyMusicTreeModelNode( m_root, "Pop music" ); m_pop->Append( new MyMusicTreeModelNode( m_pop, "You are not alone", "Michael Jackson", 1995 ) ); + m_pop->Append( + new MyMusicTreeModelNode( m_pop, "Yesterday", "The Beatles", -1 /* not specified */ ) ); m_pop->Append( new MyMusicTreeModelNode( m_pop, "Take a bow", "Madonna", 1994 ) ); m_root->Append( m_pop ); @@ -193,7 +195,8 @@ void MyMusicTreeModel::GetValue( wxVariant &variant, variant = node->m_artist; break; case 2: - variant = (long) node->m_year; + if (node->m_year != -1) + variant = (long) node->m_year; break; case 3: variant = node->m_quality; @@ -202,7 +205,9 @@ void MyMusicTreeModel::GetValue( wxVariant &variant, variant = 80L; // all music is very 80% popular break; case 5: - if (GetYear(item) < 1900) + if (node->m_year == -1) + variant = "n/a"; + else if (node->m_year < 1900) variant = "old"; else variant = "new"; @@ -248,7 +253,16 @@ bool MyMusicTreeModel::IsEnabled( const wxDataViewItem &item, MyMusicTreeModelNode *node = (MyMusicTreeModelNode*) item.GetID(); // disable Beethoven's ratings, his pieces can only be good - return !(col == 3 && node->m_artist.EndsWith("Beethoven")); + if (col == 3 && node->m_artist.EndsWith("Beethoven")) + return false; + + // also disable editing the year when it's not specified, this doesn't work + // because the editor needs some initial value + if (col == 2 && node->m_year == -1) + return false; + + // otherwise allow editing + return true; } wxDataViewItem MyMusicTreeModel::GetParent( const wxDataViewItem &item ) const diff --git a/samples/dataview/mymodels.h b/samples/dataview/mymodels.h index a81c82e28c..59733ac61c 100644 --- a/samples/dataview/mymodels.h +++ b/samples/dataview/mymodels.h @@ -26,7 +26,7 @@ class MyMusicTreeModelNode public: MyMusicTreeModelNode( MyMusicTreeModelNode* parent, const wxString &title, const wxString &artist, - unsigned int year ) + int year ) { m_parent = parent; From 26efd904ec9da95ba3ce0e2f331793109fcf275f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 1 Dec 2021 01:48:53 +0000 Subject: [PATCH 6/6] Improve wxDataViewModel::GetValue() documentation Document that this function can leave the provided wxVariant null and not return anything, but that if it does return some value, it must be of the appropriate type. --- interface/wx/dataview.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/interface/wx/dataview.h b/interface/wx/dataview.h index f210b557db..c1c4327c20 100644 --- a/interface/wx/dataview.h +++ b/interface/wx/dataview.h @@ -268,7 +268,17 @@ public: /** Override this to indicate the value of @a item. - A wxVariant is used to store the data. + + This function should fill the provided @a variant with the value to be + shown for the specified item in the given column. The value returned in + this wxVariant must have the appropriate type, e.g. string for the text + columns, boolean for the columns using wxDataViewToggleRenderer etc, + and if there is a type mismatch, nothing will be shown and a debug + error message will be logged. + + It is also possible to not return any value, in which case nothing will + be shown in the corresponding cell, in the same way as if HasValue() + returned @false. */ virtual void GetValue(wxVariant& variant, const wxDataViewItem& item, unsigned int col) const = 0;