-
-
Notifications
You must be signed in to change notification settings - Fork 101
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 custom color schemes #542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks for working on this! This is looking really good and it's definitely a highly requested feature so I appreciate you tackling it.
I left some inline comments with some minor code cleanups and copy changes alignment etc.
I agree that the long combobox here is pretty unweildy. Perhaps instead of trying to include as many styles as we can find, it would be better to have some kind of import mechanism and stick with the same 3 default styles for now. Perhaps in the interest of getting this feature merged more quickly, we should reduce the scope of this branch to only supporting a single custom style and we can expand in later follow-up branches
I think a major interaction issue we have here is that if you ever switch away to another style, your custom style gets wiped. This seems like it would be pretty frustrating for someone who switches between styles. I imagine we should probably persist custom styles in gsettings. Maybe with a "styles" child schema, even if we only support saving one custom style for now
Yes I think we want to maintain the behavior of a style also setting the window chrome to dark or light. The quick style switcher should make it really easy to alternate between styles without having to fiddle with individual settings every time
Hey! Thanks a lot for your review. I implemented almost all of your suggestions, see my comments above.
I would really like to include at least some predefined styles. I am pretty sure that many people would like to customize their terminal at least a little bit. On the other hand I assume that not all of these people (myself included) would put in the effort to enter an existing style color by color or even create a completely custom style from the ground up. Maybe reducing the number of styles to the 10-20 most popular (by some measure) styles would be a good compromise?
Yeah, I am a bit torn about that. My thinking was that most people are not creating styles from the ground up and instead start with one of the existing styles and apply some minor tweaks to their liking. The way it is currently implemented allows that while also allowing the creation of a completely new style. I am not sure how we would allow both use cases if we would save the custom style. I am open to ideas! |
@sebastianlay Hey sorry it took a while to follow up with a review here! This is looking really good. I think we can probably just try to get something in and iterate on it as we go.
At the very least we need to make sure we actually have permission to redistribute any inluded styles. It doesn't look like the |
@danrabbit Hey, you are right. I trimmed the list down to 24 styles (including our three default and the one custom) and checked the licenses for the remaining ones:
If I understand it correctly, the MIT license should be compatible with the LGPL from this repo here, right? I am just not sure what the right place for the above list would be. Some sort of CREDITS file? |
For vendoring third-party styles into this codebase, a reference to the style, the commit it came from, and a copy of the license needs to be included for each vendored style. You probably will want to make those styles able to be read from configuration files that are installed into somewhere in |
To give a bit more detail of how I suggest doing this:
|
Hey @sebastianlay, since this seems to have gotten a bit stuck on the licensing bit would you be interested in merging a version of this without the color schemes so we can get this feature in and then bring back some pre-loaded color schemes in a future branch? |
That sounds like a good idea. I will give the suggestions by @Conan-Kudo a shot and if it takes me too long I will reduce the merge request to the custom color scheme. |
Okay, I will close this in favor of #552 |
This thing fixes #418.
The implementation is based on the recommendations by @danrabbit in the corresponding issue:
This is not actually needed since the selected theme is applied immediately to the opened console.
Additionally this implementation is completely backwards compatible in that it only manipulates the already existing settings and also recognizes the existing default configurations. This has the added benefit that third-party tools and scripts also continue to work.
This also includes an excessive number of predefined themes (194 to be exact) which were all taken from https://github.com/Mayccoll/Gogh/ and the color contrast check uses the code from https://github.com/danrabbit/harvey/
Open questions / calls for improvements:
Themes.vala
is currently an ArrayList. I am pretty new to Vala so my question is whether there is a better data structure for this (something like a dictionary in C#, ideally with afind
method) and a better way to initialize this (again something like collection initializers in C#)?Screenshots:
The fourth button in the color menu that opens the color preference dialog:
The open color preference dialog with a default theme (and good contrast):
A theme selected that has poor contrast: