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(nodejs): use e718 as the default of symbol in node configuration #3533

Merged
merged 3 commits into from Jan 30, 2022

Conversation

TrickyPi
Copy link
Sponsor Contributor

@TrickyPi TrickyPi commented Jan 28, 2022

Description

Use e718 as the default of symbol in node configuration

Motivation and Context

Closes #2546

Screenshots (if appropriate):

img

How Has This Been Tested?

Visually checked on Iterm2, hyper and mac terminal.

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@TrickyPi TrickyPi changed the title fix:use e718 as the default of symbol in node configuration fix: use e718 as the default of symbol in node configuration Jan 28, 2022
@chipbuster
Copy link
Contributor

Tested on the following on Arch:

  • Alacritty
  • WezTerm
  • Hyper
  • Yakuake
  • st
  • Xfce Terminal
  • Konsole
  • Terminator
  • Kitty

Works on all of them except for Hyper and Xfce Terminal, and the rendering was broken on those before this change anyways.

@chipbuster
Copy link
Contributor

Tested on MacOS under the following terminals:

  • Terminal
  • iTerm2
  • Hyper
  • Alacritty
  • Kitty

Works on all of them.

@chipbuster
Copy link
Contributor

I would appreciate it if someone could test this on Windows, since my current setups are based on virtualized systems which do not support OpenGL. At the least, I'd like to see Windows Terminal, one of the OpenGL terminals (Alacritty, WezTerm, Kitty), and, if possible, the standard conhost.

@TrickyPi
Copy link
Sponsor Contributor Author

Tested on windows under Alacritty terminal.
before changing:
before
after changing:
after

@davidkna
Copy link
Member

Windows Terminal, Alacritty and WezTerm seem fine. conhost didn't list my nerd fonts as a font option and was thus broken.

@chipbuster
Copy link
Contributor

Okay, in that case, all we need to do is make the appropriate edit in docs/config and docs/presets and we should be good to merge.

Copy link
Contributor

@chipbuster chipbuster left a comment

Choose a reason for hiding this comment

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

Unlucky for us, the new symbol is part of the NerdFonts codepoint leakage reported in 2019. I don't have a feel for how close they are to fixing that issue, and I don't think it should be a blocker, but we should make a mental note that this is a potential problem for anyone who uses a language that uses CJK codepoints.

docs/config/README.md Show resolved Hide resolved
Copy link
Contributor

@chipbuster chipbuster left a comment

Choose a reason for hiding this comment

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

Some confusion over whether my browser or Nerd Fonts is broken at the moment, but either way I don't think it's a big enough reason to hold this PR back.

If it turns out that this is, in fact, a codepoint leakage, we'll probably need to add an FAQ explaining what's going on.

@andytom andytom changed the title fix: use e718 as the default of symbol in node configuration fix(nodejs): use e718 as the default of symbol in node configuration Jan 30, 2022
@andytom andytom merged commit 65f2975 into starship:master Jan 30, 2022
@andytom
Copy link
Member

andytom commented Jan 30, 2022

Thank you for your contribution @TrickyPi and thank you for the indepth testing @chipbuster and @davidkna.

@TrickyPi TrickyPi deleted the fix-2546 branch February 17, 2022 12:28
Perniciosius pushed a commit to Perniciosius/starship that referenced this pull request Feb 21, 2022
…tarship#3533)

* fix: use e718 as the default of symbol in node configuration

* wip: change nodejs symbol in docs/config & add a nodejs symbol configuration in docs/presets

* wip: update CONTRIBUTING.md
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.

New nodejs character positioned incorrectly
4 participants