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

Support loading custom syntax themes from a file #2565

Merged
merged 9 commits into from
Mar 19, 2025

Conversation

acuteenvy
Copy link
Contributor

@acuteenvy acuteenvy commented Mar 17, 2025

This Pull Request fixes/closes #2523 & closes #2308.

Custom syntax highlighting themes can be loaded from a .tmTheme file.

If syntax in theme.ron (added in #2532) is a filename that exists in the gitui config directory, it's used as the syntax theme. If not, gitui falls back to selecting from the default theme set.


For example, if you put the following in theme.ron:

(
    syntax: Some("mytheme"),
)

gitui will load ~/.config/gitui/mytheme.tmTheme.


I followed the checklist:

  • I added unittests
  • I ran make check without errors
    make check reports a duplicate dependency error:
    error[duplicate]: found 2 duplicate entries for crate 'base64'
    
  • I tested the overall application
  • I added an appropriate item to the changelog

@acuteenvy
Copy link
Contributor Author

All tests are running fine for me, I'm not sure why the CI is failing.

@naseschwarz
Copy link
Contributor

naseschwarz commented Mar 17, 2025

All tests are running fine for me, I'm not sure why the CI is failing.

I don't have a solution, but if you want to reproduce, just make sure your gitui config directory does not exist when you run the tests.

If you're on Linux, run XDG_CONFIG_HOME="$(pwd)/foo" cargo test , for example.

@naseschwarz
Copy link
Contributor

I'd wager you've already found it yourself, but just in case you haven't: You removed fs::create_dir_all(&path)?; from get_app_config_path(), which is used in the tests, and moved it to process_cmdline(), which is not used in the tests. The tests try to create a file in the config directory after calling get_app_config_path(), which previously also created the path, but does not anymore.

@acuteenvy
Copy link
Contributor Author

Thank you, that was it.

@naseschwarz
Copy link
Contributor

The linter failure regarding duplicate deps probably also does not require an exception, as this should be history once we upgrade to ron 0.9.

@extrawurst
Copy link
Collaborator

@naseschwarz If thats the last thing here, then lets add a cargo-deny exception in the meantime, merge this, create a followup linking to the upstream issue to remember to clean this up once Ron 0.9 is out?

@acuteenvy
Copy link
Contributor Author

I found a better way of doing this - it's possible to load themes from the config directory into the default ThemeSet. The user can then put just the theme name in theme.ron, without the .tmTheme extension.

@extrawurst
Copy link
Collaborator

nice ron 0.9 just landed aswell

@naseschwarz naseschwarz mentioned this pull request Mar 18, 2025
4 tasks
@naseschwarz
Copy link
Contributor

@extrawurst: I have not actually reviewed this, as I was not under the impression that this is ready for review. My remarks were purely in answer to the question about CI failure.

@naseschwarz naseschwarz self-requested a review March 19, 2025 23:18
Copy link
Contributor

@naseschwarz naseschwarz left a comment

Choose a reason for hiding this comment

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

I've looked at this in detail now and find this all quite nicely done.

I've tested this, including error cases I could identify, and found everything working as I would have expected.

image

We only need to consolidate this with #2570, but I suggest to do this separately.

@extrawurst extrawurst merged commit ad32993 into gitui-org:master Mar 19, 2025
21 checks passed
@extrawurst
Copy link
Collaborator

Awesome! Thank you

@acuteenvy acuteenvy deleted the syntax-theme-from-file branch March 19, 2025 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Changing Theme of Syntax Highlighting Disable syntax highlighting
3 participants