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

Added themes to IsIdentifiableReviewer #797

Merged
merged 3 commits into from
Jun 15, 2021
Merged

Added themes to IsIdentifiableReviewer #797

merged 3 commits into from
Jun 15, 2021

Conversation

tznind
Copy link
Contributor

@tznind tznind commented Jun 14, 2021

Proposed Changes

image

Added option to specify a theme. Run with --theme ./mytheme.yaml

# Supported Colours:

# Black
# Blue
# Green
# Cyan
# Red
# Magenta
# Brown
# Gray
# DarkGray
# BrightBlue
# BrightGreen
# BrightCyan
# BrightRed
# BrightMagenta
# BrightYellow
# White

FocusForeground: Black
FocusBackground: Gray

DisabledForeground: Black
DisabledBackground: Black

HotFocusForeground: BrightBlue
HotFocusBackground: Gray

HotNormalForeground: BrightBlue
HotNormalBackground: Blue

NormalForeground: White
NormalBackground: Blue

Types of changes

What types of changes does your code introduce? Tick all that apply.

  • Bugfix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-Only Update (if none of the other choices apply)
    • In this case, ensure that the message of the head commit from the source branch is prefixed with [skip ci]

Checklist

By opening this PR, I confirm that I have:

  • Reviewed the contributing guidelines for this repository
  • Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able)
  • Updated any relevant API documentation
  • Created or updated any tests if relevant
  • Created a news file
    • NOTE: This must include any changes to any of the following files: default.yaml, any of the RabbitMQ server configurations, GlobalOptions.cs
  • Listed myself in the CONTRIBUTORS file 🚀
  • Requested a review by one of the repository maintainers

Issues

If relevant, tag any issues that are expected to be resolved with this PR. E.g.:

@tznind tznind requested a review from rkm June 14, 2021 11:42
@coveralls
Copy link

coveralls commented Jun 14, 2021

Coverage Status

Coverage decreased (-0.2%) to 53.919% when pulling 80ac023 on reviewer-colors into 0a10a4e on master.

@rkm
Copy link
Member

rkm commented Jun 14, 2021

Looks good!

If the default theme doesn't display properly on Guacamole, would it make sense to define base values for these in the services yaml, so that users can always see correct colours by default but override with their own theme if they choose?

Copy link
Member

@rkm rkm left a comment

Choose a reason for hiding this comment

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

Could you add a short test for the TerminalGuiTheme deserialization from yaml?

@tznind
Copy link
Contributor Author

tznind commented Jun 14, 2021

If the default theme doesn't display properly on Guacamole, would it make sense to define base values for these in the services yaml, so that users can always see correct colours by default but override with their own theme if they choose?

I think it's more complicated than that sadly. The colors worked fine with 1.0.0 of Terminal.Gui but there seems to be an issue since 1.1.1 with Guacamole only. I know there was work done in the Terminal.Gui library on color handling in the NCurses driver. The colours still work fine with the NetDriver (although that has its own issues with Guacamole). Which you can access by using the flag --usc.

This PR is just to give us more control within the environment on what colours to use so we can debug or tinker with what works without having to recompile. Especially with regressions if a subsequent update to the library introduces new issues or there are changes to Guacamole.

I agree we should add this to the yaml file but I would do that later with a similar PR to #795 and do it for all the yaml settings in the reviewer at once (not just theme on it's own).

More Info: When RDMP boots up in Guacamole it also had color issues (with 1.1.1 Terminal.Gui library) but changing to the green color scheme fixed the problem but it might be that the very act of creating a new ColorScheme and applying it (Regardless of the colors) is what solves the problem not the actual colors themselves

@tznind tznind merged commit 02d3d71 into master Jun 15, 2021
@tznind tznind deleted the reviewer-colors branch June 15, 2021 08:24
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.

Add Guacamole compatible colour themes
3 participants