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 full color mode option #851

Merged
merged 6 commits into from
Nov 11, 2022
Merged

Add full color mode option #851

merged 6 commits into from
Nov 11, 2022

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Nov 11, 2022

@xsebek xsebek requested review from kostmo and byorgey November 11, 2022 21:39
app/Main.hs Outdated Show resolved Hide resolved
@byorgey
Copy link
Member

byorgey commented Nov 11, 2022

What does "full color mode" mean, exactly? And why would we not want it to be the default?

@xsebek
Copy link
Member Author

xsebek commented Nov 11, 2022

@byorgey I think it means showing "a full 24-bit truecolor".

Not all terminals support this. Presumably, Brick/Vty try to autodetect this, but sometimes it does not work for an unknown reason.

I could allow setting any of the Vty colour values so that even the terminals limited to a lower colour range can experiment with this. Or maybe someone could have (not set) a TERM value which is interpreted by Vty as only having 8bit colours.

@byorgey
Copy link
Member

byorgey commented Nov 11, 2022

OK, sounds good. But in what sense does this solve #815 ?

@xsebek
Copy link
Member Author

xsebek commented Nov 11, 2022

It allows you to override the default on the application side, isolating the problem.

Consider this scenario:

A hypothetical player named Peter will try to play Swarm, but it will show no colours.
Peter runs swarm --help and learns about --full-color option.
Once he runs Swarm again with the option, he will either:

  1. see colours, confirming that the problem is with how Swarm detected his terminal capabilities (not setting TERM is on him though)
  2. still see no colours, which means that he is using a Windows console or a revived IBM 2741
  3. see colours 99% of the time and can not figure out why it does not work in rare cases, so he shrugs and goes on with his life, never thinking about it again (or he can write an Issue upstream)

@byorgey
Copy link
Member

byorgey commented Nov 11, 2022

Sure, that all sounds great, and I am in favor of adding this flag. But in case (3) we still have exactly the same issue represented by #815. So I still don't see why adding this flag resolves #815. Unless the idea is "well, we did what we could and it's not worth worrying about a rare, transient, unreproducible issue".

@xsebek
Copy link
Member Author

xsebek commented Nov 11, 2022

@byorgey well, I am actually doing more than I could - I just added more options to select 8/16/full colour to see which one we really need. It turns out that with 8/16bit colours you get something like this:

image

I did this to better understand #815 and also to improve the player experience when dealing with any terminal colour issue.


I feel this should be enough to close our issue, given the response upstream. 🙂

app/Main.hs Outdated Show resolved Hide resolved
src/Swarm/TUI/Model.hs Outdated Show resolved Hide resolved
Co-authored-by: Brent Yorgey <byorgey@gmail.com>
@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Nov 11, 2022
@mergify mergify bot merged commit 0f98116 into main Nov 11, 2022
@mergify mergify bot deleted the color-mode branch November 11, 2022 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes colors don't work
2 participants