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

Fix of [#38730], solarized colorscheme bug fix #38792

Closed
wants to merge 2 commits into from
Closed

Fix of [#38730], solarized colorscheme bug fix #38792

wants to merge 2 commits into from

Conversation

Arkoniak
Copy link
Contributor

@Arkoniak Arkoniak commented Dec 9, 2020

Fixes #38730
Add workaround of solarized scheme bug and expand printstyled functionality.

@Arkoniak Arkoniak changed the title Solarized colorscheme bug fix #38730 Fix of [#38730], solarized colorscheme bug fix Dec 9, 2020
end

# filename, separator, line
Copy link
Member

Choose a reason for hiding this comment

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

Dropped comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -30,6 +30,8 @@ answer_color() = text_colors[repl_color("JULIA_ANSWER_COLOR", default_color_answ
stackframe_lineinfo_color() = repl_color("JULIA_STACKFRAME_LINEINFO_COLOR", :bold)
stackframe_function_color() = repl_color("JULIA_STACKFRAME_FUNCTION_COLOR", :bold)

alt_light_black_color() = repl_color("JULIA_ALT_LIGHT_BLACK_COLOR", :light_black)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like the "alt_light_black" name. Can we come up with a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely do not mind any other name, consider it as a placeholder.

Initially, I went with :stackframe_color but then decided that :alt_light_black is better.

@Arkoniak
Copy link
Contributor Author

Arkoniak commented Dec 22, 2020

Sorry for bumping this up.

After I lived some time with this patch, it turns out that it doesn't solve all the problems.

  1. Pkg also uses :light_black but in order to fix it, I need to change the code of some other package.
  2. There is no guarantee that somebody somewhere is not going to use :light_black instead of :alt_light_black and it'll start over again. It will turn into an endless whack-a-mole fix, which is not good.

As an alternative solution, maybe substitute :light_black directly in printstyled? I.e. if we have an alternative color for light_black then use it instead of the original light_black? I do not quite like this approach because it messes with color names, but on the other hand, it'll work 100% of the time and will be applied only in case when colorscheme is broken anyway.

@Arkoniak
Copy link
Contributor Author

Arkoniak commented Dec 22, 2020

As an alternative, one can redefine :light_black directly in startup.jl, but I had issues with that, when I was running julia as a script from the prompt. It looks like startup.jl can be read after script is read. For example if script is malformed, than I get error messages in original invisible :light_black. It seems that ENV version is the only one that can guarantee that color is redefined before any other processing takes place.

@Keno
Copy link
Member

Keno commented Dec 25, 2020

I think a general colorscheme mechanism that lets people remap the named colors in the terminal would be fine. Shouldn't be just light_black at that point though.

@Arkoniak
Copy link
Contributor Author

I'll never finish this PR and it'll never be accepted in it's current form, so there is no need to keep it open.

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.

Unreadable stacktrace in solarized theme
2 participants