wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
6.21k stars 1.78k forks source link

wxGrid cell editors do not appear sometimes on wxGTK. #22628

Closed dsa-t closed 2 years ago

dsa-t commented 2 years ago

On wxGTK, if there's a dialog with an editable wxGrid inside a wxNotebook, sometimes the grid cell editor do not appear when clicked on. The cell becomes white, but values can still be typed.

If the dialog window is resized, or an obscure sequence of actions happen, the cell editors re-appear.

This is a long-standing issue in KiCad (Although it's closed after many workarounds applied, the bug is still happening).


Here's a wxPython script that reproduces the problem with about 50% chance for me:

grid-cell-editor-bug.py.txt / gist

Here's a video with Linux Mint 20.3, wxPython 4.2.0a1, XFCE, Mint-Y-Aqua theme. Notice that when the mouse is hovered over a different wxNotebook tab, then over the buttons at the bottom, the bug resolves until the dialog is re-opened.

Platform and version information

vadz commented 2 years ago

Thanks for reporting this, this definitely looks like a wx (and probably wxGTK) bug but I had never seen it before. I'll try to find time to take a look at it, but it would be great to have an example reproducing it in C++ for debugging.

Can the problem be reproduced in the "Bugs" grid of the grid sample by chance? Maybe after making BugsGridFrame a wxDialog instead of wxFrame?

If not, can the existing Python example be simplified/reduced to the bare minimum needed to show the bug? This would make it simpler/faster for me to translate it to C++ for debugging.

TIA!

dsa-t commented 2 years ago

If I move the wxGrid out of the wxNotebook, the bug doesn't seem to appear. I don't see the bug in the Bugs window of grid sample.

Here's the fbp that was used as a basis: https://gitlab.com/kicad/code/kicad/-/blob/6.0/eeschema/dialogs/dialog_symbol_properties_base.fbp

Here are the relevant files: https://gitlab.com/kicad/code/kicad/-/blob/6.0/eeschema/dialogs/dialog_symbol_properties_base.cpp https://gitlab.com/kicad/code/kicad/-/blob/master/eeschema/dialogs/dialog_symbol_properties_base.h

https://gitlab.com/kicad/code/kicad/-/blob/6.0/eeschema/dialogs/dialog_symbol_properties.cpp https://gitlab.com/kicad/code/kicad/-/blob/6.0/eeschema/dialogs/dialog_symbol_properties.h

https://gitlab.com/kicad/code/kicad/-/blob/6.0/eeschema/fields_grid_table.cpp https://gitlab.com/kicad/code/kicad/-/blob/6.0/eeschema/fields_grid_table.h

sethhillbrand commented 2 years ago

Note also that the bug is very dependent on the GTK theme used. Some themes seem to trigger it more easily than others.

vadz commented 2 years ago

Just putting it inside a notebook is definitely not enough to trigger the bug, e.g. everything works fine for me with

this patch ```diff diff --git a/samples/grid/griddemo.cpp b/samples/grid/griddemo.cpp index dd6309dbe9..93a8af549e 100644 --- a/samples/grid/griddemo.cpp +++ b/samples/grid/griddemo.cpp @@ -30,6 +30,7 @@ #include "wx/grid.h" #include "wx/headerctrl.h" +#include "wx/notebook.h" #include "wx/generic/gridctrl.h" #include "wx/generic/grideditors.h" @@ -2260,7 +2261,9 @@ wxString BugsGridTable::GetColLabelValue( int col ) BugsGridFrame::BugsGridFrame() : wxFrame(NULL, wxID_ANY, "Bugs table") { - wxGrid *grid = new wxGrid(this, wxID_ANY); + auto book = new wxNotebook(this, wxID_ANY); + + wxGrid *grid = new wxGrid(book, wxID_ANY); wxGridTableBase *table = new BugsGridTable(); table->SetAttrProvider(new MyGridCellAttrProvider); grid->AssignTable(table); @@ -2278,8 +2281,7 @@ BugsGridFrame::BugsGridFrame() grid->SetColAttr(Col_Priority, attrRangeEditor); grid->SetColAttr(Col_Severity, attrCombo); - grid->Fit(); - SetClientSize(grid->GetSize()); + book->AddPage(grid, "Grid"); } // ============================================================================ ```

I've tested this on my usual Debian system (using bog standard Gnome with Adwaita) and under Ubuntu 20.04, where this is supposed not to work (with the default Ubuntu theme).

I'll try to return to this and make a self-contained example based on Kicad code above, but there is really a lot of it and it would be great if someone who does see the bug could strip it down to just the required minimum.

dsa-t commented 2 years ago

I think it's enough to just copy the combination of wxDialog + wxNotebook + wxGrid.

The second tab (empty panel in the script) and the buttons are just to be able to see the "hovering sequence".

Data in the wxGrid doesn't matter. The presence of a column with checkboxes doesn't matter (it can be ommited or replaced with text).

vadz commented 2 years ago

I think it's enough to just copy the combination of wxDialog + wxNotebook + wxGrid.

Unfortunately it isn't enough for me, i.e. if I do this:

diff --git a/samples/grid/griddemo.cpp b/samples/grid/griddemo.cpp
index dd6309dbe9..32de484b8a 100644
--- a/samples/grid/griddemo.cpp
+++ b/samples/grid/griddemo.cpp
@@ -30,6 +30,7 @@

 #include "wx/grid.h"
 #include "wx/headerctrl.h"
+#include "wx/notebook.h"
 #include "wx/generic/gridctrl.h"
 #include "wx/generic/grideditors.h"

@@ -1887,8 +1888,8 @@ void GridFrame::OnQuit( wxCommandEvent& WXUNUSED(ev) )

 void GridFrame::OnBugsTable(wxCommandEvent& )
 {
-    BugsGridFrame *frame = new BugsGridFrame;
-    frame->Show(true);
+    BugsGridFrame frame;
+    frame.ShowModal();
 }

 // ----------------------------------------------------------------------------
@@ -2258,9 +2259,11 @@ wxString BugsGridTable::GetColLabelValue( int col )
 // ----------------------------------------------------------------------------

 BugsGridFrame::BugsGridFrame()
-             : wxFrame(NULL, wxID_ANY, "Bugs table")
+             : wxDialog(NULL, wxID_ANY, "Bugs table")
 {
-    wxGrid *grid = new wxGrid(this, wxID_ANY);
+    auto book = new wxNotebook(this, wxID_ANY);
+
+    wxGrid *grid = new wxGrid(book, wxID_ANY);
     wxGridTableBase *table = new BugsGridTable();
     table->SetAttrProvider(new MyGridCellAttrProvider);
     grid->AssignTable(table);
@@ -2278,8 +2281,7 @@ BugsGridFrame::BugsGridFrame()
     grid->SetColAttr(Col_Priority, attrRangeEditor);
     grid->SetColAttr(Col_Severity, attrCombo);

-    grid->Fit();
-    SetClientSize(grid->GetSize());
+    book->AddPage(grid, "Grid");
 }

 // ============================================================================
diff --git a/samples/grid/griddemo.h b/samples/grid/griddemo.h
index 9acc11b1b5..fb8eea4b38 100644
--- a/samples/grid/griddemo.h
+++ b/samples/grid/griddemo.h
@@ -342,7 +342,7 @@ public:
     virtual void SetValueAsBool( int row, int col, bool value ) wxOVERRIDE;
 };

