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

Change Color Contrast On Magenta (Console Color) #9831

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Tzadiko
Copy link

@Tzadiko Tzadiko commented Mar 13, 2025

Description
I use the monero-wallet-cli, and I have trouble reading a lot of the information, simply because the colors do not have enough contrast. For example, you can see here that the magenta blends in with the background.

image

Even my non-colorblind friend has a hard time reading this.

image

I looked at the API and noticed that there are better colors that can be used. For example, all these color values would be preferred over the current color scheme.

image

Have a look at the spent transation.

Before

image

After

image

Tzadiko added 9 commits March 12, 2025 17:06
They're never called from outside, and not supposed to be.
Silly mistake, m_console_writer isn't in method scope. Derp.
Called the wrong API, since my intellisense is disabled at the moment.
I previously had it on the constructor (forgot the ~)
@selsta
Copy link
Collaborator

selsta commented Mar 13, 2025

Would it be easier to just change the low contrast colors instead of adding a flag?

Tzadiko added 2 commits March 13, 2025 16:56
…od Calls

Just cleaning up the code here, some spelling mistakes because my intellisense is broken.
@Tzadiko
Copy link
Author

Tzadiko commented Mar 13, 2025

Would it be easier to just change the low contrast colors instead of adding a flag?

Im open to it, I just don't know how others would feel about changing the existing colors, so I figured I'd fit a 'high contrast' color mode in. I'd like more people to chime in before I commit to continuing this change. Some more spots where I want to integrate the IMessageWriter.

@monerobby
Copy link
Contributor

monerobby commented Mar 14, 2025 via email

@Tzadiko
Copy link
Author

Tzadiko commented Mar 14, 2025

As a color blind user I can say I would most likely not remember to or be too lazy to type the flag relative to just struggling through. Therefore, I vote to just change the low contrast colors.

That works. Alternatively I can just set --colorblind to true by default. I kind of like the code I wrote so far, haha. I think it makes what's going on clearer and opens the code to extension down the line, if someone wants to implement a different IMessageWriter (e.g. instead of writing to console, write to a database, or file, etc). In other words, it adds value beyond just changing a color.

@Tzadiko Tzadiko force-pushed the tzadiko/color-blind branch from 3003ddd to 00fb10e Compare March 15, 2025 19:14
@Tzadiko
Copy link
Author

Tzadiko commented Mar 15, 2025

@selsta @monerobby This MR is no longer a WIP. Please see my reply as to why I believe this change is preferred to just simply changing a color code (here).

I changed --colorblind to --highcontrast and made is true by default.

There are some free functions and macros where I cannot retrieve the --highcontrast argument (as that is a property of simplewallet.h). Those methods call failure_msg_writer and message_writer (methods that existed prior) whose bodies not call MessageWriterFactory to retrieve the high-contrast writer. Placing calls to MessageWriterFactory in these existing methods reduces the changeset (the alternative is to replace each of these methods with calls to MessageWriterFactory).

To prevent needing to scope-every method.
@iamamyth
Copy link

I think it makes what's going on clearer and opens the code to extension down the line, if someone wants to implement a different IMessageWriter (e.g. instead of writing to console, write to a database, or file, etc). In other words, it adds value beyond just changing a color.

I would say that this is a false generalization, and therefore subtracts value: The only consumer of this "interface" is a console application. It does not want to write to a database. Its output may, in fact, end up on the filesystem, if stdout or stderr has been redirected, but the OS does the work in that case. Similarly, if a user really wanted to redirect the output from the CLI to "anywhere other than the console", that's why operating system output redirection exists. Simply put: Console applications should write to stdout/stderr, as appropriate. Do not add complexity without utility.

@Tzadiko
Copy link
Author

Tzadiko commented Mar 16, 2025

Do not add complexity without utility.

Okay I'll either amend this MR or open a separate one to just amend the colors, without the IMessageWriter interface and additional code that comes with it.

Edit: Done.

@Tzadiko Tzadiko changed the title WIP: Add Colorblind Support for Console Change Color Contrast On Magenta (Console Color) Mar 16, 2025
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.

4 participants