wxGrid: implement wxGridMoveEvent (PR #24392)

Dietmar Schwertberger

unread,

Mar 10, 2024, 2:10:18 PMMar 10

to wx-...@googlegroups.com, Subscribed

When working with EVT_GRID_ROW_MOVE I noticed two problems:

When I did PR #22260, in wxGrid::DoEndMoveRow(int pos) I made a mistake with the arguments to SendEvent(wxEVT_GRID_ROW_MOVE, -1, m_dragMoveRow).
Therefore, in an event handler event.GetRow() would always return -1 and event.GetCol() needs to be used instead.

The bigger issue is that there is no easy way to retrieve the target row number.

This PR adds a new event class wxGridMoveEvent which allows retrieveal of old and new positions using GetRowOrCol() and GetNewRowOrCol().

The huge disadvantage of this approach is that existing C++ code needs to change the argument type of event handlers.
E.g. in the grid sample:
void OnGridColMove(wxGridEvent& event) -> void OnGridRowMove(wxGridMoveEvent& event)

I'm not a C++ guy. When creating the new class, I just copied the code from wxGridSizeEvent. I was surprised that there is no inheritance between the different grid event classes. Both wxGridEvent and wxGridSizeEvent are derived from wxNotifyEvent.
Could wxGridMoveEvent be changed to be backward compatible or would that break something else?
(If not, I would remove the GetCol() method from the PR as at the moment it's a bit pointless.)

You can view, comment on, or merge this pull request online at:

https://github.com/wxWidgets/wxWidgets/pull/24392

Commit Summary

52dd352 wxGrid: implement wxGridMoveEvent

File Changes

(5 files)

Patch Links:

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

May 19, 2024, 6:48:30 PMMay 19

to wx-...@googlegroups.com, Subscribed

Sorry for the delay with looking at this.

I think that even though using wxGridMoveEvent would be a better approach when adding these events, it doesn't justify breaking the existing code using them by now. So instead of doing this, let's just reuse the m_col/m_row of wxGridEvent for the new value and add GetNew{Row,Col}() to retrieve them. This is a bit ugly, but will allow the existing code to keep working and so is IMO preferable unless there are any problems with doing it like this that I'm missing?

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

You are receiving this because you are subscribed to this thread.

Dietmar Schwertberger

unread,

May 20, 2024, 11:36:10 AMMay 20

to wx-...@googlegroups.com, Subscribed

By re-using m_col/m_row we would lose the information about the old position.
So, certainly m_newCol / m_newRow would be needed, right?

Currently the code looks like this:

void wxGrid::DoEndMoveRow(int pos)
{
    wxASSERT_MSG( m_dragMoveRowOrCol != -1, "no matching DoStartMoveRow?" );
    if ( SendEvent(wxEVT_GRID_ROW_MOVE, -1, m_dragMoveRowOrCol) != Event_Vetoed )
        SetRowPos(m_dragMoveRowOrCol, pos);
    m_dragMoveRowOrCol = -1;
}

wxGrid::EventResult
wxGrid::SendEvent(wxEventType type, int row, int col, const wxString& s)
{
    wxGridEvent gridEvt( GetId(), type, this, row, col );
    gridEvt.SetString(s);

    return DoSendEvent(gridEvt);
}

I would suggest to create the event, set the target information and call DoSendEvent directly from DoEndMoveRow / DoEndMoveRow instead of another SendEvent overload.

The dirtiest possible hack would be to mis-use the string argument of SendEvent...

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

You are receiving this because you are subscribed to this thread.

VZ

unread,

May 20, 2024, 11:49:08 AMMay 20

to wx-...@googlegroups.com, Subscribed

By re-using m_col/m_row we would lose the information about the old position.

I meant to reuse the other one, i.e. when moving rows we currently always have m_col == -1 AFAICS. We could set it to the new row location instead.

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

You are receiving this because you are subscribed to this thread.

Dietmar Schwertberger

unread,

May 20, 2024, 12:06:59 PMMay 20

to wx-...@googlegroups.com, Subscribed

Ah, OK. That would be fine for me, even though a bit strange.
Probably I should close this PR and submit a separate one.

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

You are receiving this because you are subscribed to this thread.

VZ

unread,

May 20, 2024, 12:14:20 PMMay 20

to wx-...@googlegroups.com, Subscribed

Yes, as I said, this is definitely ugly, but we'll just call it "implementation detail"...

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

You are receiving this because you are subscribed to this thread.

Dietmar Schwertberger

unread,

May 20, 2024, 12:25:16 PMMay 20

to wx-...@googlegroups.com, Subscribed

Closed #24392.

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

You are receiving this because you are subscribed to this thread.

Dietmar Schwertberger

unread,

May 20, 2024, 12:25:19 PMMay 20

to wx-...@googlegroups.com, Subscribed

Thanks. A new PR should follow in the next days.

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

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward