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

Ensure color codes are valid at runtime if running in 16-color mode #1158

Closed
Dishti-Oberai opened this issue Mar 1, 2022 · 5 comments · Fixed by #1167
Closed

Ensure color codes are valid at runtime if running in 16-color mode #1158

Dishti-Oberai opened this issue Mar 1, 2022 · 5 comments · Fixed by #1167

Comments

@Dishti-Oberai
Copy link
Collaborator

Dishti-Oberai commented Mar 1, 2022

Zulip terminal runs in 16 color depth mode only if the color codes used in themes for the same are acceptable by urwid. Codes of those 16 acceptable colors are mentioned in the documentation.
In case the contributor uses some other color code instead of the aforementioned codes, the terminal shall not run. The prompted error in such a case, (as of now) doesn't indicate the exact reason for the error.
Construct a test in the codebase to check if the 16 color codes used are acceptable ones and prompt an error otherwise.
PS: This issue is a further development of PR #1154.
Related discussions on CZO: https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/16-color.20names.20in.20themes

@Dishti-Oberai
Copy link
Collaborator Author

Dishti-Oberai commented Mar 1, 2022

@zulipbot add "area: colors/styles/themes"
@zulipbot add "area: tests"

@neiljp
Copy link
Collaborator

neiljp commented Mar 2, 2022

@Dishti-Oberai Thanks for filing 👍

To clarify, we already have tests via pytest which should detect this in complete included themes, but if for some reason a theme has a modification then upon loading the application itself it may fail. So, this issue is focused on run-time detection of invalid 16-color strings, so I'm removing the tests label.

Arguably there is nothing stopping users from modifying any part of the code and breaking it in whatever way, but themes are more likely to want to be modified, and if we allow external themes at a later stage then we will want such checking in a different form then.

@neiljp neiljp removed the area: tests label Mar 2, 2022
@neiljp neiljp changed the title Check usage of acceptable 16 Color codes Ensure color codes are valid at runtime if running in 16-color mode Mar 2, 2022
@neiljp
Copy link
Collaborator

neiljp commented Mar 2, 2022

For anyone taking this up, you could also explore whether we test other color depths for validity based on supported codes.

While I understand urwid itself would not want to test every string passed in at point of use, this may be something to be added upstream in urwid if it is not already there, since it is urwid which defines the valid colors and so is best placed to validate them.

@neiljp neiljp added the good first issue Good for newcomers label Mar 4, 2022
@srdeotarse
Copy link
Collaborator

@zulipbot claim

@zulipbot
Copy link
Member

zulipbot commented Mar 5, 2022

Welcome to Zulip, @srdeotarse! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip-terminal/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

srdeotarse added a commit to srdeotarse/zulip-terminal that referenced this issue Mar 15, 2022
srdeotarse added a commit to srdeotarse/zulip-terminal that referenced this issue Mar 17, 2022
Fixes zulip#1158

lint: Fix black code formatting.

/zulipterminal/cli/run.py: Fix Check16ColorCode exception.
srdeotarse added a commit to srdeotarse/zulip-terminal that referenced this issue Mar 17, 2022
Fixes zulip#1158

lint: Fix black code formatting.

/zulipterminal/cli/run.py: Fix Check16ColorCode exception.

lint: Fix black code formatting.
srdeotarse added a commit to srdeotarse/zulip-terminal that referenced this issue Mar 18, 2022
Fixes zulip#1158

lint: Fix black code formatting.

/zulipterminal/cli/run.py: Fix Check16ColorCode exception.

lint: Fix black code formatting.

test: test_themes.py: Add test for validate_colors

themes.py test_themes.py run.py: Improve validate_colors for multiple invalid colors
srdeotarse added a commit to srdeotarse/zulip-terminal that referenced this issue Mar 19, 2022
Fixes zulip#1158

lint: Fix black code formatting.

zulipterminal/cli/run: Fix Check16ColorCode exception.

lint: Fix black code formatting.

test: test/config/test_themes: Add test_validate_colors.

themes/ test_themes/ run: Improve validate_colors.

zulipterminal/config/themes/ tests/config/test_themes: Variable.
srdeotarse added a commit to srdeotarse/zulip-terminal that referenced this issue Mar 20, 2022
Adds validate_colors function to check invalid 16code color names.

Tests added.

Fixes zulip#1158
neiljp pushed a commit to srdeotarse/zulip-terminal that referenced this issue Mar 20, 2022
Adds validate_colors function to check invalid 16code color names.

Tests added.

Fixes zulip#1158.
neiljp pushed a commit that referenced this issue Mar 21, 2022
Adds validate_colors function to check invalid 16code color names.

Tests added.

Fixes #1158.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants