-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
A "_random_" color scheme option. Any new window chooses randomly from those in settings.schemes. #9845
Conversation
Cases I don't know how to handle yet:
|
Chosen Implementation:
Another implementation I considered: "_random_" comes as a "dummy" default scheme which is never used -
|
I think this is a great writeup! My thoughts:
That seems sensible to me I look forward to the whole PR! |
Okay great! Thank you for the feedback and I look forward to it too haha :) |
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
Before I decided how to choose a scheme randomly, I wanted to make sure I could apply a scheme name which technically doesn't exist. The first thing I tried was searching the following warning which comes up whenever I set the scheme to "_random":
Which took me down this line of files/functions/variables:
At that point I realized I didn't understand the warnings system well enough to trace backward easily so I thought I would try a less frictioned way to find the warning throwing code: If the warning is thrown every time a scheme name is not found in ColorScheme, just find every instance where the color schemes are searched. In this push I tried to do so. I tried to find and change every instance of .TryLookup() on a ColorScheme object I could find (when it didn't work when I changed the first one I just changed them all), but when I ran the terminal it still threw the warning :[ Could anyone give me some pointers on where to account for this error? Or maybe showing me where the documentation on how all these warnings objects mix with one another if that's the route I should go down? Or just any advice really, haha, thank you. |
Also, this probably won't apply once I figure out where the error is actually handled, but all the functions I've had to edit so far ask for some behavior based on a ColorScheme object. Whether it was calling .ApplyColorScheme([some scheme which should come from "/_random"]), or "get_strong"?, or something like that. I just let "/_random" be L"Campbell" in these intermediate steps, but it kind of makes me reconsider whether there should be a dummy "/_random" scheme? Because its random name wouldn't have been chosen by this point, right? |
WAIT READ FIRST PLS : |
Hey so that warning is coming from
I think it's a cool idea, but A: I think it would be harder to implement. B: It would involve writing out the settings file when someone edits the scheme, even in json, and that's highly fraught with peril. We just tend to avoid it, because that means the file is going to get reformatted, and people generally don't love that. And C: I personally want this setting for "pick a random scheme each and every time". Like, I want the setting to be useful for booting to different schemes each time. |
After reading the diff, it almost seems like we should have a helper for GlobalSettings().ColorSchemes().TryLookup(*schemeName) that's instead something like: ColorScheme GlobalAppSettings::ResolveColorScheme(winrt::hstring name) {
if (const auto scheme = ColorSchemes().TryLookup(realArgs.SchemeName()))
{
return scheme;
}
else if (realArgs.SchemeName() == winrt::to_hstring(L"_random"))
{
// do whatever to get a random scheme
return scheme;
}
return nullptr;
} So that we can do the logic for |
That's a really great solution! Thank you so much for pointing me in the right direction, I had been looking for it for so long! I'm excited to get working on it now, and I really appreciate you helpful and thoughtful responses :) |
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
Update: I just cleaned the project in visual studio too and rebuilt but that wasn't the problem either. I'll keep working and seeing if I can find what I'll have to remove to get this error to stop. |
Weird. Hopefully I'll get a chance later this week to check out this branch and investigate. |
Hey there! Since this one has been in draft for a couple years, I'm hitting it as part of my cleanup spree.. I love this idea, and I'm sorry we weren't able to converge. You're always welcome here! 😄 |
Summary of the Pull Request
If the user sets schema = "_random", then every new terminal window they open will have a random color scheme applied. This color scheme is chosen from existing schemes in settings.schemes (as compared to colors/fonts being randomly generated).
Detailed Description of the Pull Request / Additional comments
Pull request will pass all of the following tests:
Validation Steps Performed
N/A yet