-class BugsGridFrame : public wxFrame
+class BugsGridFrame : public wxDialog
 {
 public:
     BugsGridFrame();

I still don't see the problem.

Could you please check if you can reproduce it with the patch above, i.e. if the difference is just the theme or if, as I suspect, something is missing from the diff above (in which case it would, of course, be great, if you could find what it is, exactly)? TIA!

dsa-t commented 2 years ago

I can reproduce the bug at 100% rate with the below patch. If I add wxRESIZE_BORDER to wxDialog style, the rate drops to 10-20%.

The sizers seem to be important here. And the fact that grid->AssignTable(table); is called at the end (In KiCad, SetTable is called in the child class).

diff --git a/samples/grid/griddemo.cpp b/samples/grid/griddemo.cpp
index dd6309d..ac69fa3 100644
--- a/samples/grid/griddemo.cpp
+++ b/samples/grid/griddemo.cpp
@@ -30,6 +30,7 @@

 #include "wx/grid.h"
 #include "wx/headerctrl.h"
+#include "wx/notebook.h"
 #include "wx/generic/gridctrl.h"
 #include "wx/generic/grideditors.h"

@@ -1887,8 +1888,8 @@ void GridFrame::OnQuit( wxCommandEvent& WXUNUSED(ev) )

 void GridFrame::OnBugsTable(wxCommandEvent& )
 {
-    BugsGridFrame *frame = new BugsGridFrame;
-    frame->Show(true);
+    BugsGridFrame frame;
+    frame.ShowModal();
 }

 // ----------------------------------------------------------------------------
@@ -2258,28 +2259,39 @@ wxString BugsGridTable::GetColLabelValue( int col )
 // ----------------------------------------------------------------------------

 BugsGridFrame::BugsGridFrame()
-             : wxFrame(NULL, wxID_ANY, "Bugs table")
+             : wxDialog(NULL, wxID_ANY, "Bugs table", wxDefaultPosition, wxSize(380, 250), wxDEFAULT_DIALOG_STYLE)
 {
-    wxGrid *grid = new wxGrid(this, wxID_ANY);
-    wxGridTableBase *table = new BugsGridTable();
-    table->SetAttrProvider(new MyGridCellAttrProvider);
-    grid->AssignTable(table);
+    this->SetSizeHints( wxDefaultSize, wxDefaultSize );
+
+    wxBoxSizer* mainSizer;
+    mainSizer = new wxBoxSizer( wxVERTICAL );
+
+    auto m_notebook1 = new wxNotebook( this, wxID_ANY, wxDefaultPosition, wxDefaultSize, 0 );
+    auto generalPage = new wxPanel( m_notebook1, wxID_ANY, wxDefaultPosition, wxDefaultSize, wxTAB_TRAVERSAL );
+
+    wxStaticBoxSizer* sbFields = new wxStaticBoxSizer( new wxStaticBox( generalPage, wxID_ANY, _("Fields") ), wxVERTICAL );

-    wxGridCellAttr *attrRO = new wxGridCellAttr,
-                   *attrRangeEditor = new wxGridCellAttr,
-                   *attrCombo = new wxGridCellAttr;
+    wxGrid *grid = new wxGrid(sbFields->GetStaticBox(), wxID_ANY);
+    sbFields->Add( grid, 1, wxEXPAND|wxBOTTOM|wxRIGHT|wxLEFT, 5 );

-    attrRO->SetReadOnly();
-    attrRangeEditor->SetEditor(new wxGridCellNumberEditor(1, 5));
-    attrCombo->SetEditor(new wxGridCellChoiceEditor(WXSIZEOF(severities),
-                                                    severities));
+    wxBoxSizer* generalPageSizer = new wxBoxSizer( wxVERTICAL );
+    generalPageSizer->Add( sbFields, 1, wxEXPAND|wxBOTTOM|wxRIGHT|wxLEFT, 5 );

-    grid->SetColAttr(Col_Id, attrRO);
-    grid->SetColAttr(Col_Priority, attrRangeEditor);
-    grid->SetColAttr(Col_Severity, attrCombo);
+    generalPage->SetSizer( generalPageSizer );
+    generalPage->Layout();
+    generalPageSizer->Fit( generalPage );
+    m_notebook1->AddPage( generalPage, _("General"), true );

-    grid->Fit();
-    SetClientSize(grid->GetSize());
+    mainSizer->Add( m_notebook1, 1, wxEXPAND|wxTOP|wxRIGHT|wxLEFT, 5 );
+
+    this->SetSizer( mainSizer );
+    this->Layout();
+    mainSizer->Fit( this );
+
+
+    wxGridTableBase *table = new BugsGridTable();
+    table->SetAttrProvider(new MyGridCellAttrProvider);
+    grid->AssignTable(table);
 }

 // ============================================================================
diff --git a/samples/grid/griddemo.h b/samples/grid/griddemo.h
index 9acc11b..fb8eea4 100644
--- a/samples/grid/griddemo.h
+++ b/samples/grid/griddemo.h
@@ -342,7 +342,7 @@ public:
     virtual void SetValueAsBool( int row, int col, bool value ) wxOVERRIDE;
 };

