ch11ng / exwm

Emacs X Window Manager
2.85k stars 135 forks source link

Implement the XSETTINGS protocol #935

Closed Stebalien closed 8 months ago

Stebalien commented 9 months ago

Users can use this to configure system-wide themes, icons, fonts, etc.

(setopt exwm-xsettings-theme '("Adwaita" . "Adwaita-dark")
        exwm-xsettings-icon-theme '("Papirus-Light" . "Papirus-Dark")
        exwm-xsettings '(("Gtk/FontName" . "IBM Plex Sans 9")
                         ("Gtk/DocumentFontName" . "IBM Plex Sans 9")
                         ("Gtk/MonospaceFontName" . "JetBrains Mono 9")))

fixes #876

NOTE: If enabled, this module will conflict with any other XSETTINGS implementation and will refuse to start.


medranocalvo commented 9 months ago

Thank you @Stebalien. I'll review as soon as possible.

minad commented 9 months ago

As a data point, I've installed this exwm-xsettings package in my configuration about a week ago. It works flawlessly (theme switching and other settings). I think the only thing I am missing here is a little bit of documentation. Maybe an example configuration in the commentary would do?

This is what I use:

(require 'exwm-xsettings)
(setq exwm-xsettings-theme '("Adwaita" . "Adwaita-dark")
      exwm-xsettings '(("Xft/HintStyle" . "hintslight")
                       ("Xft/RGBA" . "rgb")
                       ("Xft/lcdfilter" . "lcddefault")
                       ("Xft/Antialias" . 1)
                       ("Xft/Hinting" . 1)
                       ("Xft/DPI" . 147456)))
(exwm-xsettings-enable)
Stebalien commented 9 months ago

LGTY?

medranocalvo commented 9 months ago

I have reviewed the code. I answered a couple of questions. I'm happy to merge when you think it's ready (e.g. remove TODO: comment).

This is also a great feature, thank you very much!

Stebalien commented 9 months ago

Ok, I've addressed everything and it should be good to go now. Thanks for the review!

medranocalvo commented 9 months ago

One final nit: up to now I've followed strictly the Emacs commit message conventions (https://github.com/ch11ng/exwm/wiki/Maintenance#commit-messages), mainly because @ch11ng did. I use Magit and C-x 4 a and it is easy enough for me. But maybe it's time to revise this "policy". Should we follow Emacs commit message conventions? @minad, I'd like you to weigh in as well.

minad commented 9 months ago

In my projects I mostly don't follow the convention, so we could as well relax it. But if mostly people feel like keeping the convention I won't mind.

Stebalien commented 9 months ago

So, there are two issues:

  1. Commit messages.
  2. Commits themselves.

With respect to the first, I kind of like Emacs's commit format as it tells you exactly what changed and why, but it's also kind of annoying. I don't have a strong preference either way (but I've updated the PR description with what the commit message should be).

With respect to the second, the issue here is that GitHub doesn't have proper "patch sets". That leaves three options for addressing PR review.

  1. Open a new PR (terrible).
  2. Amend/rebase and push (difficult to review).
  3. Keep adding new commits.

So I've gotten into the habit of doing the latter. Unfortunately, that means that the best way to merge PRs (without random commits) is, IMO, squash-merge.


For now, I'd:

  1. Squash merge.
  2. Keep the commit message format, but consider formatting the commit message (when squash-merging) on behalf of contributor if it's not too complex.

That is, unless the patch requires no changes and/or is a nicely formatted patch set. In that case, just rebase-merge.

medranocalvo commented 8 months ago

@Stebalien I agree on both 1. and 2.

On 1., I find the consistent messages slightly helpful, it is kind of a semi-automatic summary. The format allows the more usual commentary with the summary at the bottom (see git log for examples). Using C-x 4 a from the Magit diff (it might work in VC?) makes the chore easy. I've been editing most (?) contribution's commit messages and it's not been a big hassle for me. I believe that we shouldn't require contributors to write it themselves.

On 2. I believe that most contributions have been single commits (not sure?). I prefer the adding of further commits with the changes after review, but in the end it will be whatever the contributor does. (We can follow your proposal between us.) Squashing and writing the final message seems a comfortable default.

Stebalien commented 8 months ago

Moved to https://github.com/emacs-exwm/exwm/pull/2. I kept it on my local fork for now, although I'll probably start pushing branches directly to emacs-exwm/exwm in the future.