Fix desync button events (PR #25514)
alilie
When adding a button to wxPGMultiButton via Add(..., XRCID("text")) the text's corresponding id is added to XRCID_Records array, but the id gets reset in wxPGMultiButton::GenId and recalculated in wxWindowBase::CreateBase.
Avoid regenerating the id if itemid < 0.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25514
Commit Summary
- e7403fe Fix desync button events
File Changes
(1 file)
- M src/propgrid/editors.cpp (2)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25514.patch
- https://github.com/wxWidgets/wxWidgets/pull/25514.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
alilie
alilie left a comment (wxWidgets/wxWidgets#25514)I don't know if the current behavior is desirable, but in main the code like the one below does not work:
wxPGWindowList wxSampleMultiButtonEditor::CreateControls(...) {
...
wxPGMultiButton* buttons = new wxPGMultiButton( propGrid, sz );
buttons->Add( "..." , XRCID("wxPG_CUSTOM1"));
buttons->Add( "A" , XRCID("wxPG_CUSTOM2"));
buttons->Add( wxArtProvider::GetBitmap(wxART_FOLDER), XRCID("wxPG_CUSTOM3") );
...
wxPGWindowList wxSampleMultiButtonEditor::OnEvent(…) {
...
if ( event.GetId() == XRCID("wxPG_CUSTOM1") ) {
wxLogDebug("First button pressed");
return false; // Return false since value did not change
}
if ( event.GetId() == XRCID("wxPG_CUSTOM2") ) {
wxMessageBox("Second button pressed"); return false; // Return false since value did not change
}
if ( event.GetId() == XRCID("wxPG_CUSTOM3") ) {
wxMessageBox("Third button pressed"); return false; // Return false since value did not change
}
...
also patch:
diff --git a/samples/propgrid/propgrid.cpp b/samples/propgrid/propgrid.cpp index 8123ce5d0f..a303a7a3b2 100644 --- a/samples/propgrid/propgrid.cpp +++ b/samples/propgrid/propgrid.cpp @@ -110,11 +110,19 @@ wxPGWindowList wxSampleMultiButtonEditor::CreateControls( wxPropertyGrid* propGr // Create and populate buttons-subwindow wxPGMultiButton* buttons = new wxPGMultiButton( propGrid, sz ); +#if 1 + // Add two regular buttons + buttons->Add( "..." , XRCID("wxPG_CUSTOM1")); + buttons->Add( "A" , XRCID("wxPG_CUSTOM2")); + // Add a bitmap button + buttons->Add( wxArtProvider::GetBitmap(wxART_FOLDER), XRCID("wxPG_CUSTOM3") ); +#else // Add two regular buttons buttons->Add( "..." ); buttons->Add( "A" ); // Add a bitmap button buttons->Add( wxArtProvider::GetBitmap(wxART_FOLDER) ); +#endif // Create the 'primary' editor control (textctrl in this case) wxPGWindowList wndList = wxPGTextCtrlEditor::CreateControls @@ -138,24 +146,39 @@ bool wxSampleMultiButtonEditor::OnEvent( wxPropertyGrid* propGrid, { wxPGMultiButton* buttons = (wxPGMultiButton*) propGrid->GetEditorControlSecondary(); +#if 1 + if ( event.GetId() == XRCID("wxPG_CUSTOM1") ) + { + wxLogDebug("First button pressed"); + return false; // Return false since value did not change + } + if ( event.GetId() == XRCID("wxPG_CUSTOM2") ) + { + wxMessageBox("Second button pressed"); + return false; // Return false since value did not change + } + if ( event.GetId() == XRCID("wxPG_CUSTOM3") ) + { + wxMessageBox("Third button pressed"); + return false; // Return false since value did not change + } +#else if ( event.GetId() == buttons->GetButtonId(0) ) { - // Do something when the first button is pressed wxLogDebug("First button pressed"); return false; // Return false since value did not change } if ( event.GetId() == buttons->GetButtonId(1) ) { - // Do something when the second button is pressed wxMessageBox("Second button pressed"); return false; // Return false since value did not change } if ( event.GetId() == buttons->GetButtonId(2) ) { - // Do something when the third button is pressed wxMessageBox("Third button pressed"); return false; // Return false since value did not change } +#endif } return wxPGTextCtrlEditor::OnEvent(propGrid, property, ctrl, event); }
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25514)Sorry, I have no idea about this stuff (e.g. I've never used wxPGMultiButton myself), so any review/comments from people using this in their code would be really welcome!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tim Sobolev
Tim-Sobolev left a comment (wxWidgets/wxWidgets#25514)Avoid regenerating the id if itemid < 0.
You forgot to take into account that:
int wxPGMultiButton::GenId( int itemid ) const
{
return itemid < -1 ? wxID_ANY : itemid;
}
where :
wxID_ANY = -1,
and for whatever reason we have:
void Add( const wxString& label, int id = -2 );
void Add( const wxBitmapBundle& bitmap, int id = -2 );
in the "interface\wx\propgrid\editors.h" and "\include\wx\propgrid\editors.h".
And having a "magic" constant instead of enum member is a bad practice too...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25514)Sorry, I don't understand the point of that comment. Do you mean that this change is wrong?
FWIW -2 is wxID_SEPARATOR but I have no idea if it's intentional or accidental here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tim Sobolev
Tim-Sobolev left a comment (wxWidgets/wxWidgets#25514)As I understand the code, current implementation converts all ID below wxID_ANY to wxID_ANY.
So calls without specified ID, such as buttons->Add( "..."); will be used as buttons->Add( "...", wxID_ANY);
Proposed implementation will convert all such calls to buttons->Add( "...", wxID_SEPARATOR);
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25514)Yes, this is correct, but I have no idea what the original intention here was, i.e. why do we use wxID_SEPARATOR as the default value in the first place? It was like this ever since this code was added (see #9934), but without any explanation...
Maybe we should combine this change with the replacement of -2 with wxID_ANY? Or we could change GenId() to only replace -2 with wxID_ANY and not all negative IDs?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tim Sobolev
Tim-Sobolev left a comment (wxWidgets/wxWidgets#25514)I can guess that this was made to treat all negative ID as automatic and don't bother with checking if user ID is inside the range reserved for auto ids (wxID_LOWEST .. wxID_AUTO_HIGHEST).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25514)Actually, the more I think about it, the more I think that the best (because simplest and obvious) thing to do would be to just apply this:
diff --git a/include/wx/propgrid/editors.h b/include/wx/propgrid/editors.h index 5576a6ba5e..b651f7019a 100644 --- a/include/wx/propgrid/editors.h +++ b/include/wx/propgrid/editors.h @@ -473,9 +473,9 @@ public: // Returns number of buttons. unsigned int GetCount() const { return (unsigned int) m_buttons.size(); } - void Add( const wxString& label, int id = -2 ); + void Add( const wxString& label, int id = wxID_ANY ); #if wxUSE_BMPBUTTON - void Add( const wxBitmapBundle& bitmap, int id = -2 ); + void Add( const wxBitmapBundle& bitmap, int id = wxID_ANY ); #endif wxSize GetPrimarySize() const @@ -489,8 +489,6 @@ protected: void DoAddButton( wxWindow* button, const wxSize& sz ); - int GenId( int id ) const; - std::vector<wxWindow*> m_buttons; wxSize m_fullEditorSize; int m_buttonsWidth; diff --git a/interface/wx/propgrid/editors.h b/interface/wx/propgrid/editors.h index ad070ba0f0..b43946e0d5 100644 --- a/interface/wx/propgrid/editors.h +++ b/interface/wx/propgrid/editors.h @@ -548,12 +548,12 @@ public: /** Adds new button, with given label. */ - void Add( const wxString& label, int id = -2 ); + void Add( const wxString& label, int id = wxID_ANY ); /** Adds new bitmap button. */ - void Add( const wxBitmapBundle& bitmap, int id = -2 ); + void Add( const wxBitmapBundle& bitmap, int id = wxID_ANY ); /** Call this in CreateControls() of your custom editor class diff --git a/src/propgrid/editors.cpp b/src/propgrid/editors.cpp index 1d25f4e913..ee3332bfd9 100644 --- a/src/propgrid/editors.cpp +++ b/src/propgrid/editors.cpp @@ -2206,11 +2206,6 @@ void wxPGMultiButton::Finalize( wxPropertyGrid* WXUNUSED(propGrid), Move( pos.x + m_fullEditorSize.x - m_buttonsWidth, pos.y - wxPG_BUTTON_BORDER_WIDTH, wxSIZE_ALLOW_MINUS_ONE); } -int wxPGMultiButton::GenId( int itemid ) const -{ - return itemid < -1 ? wxID_ANY : itemid; -} - #if wxUSE_BMPBUTTON #if defined(__WXGTK__) @@ -2266,7 +2261,6 @@ typedef wxBitmapButton wxPGEditorBitmapButton; void wxPGMultiButton::Add( const wxBitmapBundle& bitmap, int itemid ) { - itemid = GenId(itemid); wxSize sz = GetSize(); // Internal margins around the bitmap inside the button @@ -2311,7 +2305,6 @@ void wxPGMultiButton::Add( const wxBitmapBundle& bitmap, int itemid ) void wxPGMultiButton::Add( const wxString& label, int itemid ) { - itemid = GenId(itemid); wxSize sz = GetSize(); wxButton* button = new wxButton(this, itemid, label, wxPoint(sz.x, 0), wxSize(wxDefaultCoord, sz.y), wxBU_EXACTFIT);
@alilie Do you agree with replacing your PR with this diff?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
alilie
alilie left a comment (wxWidgets/wxWidgets#25514)Do you agree with replacing your PR with this diff?
Of course.
I made the PR more to report the problem. I don't know the wx codebase as well as you do. :)
Do you want me to apply the diff above in a commit in this PR or do you do it separately and close the current PR?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25514)If you don't see any problems with this diff when testing your application with it, I'll just push it directly and close this PR, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
alilie
alilie left a comment (wxWidgets/wxWidgets#25514)I applied your diff and it seems to be working. Thank you!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.