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

fish_config prompt rendering is wrong #2924

Closed
floam opened this Issue Apr 10, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@floam
Member

floam commented Apr 10, 2016

Consider the preview for the Terlar theme:

screenshot 2016-04-09 at 10 14 11 pm

On my terminal it actually renders like this:

screenshot 2016-04-09 at 10 11 04 pm

There are issues:

  1. For certain glyphs like that lightning bolt, the webapp isn't picking the glyphs we'd like see at the terminal.
  2. There is some bug with the colors likely not changing/ending the way they actually do in a terminal (color of the second line).
  3. All of the git-aware commands only have an indication and preview of their true nature if you happen to execute them in a git repo.

A PR for 1 is incoming. 2 is caused because that color changes depending on exit status... and for some reason that's always not 0 in the demonstrations I guess.

After looking over how these are rendered, I wonder if fish wouldn't be better off using a Javascript module that is capable of more accurately and fully rendering a string full of ANSI/vt100 escape sequences the same way a terminal would. It would be neat to show color schemes and the prompt together, by feeding something like tty.js actual fish output.

floam added a commit to floam/fish-shell that referenced this issue Apr 10, 2016

Use fonts found on terminals for the web config.
Fixes fish-shell#2924

Instead of just using Courier New across the board, have the
browser try several likely available fonts before defaulting
to the system's "monospace".
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 10, 2016

Contributor

I remember there is a duplicate issue somewhere. The problem is: how do you actually get the color and font settings of your terminal?

Contributor

ghost commented Apr 10, 2016

I remember there is a duplicate issue somewhere. The problem is: how do you actually get the color and font settings of your terminal?

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Apr 10, 2016

Member

In this case the ugly glyph is caused by the webapp simply using "Courier New"1 as the font for everything. Since the glyph is not present in Courier New the browser is using it's preferred fallback for that glyph, Apple Color Emoji. The fix with a better font list has the side-effect of making the webapp generally look nicer. Who the hell uses Courier New on a terminal? (Or generally likes it? 😃)

The color issue isn't that it needs to match my color setting, I'm not referring to (hard to see) dark blue asterisk being a different shade of blue, but the "➤" on the new line not being white. This one is caused because that color changes depending on exist status and I guess something failed internally before it generated the ANSI for that.

[1]: it was actually using Courier New with a fallback of "monospace" which will use the browser's set monospace... which in most browsers is actually Courier or Courier New.

Member

floam commented Apr 10, 2016

In this case the ugly glyph is caused by the webapp simply using "Courier New"1 as the font for everything. Since the glyph is not present in Courier New the browser is using it's preferred fallback for that glyph, Apple Color Emoji. The fix with a better font list has the side-effect of making the webapp generally look nicer. Who the hell uses Courier New on a terminal? (Or generally likes it? 😃)

The color issue isn't that it needs to match my color setting, I'm not referring to (hard to see) dark blue asterisk being a different shade of blue, but the "➤" on the new line not being white. This one is caused because that color changes depending on exist status and I guess something failed internally before it generated the ANSI for that.

[1]: it was actually using Courier New with a fallback of "monospace" which will use the browser's set monospace... which in most browsers is actually Courier or Courier New.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Apr 10, 2016

Member

PR #2925 gets us closer, with crazy glyphs more often rendering like they do on the terminal.

web config screenshot:
screenshot

Member

floam commented Apr 10, 2016

PR #2925 gets us closer, with crazy glyphs more often rendering like they do on the terminal.

web config screenshot:
screenshot

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 10, 2016

Contributor

Who the hell uses Courier New on a terminal? (Or generally likes it? 😃)

Regardless of the Courier (New) font, AFAICT, lots of front-end devs love those colorful emoji.

… Ah, emoji is not displaying well (#2199), so using single-width chars could be a saner default.

Contributor

ghost commented Apr 10, 2016

Who the hell uses Courier New on a terminal? (Or generally likes it? 😃)

Regardless of the Courier (New) font, AFAICT, lots of front-end devs love those colorful emoji.

… Ah, emoji is not displaying well (#2199), so using single-width chars could be a saner default.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Apr 10, 2016

Member

AFAICT, lots of front-end devs love those colorful emoji.

Huh, I knew that but I assumed this was a webconfig bug and that prompt author couldn't have intended for the color lightning bolt to come through. But I could be mistaken! I don't keep up with the times.

Obviously, after this change, a glyph only available in Apple Color Emoji will still show a color emoji glyph in the preview. On a Mac the glyph substitution works apparently the same way in Terminal.app as it does in webkit (crazy, but true, leads to no fun when a variable width substitution is picked) - so I think after this change it should still be more likely to be the glyph they'll see later.

Member

floam commented Apr 10, 2016

AFAICT, lots of front-end devs love those colorful emoji.

Huh, I knew that but I assumed this was a webconfig bug and that prompt author couldn't have intended for the color lightning bolt to come through. But I could be mistaken! I don't keep up with the times.

Obviously, after this change, a glyph only available in Apple Color Emoji will still show a color emoji glyph in the preview. On a Mac the glyph substitution works apparently the same way in Terminal.app as it does in webkit (crazy, but true, leads to no fun when a variable width substitution is picked) - so I think after this change it should still be more likely to be the glyph they'll see later.

floam added a commit to floam/fish-shell that referenced this issue May 5, 2016

Use better font precedence list based on our docs for webconfig.
Thanks @MarkGriffiths
Fixes fish-shell#2924

This handles more cases better. It would probably be useful
to use proportional fonts in some places for the sake of readability,
but for now, let's have less surprise glyph rendering differences
for the terminal, and prettier typography while keeping it how it was
designed unchanged.

Thanks

floam added a commit to floam/fish-shell that referenced this issue May 5, 2016

Use better font precedence list based on our docs for webconfig.
Thanks @MarkGriffiths
Fixes fish-shell#2924

This handles more cases better. It would probably be useful
to use proportional fonts in some places for the sake of
readability but for now, let's have less surprise glyph
rendering differences for the terminal previews, and
prettier typography while keeping the design unchanged.

floam added a commit to floam/fish-shell that referenced this issue May 5, 2016

Use better font precedence list based on our docs for webconfig.
Thanks @MarkGriffiths
Fixes fish-shell#2924

This handles more cases better. It would probably be useful
to use proportional fonts in some places for the sake of
readability but for now, let's have less surprise glyph
rendering differences for the terminal previews, and
prettier typography while keeping the design unchanged.

@zanchey zanchey added this to the next-2.x milestone May 20, 2016

floam added a commit that referenced this issue Jul 1, 2016

Use fonts found on terminals for the web config.
Instead of just using Courier New across the board, have the
browser try several likely available fonts before defaulting
to the system's "monospace".

Thanks @MarkGriffiths
Fixes #2924

@faho faho added the enhancement label Sep 4, 2016

@faho faho modified the milestones: fish 2.4.0, next-2.x Sep 4, 2016

@faho faho added the release notes label Sep 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment