I'm mixed on this, but leaning 👍.

On the pro side, this makes it slightly more explicit what is going on (as you create the Event object rather than calling a function that creates it for you). I have always found the method name (pick_event) to be confusing so I'm not particularly sad to see them go and I hope that these are not being widely used by non-backend authors.

On the con-side having the process method push it's self through the callback registry seems a bit odd. This also moves management of (private) state on the canvas out of canvas methods and into the EventSubclass.process which is also a bit odd. I think this will require changes to atleast 3 extrernal packages (mplcairo, ipympl, the kivy backend).

The process for figure_leave_event should clear LocationEevent.lastevent

If we are going to do this the on-init (!?) processing chain that happen in LocationEvent.__init__ (via _update_enter_leave should be moved to process.

On the pro side, this makes it slightly more explicit what is going on (as you create the Event object rather than calling a function that creates it for you). I have always found the method name (pick_event) to be confusing so I'm not particularly sad to see them go and I hope that these are not being widely used by non-backend authors.

Especially, these methods are (effectively) backend-dependent, so you can't write backend-independent code using them (well, unless you're careful to always call the base class implementation FigureCanvasBase.foo_event instead of canvas.foo_event...).

On the con-side having the process method push it's self through the callback registry seems a bit odd.

I'm not super-wedded to the API; initially I had the somewhat more obvious(?) def FigureCanvasBase.process_event(event): self.callbacks.process(event.name, event), which is a bit more verbose but works too. Or we can just not add any helper and force backend implementers to call everywhere event = FooEvent(...); canvas.callbacks.process(event.name, event) which is even more verbose, but again works too. On the other hand, having a helper is also useful for e.g. testing code which wants to trigger arbitrary events (e.g., mplcursors).

This also moves management of (private) state on the canvas out of canvas methods and into the EventSubclass.process which is also a bit odd.

I agree, but really I think these things should be just plain callbacks always connected at canvas setup (e.g. adding a callback that draws on resize).

I think this will require changes to atleast 3 extrernal packages (mplcairo, ipympl, the kivy backend).

