wxGrid: patch to add better TAB handling

[fsenore]

As discussed in wx-dev I have added a better TAB handling to wxGrid.
The patch does not contain documentation changes yet: I'd rather wait until it is accepted before writing documentation.
I added a new method to set 3 different behaviours.
The default is the same as before: TAB moves only inside the current row and the focus always remains to the grid. With this setting the grid emits an event just before processing the TAB key: the user can handle it to implement custom behaviours.
The other two behaviours are predefined and they do not emit any event.

The patch also contains some changes to the griddemo sample. I changed the tabular table test, adding an event handler that makes TAB move the cursor up and down.
There are also two commented lines to set the other possible behaviours.

Of course, feel free to request changes to the code and to the names that I used in the patch.

[vadz]

Thanks for your patch! I've looked at it only briefly right now but did notice a couple of things that prevent it from being immediately applied:

1. The new features really need to be documented, both the new event and SetTabMode() with the accompanying enum. Please update interface/wx/grid.h to mention them and please use "@since 2.9.5" in their documentation.
2. It would be really nice if the patch could be formatted correctly, i.e. using wx indentation style, in particular with all braces on their own lines (including the opening ones) and a space between "if" and the following "(". This is, of course, minor, but I need to find time to do it myself before applying the patch which means that I can't apply it as quickly as would be otherwise possible.
3. I think there is still some confusion about event handling: why does the default handler for the event call DoGridProcessTab()? This shouldn't be needed because you already call it if the event was not processed. So actually you don't need to even have this handler at all. Also, you can just test the return value of ProcessEvent() (BTW, use ProcessWindowEvent() instead of GetEventHandler()->ProcessEvent()) instead of doing something unclear with veto/IsAllowed(). I don't know if it makes sense to veto this event at all to be honest.

Please don't hesitate to ask on the mailing list if you have questions about anything above. Thanks!

[fsenore]

1 - I wanted to wait until the interface was stable before writing the docs, just to save some work. Now I will write it.

2 - Ok.

3 - I did in this way because I could not find a better way. The problem happens when the tab handling mode is set to wxGridTabLocked: in this case I need to send the event to the grid's parents to make custom handling possible. Things happen in the following way.

When the wxGrid::OnKeyDown() receives a TAB it sends a wxEVT_GRID_TABBING event.
The event is received by wxGrid::OnTabbing(), which in turn skips the event to let it go to the grid's parents for custom handling.
When event.Skip() returns I do not know whether the event has been processed by some other handler or not, but I need to know it do decide if I should process it or not.
The only solution that I have found is to immediately return from wxGrid::OnTabbing(): in wxGrid::OnKeyDown() I can know if the event has been handled by some other handler and I can decide whether to handle it or not.
Of course I will be glad to use a simpler approach if it exists.

About Veto()/IsAllowed(), do you mean that if ProcessWindowEvent() returns true it means that the keypress has been handled somewhere, so I do not need to handle it by myself?

[vadz]

Replying to fsenore:

When the wxGrid::OnKeyDown() receives a TAB it sends a wxEVT_GRID_TABBING event.
The event is received by wxGrid::OnTabbing(), which in turn skips the event to let it go to the grid's parents for custom handling.
When event.Skip() returns I do not know whether the event has been processed by some other handler or not,

You do, it is the return value of Process[Window]Event(). If it returns true, the event was processed, otherwise it wasn't.

About Veto()/IsAllowed(), do you mean that if ProcessWindowEvent() returns true it means that the keypress has been handled somewhere, so I do not need to handle it by myself?

Yes, exactly.

[fsenore]

Replying to vadz:

Replying to fsenore:

When the wxGrid::OnKeyDown() receives a TAB it sends a wxEVT_GRID_TABBING event.
The event is received by wxGrid::OnTabbing(), which in turn skips the event to let it go to the grid's parents for custom handling.
When event.Skip() returns I do not know whether the event has been processed by some other handler or not,

You do, it is the return value of Process[Window]Event(). If it returns true, the event was processed, otherwise it wasn't.

