wxGrid: allow drag-resizing row and col labels #24362

DietmarSchwertberger · 2024-02-27T17:59:36Z

This is a first version to add a feature that has been on my wish list for years.

This first version only allows dragging the edges of the corner label.

I'm not sure whether this is good enough or it should also be possible to drag the edges of the whole label windows.
When you look at the modifications, you can see that the modifications in ProcessRowColLabelMouseEvent are rather simple compared to ProcessCornerLabelMouseEvent where both directions have to be handled.

The new methods and events are not yet in the interface header files. I will add them as soon as the API modifications are agreed:

// for grid:
int GetRowLabelMinimalSize();
int GetColLabelMinimalSize();
SetRowLabelMinimalSize( int width );
SetColLabelMinimalSize( int height );

EnableDragRowLabelSize(bool enable = true);
DisableDragRowLabelSize();
EnableDragColLabelSize(bool enable = true);
DisableDragColLabelSize();

bool CanDragRowLabelSize();
bool CanDragColLabelSize();

EVT_GRID_ROW_LABEL_SIZE
EVT_GRID_COL_LABEL_SIZE

// for wxGridOperations:
bool CanDragLabelSize(wxGrid* grid);
void SetLabelSize(wxGrid* grid, int size);
int GetLabelSize(wxGrid* grid);
int GetMinimalLabelSize(wxGrid* grid);

Internally, a m_dragRowOrCol value -2 is used:

    // or -2 if row or col label is being resized
    // or -1 if there is no resize operation in progress.
    int     m_dragRowOrCol;

To be decided:

should it be enabled by default? I would say yes.
implement for more than the corner label edges? Probably yes, but the code would be more complicated.
I was wondering about event.Skip() calls. I have added it to the case of event.Moving() inside ProcessCornerLabelMouseEvent.
Inside ProcessRowColLabelMouseEvent this is not done now. I'm wondering what is the correct way and whether event.Skip() should also be added to e.g. event.Leaving().

DietmarSchwertberger · 2024-02-28T12:53:10Z

OK, dragging the edge towards the grid is now working as well.
The code is less ugly than expected.
It still needs to be implemented for the grid area itself as well and also requires some debugging with frozen rows/columns.

DietmarSchwertberger · 2024-02-28T16:29:07Z

Dragging the label edges is now working also when inside the grid cell window.

I have removed this line from DoGridCellLeftDown:
wxCHECK_RET( dragRowOrCol != -1, "Can't determine row or column in resizing mode" );

This seems in line with the current implementation on relying that the cursor mode was set in DoGridMouseMoveEvent

To be done:

update interface/grid.h once the API is agreed.
some fine tuning (sometimes the resizing does not start, probably when pressing the mouse button and moving at the same time

…e starts at x or y = 0

DietmarSchwertberger · 2024-02-29T16:00:21Z

The documentation is now updated.
To be decided:

leave drag-resizing on by default?
leave minimum size at 0 by default?
where is event.Skip() required?

vadz

First of all, thanks for implementing this!

The documentation is now updated.

Thanks!

To be decided:

* leave drag-resizing on by default?

I'm not sure but AFAIK this is typically not something allowed by default. Personally, I think it's most useful for the row labels and not so much for the column ones, but it wouldn't make much sense to only allow one and not the other, which is another argument in favour of not allowing this by default and telling people to enable it in the directions they need it.

* leave minimum size at 0 by default?

If we enable it by default, it definitely shouldn't be 0 because this would result in complains from users hiding the labels entirely when this was never intended by the developer. If we don't, we could make it 0 by default but mention that you may want to change this when enabling it.

* where is `event.Skip()` required?

It should definitely be used in the leaving/entering events, there is no reason to stop handling them.

vadz · 2024-03-03T14:14:47Z

src/generic/grid.cpp

+    bool atBottomEdge = false, atRightEdge = false, atLabelEdge = false;
+    const wxPoint posEvent = event.GetPosition();
+    int edge = FromDIP(WXGRID_LABEL_EDGE_ZONE);
+    const wxGridOperations* oper = nullptr;


This is a memory leak AFAICS because oper is never deleted. Please use

Suggested change

const wxGridOperations* oper = nullptr;

std::unique_ptr<wxGridOperations> oper;

instead.

vadz · 2024-03-03T14:16:31Z

src/generic/grid.cpp

-                                : static_cast<const wxGridOperations&>(wxGridColumnOperations())
-                            ).SetLineSize(this, m_dragRowOrCol, m_dragRowOrColOldSize);
+                            if ( m_dragRowOrCol == -2 )
+                            {   if ( m_cursorMode == WXGRID_CURSOR_RESIZE_ROW )


Some strange indentation here

Suggested change

{ if ( m_cursorMode == WXGRID_CURSOR_RESIZE_ROW )

{

if ( m_cursorMode == WXGRID_CURSOR_RESIZE_ROW )

vadz · 2024-03-03T14:17:19Z

src/generic/grid.cpp

@@ -8726,6 +8885,19 @@ void wxGrid::SetColLabelSize( int height )
    }
 }

+void wxGrid::SetRowLabelMinimalSize(int width)
+{
+    if ( width >= 0 )


Does it really make sense to pass negative width here? If not, we should replace this with

Suggested change

if ( width >= 0 )

wxCHECK_RET( width >= 0, "invalid min row label width" );

vadz · 2024-03-03T14:19:06Z

include/wx/generic/grid.h

@@ -2659,6 +2680,7 @@ class WXDLLIMPEXP_CORE wxGrid : public wxScrolledCanvas
    const wxColour *m_dragLastColour;

    // Row or column (depending on m_cursorMode value) currently being resized
+    // or -2 if row or col label is being resized


We really ought to have a symbolic constant for this instead of repeating this -2 many times.

But, to be honest, I rather dislike having such multiple-state variable. Having an int which can be invalid is not too bad, but having an int with 3 different kinds of values is not great. Could things become clearer if we introduced a separate bool m_dragLabel?

DietmarSchwertberger · 2024-03-03T15:50:43Z

Thanks for the review. I will apply the suggested modifications and also try the variant with a separate m_dragLabel.
When the changes are in the repository, I will also try with my wxPython code whether I still get the events that I got before.

I hope this is correct - at least it compiles:

std::unique_ptr<wxGridOperations> oper;
...
oper = (std::unique_ptr<wxGridOperations>) (new wxGridRowOperations());

vadz · 2024-03-03T15:54:49Z

I hope this is correct - at least it compiles:

std::unique_ptr<wxGridOperations> oper;
...
oper = (std::unique_ptr<wxGridOperations>) (new wxGridRowOperations());

This is actually correct but very strange. Please use either

oper = std::unique_ptr<wxGridOperations>(new wxGridRowOperations());

which does exactly the same thing but looks better or, to avoid creation of another temporary pointer,

oper.reset(new wxGridRowOperations());

TIA!

DietmarSchwertberger · 2024-03-03T16:08:13Z

Thanks. The oper.reset(...) variant looks nice.
Strange enough, I did not find this use case of a delayed creation in my C++ book or online.

DietmarSchwertberger · 2024-03-03T16:24:24Z

The advantage of using m_dragRowOrCol = -2 was that I could re-use e.g. DoStartResizeRowOrCol(col, size) without changes.

DietmarSchwertberger · 2024-03-03T22:12:42Z

With 324430b now m_dragLabel is being used.

DoGridDragResize is still shared. I'm not sure whether some of the other methods should be shared as well.

DietmarSchwertberger · 2024-03-04T19:57:01Z

The last two commits do simplify grid again a bit, but complicate GridOperations.
The same could probably be done for DoEndDragResizeRow and DoEndDragResizeCol.

What do you think?

Still open: for which events to call Skip()

DietmarSchwertberger · 2024-03-04T21:40:46Z

The last commits somewhat reduce complexity.

At many points there is code like

oper = (m_cursorMode == WXGRID_CURSOR_RESIZE_ROW ?  (wxGridOperations*) new wxGridRowOperations() :  wxGridOperations*) new wxGridColumnOperations());

I think a helper function DoGetOperationsFromCursorMode should be introduced, or when setting the cursor mode, an attribute like m_cursorOperations should be set.
Actually, the grid is complex enough that it would have deserved a proper state machine...

DietmarSchwertberger · 2024-03-04T22:53:54Z

OK, a helper like this could be used at four points:

wxGridOperations* wxGrid::DoGetOperationsFromCursorMode()
{
    if ( m_cursorMode == WXGRID_CURSOR_RESIZE_ROW )
        return new wxGridRowOperations();
    if ( m_cursorMode == WXGRID_CURSOR_RESIZE_COL )
        return new wxGridColumnOperations();
    return nullptr;
}

The number of lines is not really reduced due to declaration and implementation of the helper, but still I prefer the simpler code.

I don't like either of these usage options, though:

{
    const wxGridOperations* oper = DoGetOperationsFromCursorMode();
    ...
    delete oper;
}

{
    std::unique_ptr<wxGridOperations> oper(DoGetOperationsFromCursorMode());
    ...
}

Do you see a better option? The nullptr is not used here and oper always should be destroyed when it goes out of scope.

vadz · 2024-03-04T23:22:32Z

Sorry, didn't have time to look at the new code yet, but just 2 comments:

Still open: for which events to call Skip()

Sorry, which events are we speaking about here? I saw the question about the mouse ones and I answered it (you should call Skip() for them), are there others?

Do you see a better option? The nullptr is not used here and oper always should be destroyed when it goes out of scope.

Yes, I definitely do: DoGetOperationsFromCursorMode() must return (a possibly empty) std::unique_ptr<wxGridOperations> instead of raw pointer. It would be used as

const auto oper(DoGetOperationsFromCursorMode());
...
// no need for deleting anything and no danger of memory leaks

DietmarSchwertberger · 2024-03-05T17:24:20Z

Thanks. I think I have now all infos and will modify accordingly.
Yes, the events in question are the mouse events only. I will compare the handling in the different windows.
I think I can finalize the PR only at the weekend. I will set to draft for the time being.

DietmarSchwertberger · 2024-03-12T21:04:29Z

About event handling and event.Skip():

ProcessGridCellMouseEvent is calling event.Skip() only in some cases at the end, but e.g. not on Entering or Leaving.
E.g. for event.Moving() it's calling DoGridMouseMoveEvent and not calling event.Skip().
This is probably less critical than expected, as grid and label area mouse handlers are only called after the user code event handlers.
When checking with the debugger, I did not see that there was an event handler after the grid's own mouse event handlers.

The old implementation of ProcessCornerLabelMouseEvent does call event.Skip() in many cases, but I can't see the motivation when to call and when not:

    if ( event.LeftDown() )
        if ( SendEvent( wxEVT_GRID_LABEL_LEFT_CLICK, -1, -1, event ) == Event_Unhandled )
            SelectAll();
    else if ( event.LeftDClick() )
        SendEvent( wxEVT_GRID_LABEL_LEFT_DCLICK, -1, -1, event );
    else if ( event.RightDown() )
        if ( SendEvent( wxEVT_GRID_LABEL_RIGHT_CLICK, -1, -1, event ) == Event_Unhandled )
            event.Skip();  // no default action at the moment
    else if ( event.RightDClick() )
        if ( SendEvent( wxEVT_GRID_LABEL_RIGHT_DCLICK, -1, -1, event ) == Event_Unhandled )
            event.Skip();  // no default action at the moment
    else
        event.Skip();

So, I have checked ProcessRowColLabelMouseEvent and ProcessCornerLabelMouseEvent to be as unmodified and consistent as possible. I think that's the best option for now. If there are problems, then they will proabably not be with user code. Probably, some event.Skip() calls could be removed without problems.

DietmarSchwertberger added 3 commits February 27, 2024 18:37

wxGrid: allow drag-resizing row and col labels

42bf9b1

also allow dragging the edge between labels and grid

bb9bb85

add comments for clarification

9604b56

DietmarSchwertberger added 2 commits February 28, 2024 14:53

fix dragging label size with frozen rows/cols

509eb6f

allow label resizing also by dragging inside the grid cell area

7a2aac5

DietmarSchwertberger added 3 commits February 28, 2024 23:50

capture mouse in DoGridCellLeftDown to avoid problems when drag resiz…

61dabd2

…e starts at x or y = 0

bug fix for CanDragColLabelSize

df44773

update interface documentation for drag-resizing labels

aa3871d

DietmarSchwertberger marked this pull request as ready for review February 29, 2024 16:02

vadz reviewed Mar 3, 2024

View reviewed changes

DietmarSchwertberger added 2 commits March 3, 2024 17:16

disable label resizing by default

f64a59a

improvements after review

c6ce3ba

DietmarSchwertberger added 2 commits March 3, 2024 22:49

handle case where Leaving event is sent before Moving/Dragging

8dd4cd5

use m_dragLabel when dragging label size, instead of m_dragRowOrCol

324430b

DietmarSchwertberger added 2 commits March 4, 2024 20:40

unify DoEndDragResizeRow/ColLabel via GridOperations

3bef9b2

fix pointers for previoius commit

9a352ae

DietmarSchwertberger added 2 commits March 4, 2024 21:49

simplify DoGridCellLeftDown

9dba419

unify DoEndDragResizeRow and DoEndDragResizeCol

d491d4c

replace switch with if/else again

2c749c1

DietmarSchwertberger marked this pull request as draft March 5, 2024 17:24

DietmarSchwertberger added 3 commits March 6, 2024 20:05

remove space

2777b75

simplify memory management for DoGetOperationsFromCursorMode

1117900

remove event.Skip()

bb11272

DietmarSchwertberger marked this pull request as ready for review March 12, 2024 21:40

vadz added this to the 3.3.0 milestone May 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wxGrid: allow drag-resizing row and col labels #24362

wxGrid: allow drag-resizing row and col labels #24362

DietmarSchwertberger commented Feb 27, 2024 •

edited

DietmarSchwertberger commented Feb 28, 2024

DietmarSchwertberger commented Feb 28, 2024

DietmarSchwertberger commented Feb 29, 2024

vadz left a comment

vadz Mar 3, 2024

vadz Mar 3, 2024

vadz Mar 3, 2024

vadz Mar 3, 2024

DietmarSchwertberger commented Mar 3, 2024

vadz commented Mar 3, 2024

DietmarSchwertberger commented Mar 3, 2024

DietmarSchwertberger commented Mar 3, 2024

DietmarSchwertberger commented Mar 3, 2024

DietmarSchwertberger commented Mar 4, 2024

DietmarSchwertberger commented Mar 4, 2024

DietmarSchwertberger commented Mar 4, 2024

vadz commented Mar 4, 2024

DietmarSchwertberger commented Mar 5, 2024

DietmarSchwertberger commented Mar 12, 2024

	const wxGridOperations* oper = nullptr;
	std::unique_ptr<wxGridOperations> oper;

	{ if ( m_cursorMode == WXGRID_CURSOR_RESIZE_ROW )
	{
	if ( m_cursorMode == WXGRID_CURSOR_RESIZE_ROW )

	if ( width >= 0 )
	wxCHECK_RET( width >= 0, "invalid min row label width" );

wxGrid: allow drag-resizing row and col labels #24362

Are you sure you want to change the base?

wxGrid: allow drag-resizing row and col labels #24362

Conversation

DietmarSchwertberger commented Feb 27, 2024 • edited

DietmarSchwertberger commented Feb 28, 2024

DietmarSchwertberger commented Feb 28, 2024

DietmarSchwertberger commented Feb 29, 2024

vadz left a comment

Choose a reason for hiding this comment

vadz Mar 3, 2024

Choose a reason for hiding this comment

vadz Mar 3, 2024

Choose a reason for hiding this comment

vadz Mar 3, 2024

Choose a reason for hiding this comment

vadz Mar 3, 2024

Choose a reason for hiding this comment

DietmarSchwertberger commented Mar 3, 2024

vadz commented Mar 3, 2024

DietmarSchwertberger commented Mar 3, 2024

DietmarSchwertberger commented Mar 3, 2024

DietmarSchwertberger commented Mar 3, 2024

DietmarSchwertberger commented Mar 4, 2024

DietmarSchwertberger commented Mar 4, 2024

DietmarSchwertberger commented Mar 4, 2024

vadz commented Mar 4, 2024

DietmarSchwertberger commented Mar 5, 2024

DietmarSchwertberger commented Mar 12, 2024

DietmarSchwertberger commented Feb 27, 2024 •

edited