-class BugsGridFrame : public wxFrame
+class BugsGridFrame : public wxDialog
 {
 public:
     BugsGridFrame();
vadz commented 2 years ago

Perfect, thank you, I can see it too now, will debug.

vadz commented 2 years ago

I have a very ugly workaround which solves the problem for me:

diff --git a/src/generic/grideditors.cpp b/src/generic/grideditors.cpp
index e122b44238..51bdcf263c 100644
--- a/src/generic/grideditors.cpp
+++ b/src/generic/grideditors.cpp
@@ -329,7 +329,7 @@ void wxGridCellEditor::SetSize(const wxRect& rect)
 {
     wxASSERT_MSG(m_control, wxT("The wxGridCellEditor must be created first!"));

-    m_control->SetSize(rect, wxSIZE_ALLOW_MINUS_ONE);
+    m_control->SetSize(rect, wxSIZE_ALLOW_MINUS_ONE | wxSIZE_FORCE);
 }

 void wxGridCellEditor::DoPositionEditor(const wxSize& size,
diff --git a/src/gtk/window.cpp b/src/gtk/window.cpp
index 913a7ae51a..4989f1208a 100644
--- a/src/gtk/window.cpp
+++ b/src/gtk/window.cpp
@@ -3956,6 +3956,13 @@ void wxWindowGTK::DoSetSize( int x, int y, int width, int height, int sizeFlags
         DoMoveWindow(x, y, width, height);
     }

+    if ( sizeFlags & wxSIZE_FORCE )
+    {
+        wxWindow *parent = wxGetTopLevelParent((wxWindow *)this);
+        if ( parent )
+            gtk_widget_queue_resize(parent->m_wxwindow);
+    }
+
     if (((sizeChange
 #ifdef __WXGTK3__
                      || m_needSizeEvent

This should not be applied in its current state, of course, but could you please check if doing this also solves the problem in KiCAD?

I think we do need to force triggering size allocation somehow because our pizza_size_allocate() just doesn't get called at all when this bug happens. The problem is that I still don't understand why does it get called sometimes but not always, but I'll try to find it by comparing what happens in working and non-working cases. In the meanwhile, if we could at least be sure that this is the same/only problem, it would already be reassuring, so please let me know if you still see any problems even after applying the patch above -- TIA!

@paulcor If you have any ideas about why size allocation isn't redone in this case, I'd definitely appreciate them.

voneiden commented 2 years ago

@vadz I'm just a KiCAD end user so I have no idea what's happening under the hood but I applied the ugly workaround against wxGTK-3.0.5.1 on gentoo. It improved things partially (thanks!) but not fully.

Before the patch, these three inputs (value, footprint, datasheet) work only if I resize the window:

https://user-images.githubusercontent.com/437576/187090619-d74a5a40-5d9f-464d-9313-fe160da6e4e8.mp4

After the patch, all inputs start to work once I click the value field. The two input fields that don't work (footprint and datasheet) have a button on the right side:

https://user-images.githubusercontent.com/437576/187090628-9fae41e4-e8ac-45e1-b9dc-8221acc45610.mp4

vadz commented 2 years ago

Thanks for testing and sorry to hear it still doesn't work, but a lot of things have changed since 3.0.5, it would be interesting to know if you still see the problem with f294083c24cd78d587cd3ba11358cba6c2122951 applied to 3.2.0?

voneiden commented 2 years ago

@vadz Gentoo ecosystem is still stuck with 3.0.5, but I managed to grab 3.2.0 and compile KiCAD v6.0.7 against it. It seems I'm unable to reproduce the issue at all with 3.2.0 - the inputs work as expected. (Adwaita theme, AwesomeWM, X11). So in my case it seems the patch is not necessary.

@dsa-t mentioned in the issue replicates with 3.2.0-RC1? IDK, maybe there are some other variables at play too? There was some speculation that GTK theme could play a role in the KiCAD issues.

dsa-t commented 2 years ago

It reproduced in the wxPython script with 3.2.0 RC1 and 3.0.4.

Then with the recompiled regular wxWidgets, I could't see the bug in the KiCad master branch (or I didn't test enough times), so I'm not sure what affects it.

vadz commented 2 years ago

@dsa-t Sorry, what do you mean by "regular wxWidgets"? I'm mostly interested if it can still be reproduced with the current master, i.e. with f294083c24cd78d587cd3ba11358cba6c2122951

dsa-t commented 2 years ago

Strangely enough, even with 3.0.4+dfsg-15build1 libs and KiCad 6.0 branch I can't reproduce the bug at this moment via VNC, nor in the modified grid sample. I've changed some vsync/tearing settings and switched from DRI2 to DRI3 recently, so that might've affected it.

The "regular wxWidgets" was the wxWidgets master branch before https://github.com/wxWidgets/wxWidgets/commit/f294083c24cd78d587cd3ba11358cba6c2122951 I think.

paulcor commented 2 years ago

I believe this is fixed in master, and now also in 3.2.1. Please re-open if you can still reproduce the problem.