This is not clear to me. In wxGrid::OnTabbing() I receive the event as a parameter and I skip it to let it go to the grid's parents. In that function I do not call ProcessWindowEvent() (it is called in wxGrid::OnKeyDown()) so I have no way to know if the event has been handled. That's why I can only test it in wxGrid::OnKeyDown().
Of course, I might be missing something.

[vadz]

As I wrote in the original comment, you shouldn't be doing this in OnTabbing() and in fact you shouldn't even have this handler in the first place. You should do everything in OnKeyDown(). The pseudo code is

wxGridEvent event(wxEVT_TABBING);
if ( !ProcessWindowEvent(event) )
{
   ... implement default handling ...
}

[fsenore]

Ok, this makes sense to me. Modifying OnKeyDown() was my original choice, but in a mail dated September 21 you wrote:

"Yes, but OnKeyDown() is already not that simple and I'd really like to
avoid complicating this further. It's much more clear to just generate an
event there and then do whatever the event handler decides to do and leave
all the complexity to this event handler."

I understood this as a hint to make minimal changes to OnKeyDown() (and I agree that the function is already rather complicated). Minimal changes to OnKeyDown() required moving the code to another function handling the event so I ended up writing the code in that way.

Of course I can remove OnTabbing() and move its code to OnKeyDown(). This would make things simpler to understand but it would add some code to OnKeyDown().

So, should I remove OnTabbing()?

[vadz]

Yes, you should remove OnTabbing(), AFAICS it's completely unneeded. But it doesn't mean you need to put the default tab handling in OnKeyDown() itself, you can still keep it in this separate DoGridProcessTab() function, as you already did. It's not a problem to add one line calling it to OnKeyDown(), of course

[fsenore]

I was editing the code and I discovered that wxGrid::OnKeyDown() already propagates a wxKeyEvent to its parents (it happens in its first lines of code). If the key press is handled no more processing happens in wxGrid itself.

Knowing this I am wondering if it still makes sense to add a new wxEVT_GRID_TABBING event.
Users wanting to custom-handle the TAB key could simply handle the KeyDown event that the grid already sends to its parents, but of course I might be missing something.

Do you think that it's still worth adding a new wxEVT_GRID_TABBING event? The code has already been written so adding the event would not take more work from me, but it would add complexity to interface.

[vadz]

Replying to fsenore:

I was editing the code and I discovered that wxGrid::OnKeyDown() already propagates a wxKeyEvent to its parents (it happens in its first lines of code).

Sorry, I had no idea it did this :-( I still wonder why does it do it but the fact is that it does and apparently always did (this code dates from r5818 from February 2000).

I must admit I'm very uncomfortable with this, it's a completely exceptional case in wxWidgets event propagation model as usually the events either don't propagate at all or propagate recursively upwards while this one propagates only to the immediate parent. Unfortunately removing this now could silently break some currently working code so it's not a good idea. But I still don't like it.

Knowing this I am wondering if it still makes sense to add a new wxEVT_GRID_TABBING event.

Yes, it's a legitimate question now...

Do you think that it's still worth adding a new wxEVT_GRID_TABBING event? The code has already been written so adding the event would not take more work from me, but it would add complexity to interface.

Yes, I still think it would be better to have a normal wxCommandEvent (well, wxGridEvent but it derives from wxCommandEvent) event which would be propagated according to the usual rules.

[fsenore]

Here is a new patch that should fix the problems with the first one.

The patch also contains some changes to the griddemo sample. I changed the tabular table test, adding an event handler that makes TAB move the cursor up and down.
There are also two commented lines to set the other possible behaviours.

[fsenore]

Second version of the patch.

[vadz]

Thanks, I'll apply this soon with some renamings and a tiny logic change: I finally think it makes more sense to always generate the TABBING event, it's inconsistent to only do it for one mode but not the others.

[VZ]

(In [72672]) Make TAB behaviour in wxGrid more configurable.

Allow making TAB/Shift-TAB wrap to the next/previous row or going to the
next/previous control when the cursor is at the end/beginning of the current
row easily.

Also add wxEVT_GRID_TABBING event to allow customizing TAB behaviour even
further.

Update the sample to show the different possible standard behaviours and a
stupid example of a custom one (it would be probably more useful to implement
something a tad more realistic, e.g. tabbing to the next non-empty cell).

Closes #14711.