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

Fixes #2348. The Basic Colors scenario doesn't output the blue on white properly. #2349

Merged
merged 3 commits into from Feb 15, 2023

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Feb 13, 2023

Fixes #2348 - After the adding of the Color.Invalid the calculation used doesn't work anymore. Converting to an array and changing from foreach to for statement fixed the issue.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tig
Copy link
Collaborator

tig commented Feb 14, 2023

Fixes #2348 - After the adding of the Color.Invalid the calculation used doesn't work anymore. Converting to an array and changing from foreach to for statement fixed the issue.

It was my goal when I added Color.Invalid was to do it in a way that wasn't a breaking change. You've demonstated that it WAS a breaking change. What the Scenario is doing is likely a rare use-case, but the fact you had to change Scenario code means what I did was a breaking change.

Since my change to add Color.Invalid is still in develop we should consider a way to fix the underlying code such that your Scenario code continues to function as it did previously.

What do you think?

@BDisp
Copy link
Collaborator Author

BDisp commented Feb 14, 2023

It was my goal when I added Color.Invalid was to do it in a way that wasn't a breaking change. You've demonstated that it WAS a breaking change. What the Scenario is doing is likely a rare use-case, but the fact you had to change Scenario code means what I did was a breaking change.

That really is a breaking change.

Since my change to add Color.Invalid is still in develop we should consider a way to fix the underlying code such that your Scenario code continues to function as it did previously.

If you also look at the InvertedColors scenario it have a weird behavior. The Color.Invalid falls to a valid color which it's not what you want.

What do you think?

I think it should be removed. I will do that and also add a unit test with a code like the scenario to ensure that the Color enum won't break. The HasValidColors must be compare by the -1 value but without having new value in the Color enum. I'll submit a commit here with the changes.

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

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

Thank you!

@tig tig merged commit 65607ba into gui-cs:develop Feb 15, 2023
@BDisp BDisp deleted the basic-colors-scenario-fix_2348 branch February 15, 2023 00:15
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.

The Basic Colors scenario doesn't output the blue on white properly.
2 participants