Cells without values are not correctly shown in wxDataViewCtrl with wxGTK 3.1.6.
This has initially been reported in Poedit: vslavik/poedit#746. Following comments from @vslavik and @vadz, I'm moving the ticket to wxWidgets.
In Poedit compiled against wxGTK 3.1.6, the icons indicating the presence of an issue or a comment on the translatable strings are incorrectly updated when scrolling up/down the list of strings. It looks like the strings without issue/comment "reuse" the icons of other visible strings with an issue or a comment.
Reverting commit e067e42 fixes the issue.
The warning/comment icons are correctly displayed.
https://user-images.githubusercontent.com/9394745/163816494-01653885-1d0a-442a-8507-6b89020ea8ab.mp4
The warning/comment icons are not correctly displayed - warning is displayed for string without warnings, the (wrong) warning icon for a given line changes to the comment icon when scrolling down.
https://user-images.githubusercontent.com/9394745/163816668-77f1c3c4-0860-482a-8370-5241dd539976.mp4
I'm not able to create such a snippet. I believe the relevant code in Poedit is located in edlistctrl.cpp; in particular at line 336 in PoeditListCtrl::Model::GetValueByRow
is the code that defines the icon to display. @vslavik: Please correct if I'm wrong.
Assuming Poedit 3.0.1 is installed under the environment mentioned below:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
@vslavik This is the next one in my list of things to fix before 3.1.7, so if you could please make the small reproducer you mentioned in the original bug report, it would be very welcome. Otherwise I'll try to create one myself. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Sorry, I thought I already did, but apparently forgot to upload it :(
Below is a minimal-ish diff against the dataview sample. It's an icon column where some rows don't have an icon and so use wxNullVariant
for the value.
With this patch applied, behavior is:
Using variant << wxNullIcon
instead of wxNullVariant
works on both wxGTK and macOS (didn't test Windows). I think it didn't somewhere at some point in the past.
Putting the question of regressions and backward compatibility aside for a moment, I'm certainly not sure whether wxNullVariant
or wxNullIcon
or both should produce "no value" result, but whatever the preferred approach is, it would be nice to have it documented somewhere.
What I am sure about is that it needs to be possible to represent "no value" in models and renderers somehow. wxNullVariant
would be a consistent way for any data type and renderer, but OTOH, there's history of more than one instance of renderers not expecting it.
diff --git a/samples/dataview/dataview.cpp b/samples/dataview/dataview.cpp index 4b2f1d933e..ec3a4f20ff 100644 --- a/samples/dataview/dataview.cpp +++ b/samples/dataview/dataview.cpp @@ -904,7 +904,7 @@ void MyFrame::BuildDataViewCtrl(wxPanel* parent, unsigned int nPanel, wxDataViewColumn* const colCheckIconText = new wxDataViewColumn ( L"\u2714 + icon + text", - new wxDataViewCheckIconTextRenderer(), + new wxDataViewBitmapRenderer("wxIcon", wxDATAVIEW_CELL_INERT, wxALIGN_CENTER | wxALIGN_CENTRE_VERTICAL), MyListModel::Col_ToggleIconText, wxCOL_WIDTH_AUTOSIZE ); diff --git a/samples/dataview/mymodels.cpp b/samples/dataview/mymodels.cpp index d6d76c2414..25d25c6183 100644 --- a/samples/dataview/mymodels.cpp +++ b/samples/dataview/mymodels.cpp @@ -388,8 +388,8 @@ MyListModel::MyListModel(int modelFlags) : const wxSize size = GetIconSizeFromModelFlags(modelFlags); - m_icon[0] = wxArtProvider::GetBitmapBundle(wxART_QUESTION, wxART_LIST, size); - m_icon[1] = wxArtProvider::GetBitmapBundle(wxART_WX_LOGO, wxART_LIST, size); + m_icon[0] = wxArtProvider::GetIcon(wxART_QUESTION, wxART_LIST, size); + m_icon[1] = wxArtProvider::GetIcon(wxART_WX_LOGO, wxART_LIST, size); } void MyListModel::Prepend( const wxString &text ) @@ -473,20 +473,10 @@ void MyListModel::GetValueByRow( wxVariant &variant, case Col_ToggleIconText: { - wxString text; - wxCheckBoxState state; - if ( row >= m_iconColValues.GetCount() ) - { - text = "virtual icon"; - state = wxCHK_UNDETERMINED; - } + if (row % 2) + variant << m_icon[row % 2]; else - { - text = m_iconColValues[row]; - state = m_toggleColValues[row] ? wxCHK_CHECKED : wxCHK_UNCHECKED; - } - - variant << wxDataViewCheckIconText(text, m_icon[row % 2], state); + variant = wxNullVariant; } break; diff --git a/samples/dataview/mymodels.h b/samples/dataview/mymodels.h index f53d8e12e9..e7f98dc9ee 100644 --- a/samples/dataview/mymodels.h +++ b/samples/dataview/mymodels.h @@ -237,7 +237,7 @@ public: virtual wxString GetColumnType( unsigned int col ) const wxOVERRIDE { if (col == Col_ToggleIconText) - return wxDataViewCheckIconTextRenderer::GetDefaultType(); + return "wxIcon"; return "string"; } @@ -254,7 +254,7 @@ private: wxArrayString m_textColValues; wxArrayString m_iconColValues; IntToStringMap m_customColValues; - wxBitmapBundle m_icon[2]; + wxIcon m_icon[2]; }; // ----------------------------------------------------------------------------
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Thanks! Just for the record, here is an even more minimal diff sufficient to reproduce the problem:
diff --git a/samples/dataview/dataview.cpp b/samples/dataview/dataview.cpp
index 4b2f1d933e..4057a43959 100644
--- a/samples/dataview/dataview.cpp +++ b/samples/dataview/dataview.cpp @@ -904,7 +904,7 @@ void MyFrame::BuildDataViewCtrl(wxPanel* parent, unsigned int nPanel, wxDataViewColumn* const colCheckIconText = new wxDataViewColumn ( L"\u2714 + icon + text", - new wxDataViewCheckIconTextRenderer(),
+ new wxDataViewBitmapRenderer(),
MyListModel::Col_ToggleIconText,
wxCOL_WIDTH_AUTOSIZE
);
diff --git a/samples/dataview/mymodels.cpp b/samples/dataview/mymodels.cpp
index d6d76c2414..5aa6f20388 100644 --- a/samples/dataview/mymodels.cpp +++ b/samples/dataview/mymodels.cpp @@ -472,6 +472,11 @@ void MyListModel::GetValueByRow( wxVariant &variant, break; case Col_ToggleIconText: + if ( row % 2 ) + variant << m_icon[row % 2].GetBitmap(wxSize(16, 16)); // FIXME + else + variant = wxNullVariant; + break; { wxString text; wxCheckBoxState state;
(BTW, the "FIXME" comment indicates that we need to add operator<<(wxVariant, wxBitmapBundle)
, but this is not directly related to this issue).
What I am sure about is that it needs to be possible to represent "no value" in models and renderers somehow.
Something already working today is to override HasValue()
to return false for the cells without values, as shown by MyListStoreHasValueModel
in the sample. But this is another virtual function to override, of course, and I think it would be convenient to be able to just return null variant from GetValue()
instead. And, of course, we already have
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.
in GetValue()
documentation. OTOH being able to do this kind of makes HasValue()
unnecessary/redundant, so one might wonder what's the point of having it at all...
Anyhow, I'll try to make returning null variant work everywhere, but if you need something that already works right now/even with 3.1.6, HasValue()
might be a solution.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Oh wait, forget that. Overriding HasValue()
doesn't work in wxGTK neither and it's actually the same bug which prevents returning empty wxVariant
from working.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Please test #22409 and let me know if you still see any problems with it, including any performance regressions (wxDVC is horribly slow already in wxGTK, so it can be a bit difficult to notice, but do let me know if it becomes even slower with this change). TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
I don't understand this:
and I think it would be convenient to be able to just return null variant from GetValue() instead. And, of course, we already have
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.
How is "not returning a value" different from explicitly assigning wxNullVariant
? It's the exact same thing (per CheckedGetValue()
code), just explicit instead of implicit, no?
OTOH being able to do this kind of makes HasValue() unnecessary/redundant, so one might wonder what's the point of having it at all…
I didn't know about it, so thanks for pointing me to it. You're right there's redundancy, but it seems useful to have both for the ability to write easier-to-follow model code:
HasValue()
is useful for eliminating cells if the rules are simple (e.g. container row, empty data)HasValue()
- that's actually my case.—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Please test #22409 and let me know if you still see any problems with it, including any performance regressions (wxDVC is horribly slow already in wxGTK, so it can be a bit difficult to notice, but do let me know if it becomes even slower with this change). TIA!
Tested #22409, fixes the issue seen with Poedit 👍 No performance regression observed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
I don't understand this:
I think it's just due to clumsy wording on my part and we actually are saying the same thing.
and I think it would be convenient to be able to just return null variant from GetValue() instead.
This was meant to confirm that what is reported here is a bug and that we still need to support it instead of just telling people to override HasValue()
.
How is "not returning a value" different from explicitly assigning
wxNullVariant
? It's the exact same thing (perCheckedGetValue()
code), just explicit instead of implicit, no?
Yes, absolutely, it's the same thing and should work.
OTOH being able to do this kind of makes HasValue() unnecessary/redundant, so one might wonder what's the point of having it at all…
I didn't know about it, so thanks for pointing me to it. You're right there's redundancy, but it seems useful to have both for the ability to write easier-to-follow model code:
1. `HasValue()` is useful for eliminating cells if the rules are simple (e.g. container row, empty data) 2. Not returning value is useful if the logic for determining whether a value is shown is as complicated as the one for determining the value, and would have to be duplicated in `HasValue()` - that's actually my case.
Yes, I've also eventually convinced myself that it was sufficiently useful to have both to not update the documentation to recommend using one rather than the other.
@super7ramp Thanks for testing! I'll merge the other PR in a couple of days if there are no other comments about it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.