mplcairo doesn't need to change anything because it fully relies on Matplotlib's base classes for UI integration (that's basically why I wrote the foocairo backends for Matplotlib -- making sure that GUI and rendering were orthogonal); IOW it only overrides paintEvent (Qt), on_draw_event (GTK3), etc.

A quick look suggests that yes, Kivy and ipympl will need updates, but they can at least use the no-helper API (event = FooEvent(...); canvas.callbacks.process(event.name, event)) which should be safe in all cases.

The process for figure_leave_event should clear LocationEevent.lastevent
If we are going to do this the on-init (!?) processing chain that happen in LocationEvent.init (via _update_enter_leave should be moved to process.

I need to look into it, but again I'd rather not have this lastevent at all and just have the GUI backends query the cursor position when the event is triggered and fill in that info correctly.
Edit: now done in a separate commit; this is needed for axes_{enter,leave}_event to work.

but they can at least use the no-helper API

But then they lose the axes enter/leave state management. They are going to have to carry some version gating code for a while.

I guess we could pull all of the "extra" logic back into the canvas and add a "pre-process" step to the call back registry?

so you can't write backend-independent code using them

Why are you trying to write code against these methods? My sense of the design is that the canvas.foo_event were "internal" and should only be called by the backends (which is why the API drifted between the backends) and that users should be writing code against the other side of the callback registry.

Just a quick thought (not an expert in the event system): Aren‘t events sort of data classes (if that had existed back then)? It feels a bit strange to add process functionality there.

But then they lose the axes enter/leave state management. They are going to have to carry some version gating code for a while.

I guess we could pull all of the "extra" logic back into the canvas and add a "pre-process" step to the call back registry?

Fair enough, I guess we can have that code both in LocationEvent.process (like I did in this PR) and in each of the FigureCanvasBase.location_event(). (In any case, that seems better than having it in the LocationEvent constructor -- big side effects in a class constructor seem just... awkward.)

so you can't write backend-independent code using them

Why are you trying to write code against these methods? My sense of the design is that the canvas.foo_event were "internal" and should only be called by the backends (which is why the API drifted between the backends) and that users should be writing code against the other side of the callback registry.

I'm not actually trying to write code against these methods; my point here was just that end-users can't rely on it (as you say they're basically internal).

The original motivation was to improve event metadata in the case of MouseEvents (#6159). Basically, right now the GUI backends don't fill in the .key attribute of MouseEvents -- because indeed Qt/GTK/Tk/wx expose no API to tell what keys are pressed when a mouse event occurs. Instead, the key attribute is filled based on the last key_{press,release}_event. But that's actually quite brittle, because key presses/releases can also occur while the canvas doesn't have the focus, and these won't be recorded (this isn't an abstract problem, but one I actually encountered, and quite puzzling in fact, because most of the time .key will be correct... but fail in a few times, then you repress the key a few times and "weirdly" it works again, and you wonder whether your keyboard is dying). However, Qt/... expose an API to tell which modifier keys (Ctrl/Alt/Shift) are pressed when a mouse event occurs, so MouseEvent could gain a .modifiers attribute recording that info (and then we may choose later to deprecate .key and tell the users to listen to key_{press,release}_event themselves if they want, but that's another story). With the approach of this PR, one would just need to add a .modifiers parameter/attribute to MouseEvent, and then the backend could just set it and call process() on it. In fact a third-party backend could even choose to implement that independently of Matplotlib by creating its own MouseEvent subclass and then push it to the callback registry. But with the current API this is slightly more annoying, because it needs to go through button_{press,release}_event/etc., which all need to have their signature extended. And then is it a bug if a backend exposes a button_press_event which has no modifiers parameter? (On the other hand it's not really a problem because these methods already have incompatible signatures anyways.)

The situation is similar wrt. figure_leave_event, which currently fills its location based on the last LocationEvent, but really the GUI backends could/should just query the cursor position when the LocationEvent is triggered and fill in that info themselves.

Just a quick thought (not an expert in the event system): Aren‘t events sort of data classes (if that had existed back then)? It feels a bit strange to add process functionality there.

As noted above, initially I had a def FigureCanvasBase.process_event(event): self.canvas.process(event.name, event). One of the issues, though, is that with that approach you need to put all the event-specific process() logic into a big switchyard in that single method, rather than in each separate process() implementation.

Also I don't think that dataclasses means "no methods".

Seems like this still needs some discussion?

Likely, yes. (I'll wait for the rebase until we can agree on whether we want this or not.)

On a similar note, the approach here would also help with reporting of pressed buttons in the case of motion_notify_event: currently, we just report the button corresponding to the last button_press_event that occurred over the canvas, but all GUI frameworks can actually report all of the buttons that are being pressed while a cursor motion occurs (which may not even include the last button_press_event, if the mouse motion started out of the canvas).

... and rebased.

So was this ever on a call?

A few times, but last time was a while ago.

My attempt to summarize:

1. the idea of having a method on the base class per event is / was sound
2. using the same methods do directly connect to the UI toolkits caused signature instability in between the subclasses
3. Our events know a lot about the context they are in and can "self actuate" because they know which canvas they came from. On one hand this feels a bit weird, but on the other hand it is a way to make sure that we do not pass events from one canvas to callbacks on the other (might be better if they Events knew less, but it is too late for that)
4. we are still doing "dead reckoning" of a few things (the last event so we can track leaving / entering axes and figures, attaching keys to button events and buttons to key events)
5. this gives us a path get let the UI toolkit track that information for us, but does not get us there yet

I'ma little wary of documenting to use canvas.callbacks.process(event.name, event) when that is not what we do internally (because the dead reckoning book keeping is done in Event._process (in the sub-classes implementation or because we actually emit some events 2x but with different names for the enter/leave events (it was like this before). Although in #16931 (comment) it is noted the consensus was to make _process private, I'm leaning the other way now and think we should make it public. It might also make sense to have a single proccess_ui_event(name, evt) on the canvas base-class that merges all the special casing into one place?

I am happy with the motion_notify subscription, doing that work in __init__ (maybe triggering other callbacks!) was not great. There might be some subtle differences in the order of events (I think that you used to get the enter/leave event then the location event, now you will get the location event and then the enter / leave event). I really hope no one is relying on that ordering and do not think it is worth more effort to restore the old behavior (but we should note it).

Over all 👍🏻 and think we should get this in for 3.6.

Thanks for the review. I moved all special-casing in the various _process methods into plain event handler callbacks (in separate commits, which should be hopefully self-contained enough for review), which seems cleaner. How does that look to you? I can still make _process public if you prefer and/or revert back to putting the special-casing into _process overrides.

As for event ordering, I believe user callbacks registered for the axes_enter_event will still be called before user callbacks registered for the motion_notify_event, because the order is now 1) we process the motion_notify_event, and call the the first callback registered to it, which is _axes_enter_leave_emitter 2) this synthesizes and processes the axes_enter/leave_event, and calls all user callbacks registered to it, and finally 3) we return to prcessing the motion_notify_event, calling all remaining callbacks, which are the user callbacks.

I did the rebase, anyone (including @anntzer ) can merge this when green.