Cells without values not correctly shown in wxDataViewCtrl with wxGTK 3.1.6 (Issue #22359)

43 views
Skip to first unread message

Antoine Belvire

unread,
Apr 24, 2022, 3:30:36 PM4/24/22
to wx-...@googlegroups.com, Subscribed

Description

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.

Expected vs observed behaviour

Expected behaviour (wxGTK 3.1.5):

The warning/comment icons are correctly displayed.

https://user-images.githubusercontent.com/9394745/163816494-01653885-1d0a-442a-8507-6b89020ea8ab.mp4

Actual behaviour (wxGTK 3.1.6):

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

Patch or snippet allowing to reproduce the problem

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.

To Reproduce

Assuming Poedit 3.0.1 is installed under the environment mentioned below:

  1. Decompress and open the following translation file: zypper-master-fr.po.gz
  2. (Optional) In order to get the same string ordering as in the screencasts above, configure View with the following:
    • Show warnings
    • Sort by file order
    • Group by context
    • Entries with error first
    • Untranslated entries first
  3. Scroll down the string list

Platform and version information

  • wxWidgets version: 3.1.6
  • wxWidgets port: wxGTK
  • OS and its version: openSUSE Tumbleweed (20220420)
  • For wxGTK only:
    • GTK version: 3.24.33
    • Which GDK backend is used: Wayland


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/22359@github.com>

VZ

unread,
May 5, 2022, 9:22:11 PM5/5/22
to wx-...@googlegroups.com, Subscribed

@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.Message ID: <wxWidgets/wxWidgets/issues/22359/1119181494@github.com>

Václav Slavík

unread,
May 6, 2022, 6:52:08 AM5/6/22
to wx-...@googlegroups.com, Subscribed

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:

  • macOS: correct (odd rows with icon, even without)
  • wxGTK: wrong (all rows have icons)
  • Windows: didn't test

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.Message ID: <wxWidgets/wxWidgets/issues/22359/1119490610@github.com>

VZ

unread,
May 7, 2022, 1:16:05 PM5/7/22
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/issues/22359/1120243692@github.com>

VZ

unread,
May 7, 2022, 5:45:31 PM5/7/22
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/issues/22359/1120297940@github.com>

VZ

unread,
May 7, 2022, 6:42:47 PM5/7/22
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/issues/22359/1120311248@github.com>

Václav Slavík

unread,
May 8, 2022, 6:26:52 AM5/8/22
to wx-...@googlegroups.com, Subscribed

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:

  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.


Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issues/22359/1120391596@github.com>

Antoine Belvire

unread,
May 8, 2022, 7:00:55 AM5/8/22
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/issues/22359/1120396568@github.com>

VZ

unread,
May 8, 2022, 10:23:55 AM5/8/22
to wx-...@googlegroups.com, Subscribed

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 (per CheckedGetValue() 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.Message ID: <wxWidgets/wxWidgets/issues/22359/1120428121@github.com>

VZ

unread,
May 11, 2022, 11:31:04 AM5/11/22
to wx-...@googlegroups.com, Subscribed

Closed #22359 via #22409.


Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/issue/22359/issue_event/6591539327@github.com>

Reply all
Reply to author
Forward
0 new messages