Add exwm-client-message-functions

[progfolio]

• exwm.el (exwm-client-message-functions): Abnormal hook for dispatching client messages.
  (exwm--on-ClientMessage): delegate to said hook, decompose body into separate hook functions

See: ch11ng/exwm#931

[progfolio]

I think this is a good idea in general, but there are still some implementation details to work out.

Currently, each function added to exwm-client-message-functions is responsible for inspecting the message type and returning non-nil (return values for all the new functions still need to be checked) if the hook should stop running.

This approach is simplest to implement and most flexible, but we do incur the performance cost of repeatedly checking slot-values on the message in each hook function. We should do some performance measurements to see if this is worth optimizing.

One simple optimization would be ordering the default value of the hook functions so that the most frequent types of messages are checked for first.
We could also pass the slots which are bound in exwm--on-ClientMessage (type, id, data), along with the message to the hook functions. That does complicate the function signature of each handler, but it would also simplify each handler's implementation and be more performant.

Another idea would be to generate hook functions for each type of client message programmatically.
Then users could subscribe to hooks they're interested in without worrying about the type in the implementation.

Once we figure this out, we should add a similar hook for exwm--on-net-wm-state, so users can customize what happens for each state property (e.g. fullscreen, as requested in the original linked issue).

Thoughts, @minad @Stebalien ?

[Stebalien]

I'm not a fan of exposing such a hook to users as EXWM is likely to misbehave if a user mishandles these messages. If the user really needs full control, they can listen to the event themselves and/or advise the function.

In terms of a handling fullscreen changes, etc., I'd add a separate hook for that (like the floating hook, manage hook, etc.).

I am a fan of breaking this up but I wouldn't use a hook. I'd eitehr:

1. Use an internal alist/hash.
2. Use a cond/pcase, explicitly invoking the handler functions.

From my brief look, it appears that we always switch on the "type".

[progfolio]

I'm not a fan of exposing such a hook to users as EXWM is likely to misbehave if a user mishandles these messages. If the user really needs full control, they can listen to the event themselves and/or advise the function.

Isn't that the case for every hook in Emacs?
Of course it can be misused, but it's not the responsibility of package maintainers to support homespun hook functions, so I don't see any more of a burden than the usual hook introduces, and it gives those who wish to change things a way to do it without resorting to advice.

[Stebalien]

Isn't that the case for every hook in Emacs?

Not really. Hooks are user-facing extension points and package authors tend to take care to make sure:

1. They aren't too sharp.
2. They aren't compatibility hazards. A hook is a promise not to change something (without good reason).

I agree we should have a hook, but it should be higher level. Let the user hook into a specific behavior (EXWM is making a window full-screen) instead of asking the user to hook into the raw X event stream.

[progfolio]

1. They aren't too sharp.

I think any hook can be "too sharp" given that it executes arbitrary code, so it's not a metric I would use to consider whether a hook is worth it or not. All hooks are "caveat utilitor". That's the price of the flexibility.

2. They aren't compatibility hazards. A hook is a promise not to change something (without good reason).

Do you foresee any compatibility hazards in this case?
I don't think the X client message format will be changing drastically any time soon.

I agree we should have a hook, but it should be higher level. Let the user hook into a specific behavior (EXWM is making a window full-screen) instead of asking the user to hook into the raw X event stream.

I see no reason we can't offer both types of hooks. Lower level for those who wish to hook in there, and higher level for users who want that.

[Stebalien]

In this case, there are multiple ways to get a full screen window:

1. The window can request it.
2. The user can request it.
3. Possibly more in the future.

When we encourage users to hook into low-level features, any low-level change can break their code. That's not to say that we should prevent users from hooking into these features, but we shouldn't encourage it by creating hooks.

If the user wants to receive client messages, they can listen for them the same way EXWM does. But by hooking into EXWM's event stream (and possibly filtering it), they can easily end up getting EXWM into an inconsistent state. Again, we shouldn't prevent this, but this kind of low-level interception is exactly what advice is designed for.

[progfolio]

In this case, there are multiple ways to get a full screen window:

1. The window can request it.
2. The user can request it.
3. Possibly more in the future.

When we encourage users to hook into low-level features, any low-level change
can break their code. That's not to say that we should prevent users from
hooking into these features, but we shouldn't encourage it by creating hooks.

If the user wants to receive client messages, they can listen for them the
same way EXWM does. But by hooking into EXWM's event stream (and possibly
filtering it), they can easily end up getting EXWM into an inconsistent state.
Again, we shouldn't prevent this, but this kind of low-level interception is
exactly what advice is designed for.

We'll have to agree to disagree here.
I'm not a fan of when packages eschew hooks in favor of recommending users advise lower level, private package functions. It's not any easier or safer for the users or anyone who wishes to write a compatible library to interact with the library. As often mentioned, advice is the "sledgehammer of last resort".

In any case I've amended the patch to use what I think is a fair compromise.
exwm--client-message-functions is now a "private" alist mapping event types to their respective handlers.
The handler, if found, is called with the ID and DATA slots bound in exwm--on-ClientMessage.

[minad]

We'll have to agree to disagree here.
I'm not a fan of when packages eschew hooks in favor of recommending users advise lower level, private package functions. It's not any easier or safer for the users or anyone who wishes to write a compatible library to interact with the library. As often mentioned, advice is the "sledgehammer of last resort".

Well, but the client message handling is already low level, so you are providing a hook for something which shouldn't really be exposed. I tend to agree that hooks should be provided for high level functionality. Also I am on board with refactoring and decompose the client message handler function.

[minad]

In any case I've amended the patch to use what I think is a fair compromise.
exwm--client-message-functions is now a "private" alist mapping event types to their respective handlers.
The handler, if found, is called with the ID and DATA slots bound in exwm--on-ClientMessage.

This sounds like a good compromise. There also shouldn't be a significant performance impact, given that alist-get/assq is fast for short lists.

[Stebalien]

I agree, this sounds like a good compromise.

[Stebalien]

Tested locally and this appears to work. I'll merge when you mark it ready.

[progfolio]

Tested locally and this appears to work. I'll merge when you mark it ready.

Do not merge yet.
It actually doesn't work.
We can't construct the alist via the xcb:Atom:... symbols because they are defined at runtime. Compiler will see them all as nil, leading to unhandled events in byte-compiled version.

[Stebalien]

Huh. You're right. And yet EXWM sitll... mostly seems to work. I expected it to completely break in this case...

[Stebalien]

It looks like we need to set it after starting EXWM (or, even better, have a template with the atom symbols that we then evaluate on init to create the runtime mapping).

TBH, these atoms should probably be properties of the connection itself...

[progfolio]

It looks like we need to set it after starting EXWM (or, even better, have a
template with the atom symbols that we then evaluate on init to create the
runtime mapping).

TBH, these atoms should probably be properties of the connection itself...

I went with a simple approach for now.
Variable is declared, then setq'd during exwm-init.
Everything's working on my end now.

[progfolio]

But by hooking into EXWM's event stream (and possibly filtering it), they can easily end up getting EXWM into an inconsistent state

Huh. You're right. And yet EXWM still... mostly seems to work. I expected it to completely break in this case...

Looks like it's not as big a threat as it seemed.
I still think a hook might be a good addition if someone requests it, but we can always add that if the need arises.

[Stebalien]

Looks like it's not as big a threat as it seemed.

(subtle bugs are worse, IMO)

[Stebalien]

Ok, tested minimize, full screen, and close by client request.