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 console color support. #8

Merged
merged 3 commits into from
Jan 29, 2023
Merged

Add console color support. #8

merged 3 commits into from
Jan 29, 2023

Conversation

frere-jacques
Copy link

No description provided.

@frere-jacques frere-jacques mentioned this pull request Jan 6, 2023
@francma
Copy link
Owner

francma commented Jan 9, 2023

Did you try to build the theme with these changed colors and use it? I see that you are using some variables that do not exist - for example bg_dim. Also I don't get why some colors use bg variant and some something else lie bg5 or bg_visual.

<option name="CONSOLE_BLUE_OUTPUT">
  <value>
    <option name="FOREGROUND" value="${blue}" />
    <option name="BACKGROUND" value="${bg_blue}" />
  </value>
</option>
<option name="CONSOLE_CYAN_BRIGHT_OUTPUT">
  <value>
    <option name="FOREGROUND" value="${aqua}" />
    <option name="BACKGROUND" value="${bg5}" />
  </value>
</option>

@frere-jacques
Copy link
Author

Hmm, I have to confess that I didn't try that. I thought it's straight forward, but I see I should do this, sry.

Regarding bg_dim I just assumed you have the complete color palette available. Is there a reason why you don't use it? Otherwise I would add it.

I used bg5 and bg_visual because these colors fit foreground colors best and there is not a background color for each foreground color. What is your opinion on that?

@francma
Copy link
Owner

francma commented Jan 9, 2023

Regarding bg_dim I just assumed you have the complete color palette available. Is there a reason why you don't use it? Otherwise I would add it.

The color palette was updated recently in original Everforest theme. Now it should be updated 62c6639

I used bg5 and bg_visual because these colors fit foreground colors best and there is not a background color for each foreground color. What is your opinion on that?

I would define colors just like in official alacritty theme https://gist.github.com/sainnhe/6432f83181c4520ea87b5211fed27950

@frere-jacques
Copy link
Author

frere-jacques commented Jan 9, 2023

Can you add grey2 too or at least accept it if I add it?
I can looked now in the notebook and jupyter fields too. I can try to compile and test it. And make a new pull request.
There is one thing I couldn;t figure out how to set which is a minor inconvenience when looking in table output of jupyter notebooks and selecting a line. There is a bright highlight and font stays fg. I guess this highlight comes from another point of config.

You mean I should take background and foreground identical? I don't know if that will look good for intellij. There you have eg. Blue BG and FG and Bright Blue BG and FG. I can set Blue and Bright Blue identical, but I guess it's better to have BG and FG different. So I picked the BG colors and for Purple and Aqua the visual and bg5. But I can try it out, if you want.

@frere-jacques
Copy link
Author

frere-jacques commented Jan 9, 2023

Okay so I updated my stuff to remove grey2. I also updated the general foreground highlighing to be fg.
I didn't update the backgrounds yet. I guess the background and foreground are there for a purpose and I am afraid it would break something. Have a look at this screen. I tested it with my community version. I couldn't test the notebook stuff with that.
20230109Jan011673286042

Edit: And sry, since I am really a noob I have no clue, if I have to do a new pull request, since I am now some commits further.

@frere-jacques
Copy link
Author

Did you found time to think about my proposal?

Btw. I found two really annoying colors that appear not to be configurable via ui. I asked on jetbrains page for the labels to replace via theme. Checkout here:
https://intellij-support.jetbrains.com/hc/en-us/community/posts/9648252123922-Help-identifying-color-settings

@francma
Copy link
Owner

francma commented Jan 19, 2023

Please don't use bg_visual. It would make selecting text invisible.

Also please reword the commit messages and maybe even split it to 2 commits (console support, jupyter support).

And please try to test the colors with some cli programs such as htop, to make sure that everything is working correctly.

@frere-jacques
Copy link
Author

Thanks for clarification. I can understand your decision on background visual. I will check how other themes deal with this foreground background topic. And test with htop.

I can put my consolidated edits in two new commits and make a new push release.

@frere-jacques
Copy link
Author

frere-jacques commented Jan 19, 2023

Hi Martin,

I fetched freshly from your upstream repo and made three distinct clean commits. Sry for the many small steps till this point. If you don't see any problem with the contributions, could you upload the new version to jetbrains plugins too?

If I find further optimization I will make new updates :).

@frere-jacques
Copy link
Author

Is anything still wrong with my contribution?

@francma
Copy link
Owner

francma commented Jan 29, 2023

Looks good. Thank you.

I'll upload a new version soon, in the meantime you can use locally build version.

@francma francma merged commit 7a1db66 into francma:master Jan 29, 2023
@francma francma mentioned this pull request Jan 29, 2023
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.

2 participants