Skip to content

Conversation

@Stebalien
Copy link
Member

This PR contains a bunch of related changes following on from the introduction of `xcb:unmarshal-new' in XELB. I suggest reading the individual commits and messages, but the main goal is to make the code more readable by avoiding the C-like declare-then-set pattern and instead bind values directly where possible.

The last commit also has some fixes and behavioral changes.

I'm submitting this as a PR because it's very large and don't want to clobber any other work in progress without warning.

@Stebalien
Copy link
Member Author

The one concern I have with this PR is XIM: I don't use it, haven't tested my changes, and am a bit scared of trying to configure it given all the bug reports I've seen.

@Stebalien Stebalien force-pushed the steb/war-on-setq branch 2 times, most recently from cea2f7c to a278dab Compare November 15, 2025 00:56
@Stebalien Stebalien changed the title Refactor: Begun, the war on setq has Refactor: Begun, the setq wars have Nov 15, 2025
@minad
Copy link
Member

minad commented Nov 15, 2025

I think we should tag a xelb release first before starting to use xcb:unmarshal-new.

@minad
Copy link
Member

minad commented Nov 15, 2025

Otherwise I am fine with overhauling the code like this. I'd suggest to first merge the trivial commits, e.g., only the xcb:unmarshal-new changes. Furthermore it would be great if the commits are fine-grained such that we can easily bisect in case something goes unexpectedly wrong, one commit per function or per file, if that's not too much. Thanks!

@Stebalien Stebalien force-pushed the steb/war-on-setq branch 2 times, most recently from 438f5dc to 2e174a7 Compare November 15, 2025 17:49
@Stebalien
Copy link
Member Author

I think we should tag a xelb release first before starting to use xcb:unmarshal-new.

SGTM.

I'd suggest to first merge the trivial commits, e.g., only the xcb:unmarshal-new changes.
Furthermore it would be great if the commits are fine-grained such that we can easily bisect in case something goes unexpectedly wrong, one commit per function or per file, if that's not too much.

I broke out the remaining "larger" changes from the first commit:

  1. The first commit is now just a mechanical replacement of unmarshal with unmarshal-new.
  2. The second commit removes some unnecessary let forms by combining them with subsequent with-slots forms.
  3. The rest of the commits are broken down file-by-file, with the bigger changes broken out into separate commits.

@Stebalien
Copy link
Member Author

I went ahead and tagged a new XELB release. IMO, we had enough changes and bug fixes there to warrant one anyways.

@Stebalien
Copy link
Member Author

I've merged the first few commits, but none of the larger refactors. I'll wait a few days before merging the rest.

@minad
Copy link
Member

minad commented Nov 16, 2025

I've merged the first few commits, but none of the larger refactors. I'll wait a few days before merging the rest.

Thanks, I was about to suggest this. Yes, please wait a few days and check if all is well in your setup. But then given that the commits are fine-grained we can easily diagnose.

We should also bump the Xelb version requirement, in case you haven't done that already. I've seen that the new Xelb version landed on GNU ELPA.

@Stebalien Stebalien force-pushed the steb/war-on-setq branch 2 times, most recently from 866fc9c to 2b203c7 Compare November 19, 2025 15:53
@Stebalien
Copy link
Member Author

I've pushed everything except:

  1. The XIM changes. I don't use XIM and want to isolate that change at the end.
  2. The change to ConfigureRequest handling (large refactor, tricky code, lots of edge-cases).

@minad
Copy link
Member

minad commented Nov 19, 2025

The one concern I have with this PR is XIM: I don't use it, haven't tested my changes, and am a bit scared of trying to configure it given all the bug reports I've seen.

Regarding XIM - I also don't use it and don't have immediate plans to do. I could maybe try it out just out of curiosity, but there are so many things to do ...

If you don't feel comfortable with the changes and can live with some uncleanliness in the exwm-xim file we can leave it as is, and preserve this commit as an open PR?

exwm-manage: Refactor the ConfigureRequest handler

Would it make sense to split this commit up? Maybe remove the Chesterton fence in a first separate commmit, just to simplify potential future bisection? As a general guideline, which I try to follow, I usually separate functional changes from pure reorganization, which should preserve behavior exactly.

@Stebalien
Copy link
Member Author

If you don't feel comfortable with the changes and can live with some uncleanliness in the exwm-xim file we can leave it as is, and preserve this commit as an open PR?

I feel relatively comfortable with the changes to XIM, they're pretty straight-forward.

Would it make sense to split this commit up? Maybe remove the Chesterton fence in a first separate commmit, just to simplify potential future bisection? As a general guideline, which I try to follow, I usually separate functional changes from pure reorganization, which should preserve behavior exactly.

I've split it into several commits. Unfortunately, I had to make the fixes first before the rewrite because preserving the minimum width/height delta in the rewrite was too much work.

This makes it easier to trace where variables are used/set.

* exwm-xim.el (exwm-xim--on-SelectionRequest):
(exwm-xim--on-ClientMessage-0):
(exwm-xim--on-ClientMessage):
(exwm-xim--make-request): Prefer let over setq.
Instead of binding a `req' variable around the entire function, decode
and bind request slots directly where they're used.

* exwm-xim.el (exwm-xim--on-request): Refactor for readability.
@Stebalien
Copy link
Member Author

I've merged everything except the XIM changes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants