Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Xresources-based theme #35

Merged
merged 8 commits into from
Jul 24, 2024
Merged

Conversation

SqrtMinusOne
Copy link
Contributor

@SqrtMinusOne SqrtMinusOne commented Jul 12, 2024

I use Xresources to configure themes. This PR adds themes called xresources-light and xresources-dark, provided that xrdb is found and xrdb -query returns the necessary colors.

This is a bit suboptimal for light theme (which I use), because light themes don't need as much shading as C-o and C-p currently do. But solarized-light also has this issue.

image

@@ -701,6 +702,52 @@ def on_mount(self) -> None:
)
self.theme_change_signal = Signal[ColorSystem](self, "theme-changed")
self.theme = self.settings.theme
self.try_to_add_xresources_theme()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd want to put this behind a config option use_xresources which is False by default. If that option is True, then this method would be called.

It should also be possible to set the theme to xresources-dark or xresources-light directly from the config file (e.g. theme: xresource-dark).

Copy link
Owner

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this to sit behind a config option.

@darrenburns
Copy link
Owner

Thanks for the contribution! - I've done a short review, let me know what you think or if you need help.

@SqrtMinusOne
Copy link
Contributor Author

Done. I've:

  • add an option called use_xresources, as you suggested;
  • changed the method to fail with more verbose errors in case something goes wrong;
  • moved the invocation of the method from on_mount to the constructor. I'm not sure if there's a more suitable place for it, but it works.

@SqrtMinusOne
Copy link
Contributor Author

Oops, my Emacs has auto-formatted the table in the README. I hope you don't mind.

@darrenburns
Copy link
Owner

Thanks! I'll try to review this this week.

@darrenburns
Copy link
Owner

Thank you - I've made a few minor changes. Moved stuff into a new module to keep it separate from the App, an simplified the code a bit.

Would you mind giving it a quick test? If it looks good to you I will merge.

@darrenburns darrenburns merged commit b8d77fd into darrenburns:main Jul 24, 2024
1 check passed
@SqrtMinusOne
Copy link
Contributor Author

SqrtMinusOne commented Jul 25, 2024

Thanks.

But the current version fails with:

  File "/home/pavel/00-Scratch/posting/src/posting/xresources.py", line 36, in load_xresources_themes
    missing_colors = set(XRDB_MAPPING.values()) - supplied_colors.keys()
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: unhashable type: 'list'

The point of list(itertools.chain(*self.XRDB_MAPPING.values())) was to flatten that list. I'll make a new PR with a fix.

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.

2 participants