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 last character having black background #108

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

krayon
Copy link

@krayon krayon commented Apr 12, 2017

Addresses #49 by resetting the colours, then setting the foreground again.

@arialdomartini
Copy link
Owner

This PR has been merged in @cy4n's fork (see cy4n#1)

@arialdomartini
Copy link
Owner

Hi @krayon, @cy4n.
with this PR, on macOS, with Bash on Terminal, I get this result

screen shot 2017-10-07 at 08 15 39

Please, help understand why. It seems like the red color used in the foreground for the last character is not the same red color used as the background in the previous characters.

Do you have any hints?

@cy4n
Copy link
Contributor

cy4n commented Oct 7, 2017

maybe there's something up with your color palette (or mine), but it works fine for me. same red in the last as in the red part before
screen shot 2017-10-07 at 08 31 21

@arialdomartini
Copy link
Owner

arialdomartini commented Oct 7, 2017

Have you seen what discovered by @jgerken and commented in #106?
He writes

This issue is fixed for most clients using bash version 2.53 and above. The default macOS X Terminal.app is a client for which the fix does not work correctly. Terminal uses the default background color as it should but a brighter red for the foreground color - it simply ignores the setting that the dark red should be used. If your client shows similar problems you have to use one of the workarounds from below.

It seems exactly the issue I'm experiencing.
Still I wasn't able to get a correct prompt, neither with his PR (on Terminal.app. No issues with iTerm2)
EDIT
I was able to get a correct prompt even in the bugged Terminal.app applying the trick by @Sgiath, documented in the Readme and which this PR seems to remove, which suggests to set the first palette color equal to the background color.

I'm a bit confused, now, because although your PR seems to be much much simpler than #106, as it removes a bunch of complication in the readme, it seems not to work with Terminal.app. I don't personally use Terminal.app, but of course I don't want to introduce bugs to those who use it.
Help me understand if I'm wrong.

@cy4n
Copy link
Contributor

cy4n commented Oct 7, 2017 via email

@krayon
Copy link
Author

krayon commented Oct 7, 2017

That display issue is most definitely a fault of the terminal (or it's
configuration). And a bug should be filed with it's maintainer. Showing a
different red could just mean it needs it's colours remapped, something that it
may even allow users to do via configuration.

The original code didn't work "properly" for ANY terminal, whereas at least
with this, only one terminal is bugged. I personally would say to anyone
experiencing the issue "use a working terminal" :P but obviously that's your
call. Having said that...

@arialdomartini : Are you saying that it works with the buggy terminal if you
set omg_last_symbol_color to something specific? And if so, what?

If it's \e[0;31m\e[40m then the terminal is truly screwed because that's just
turning on black background AFTER the red foreground.

In the event that it is something like setting the foreground variable etc as
per the original readme, such as:

foreground=160
background=240
omg_last_symbol_color="\[\033[38;5;${foreground}m\]\[\033[48;5;${background}m\]"

Then the solution for the buggy terminal might be setting red='\e[0;38;5;31m'.

If that works, IMHO you're still better off with applying the PR and making the
above recommendation for anyone using that buggy terminal, requiring only them
to make a change to work around the issue in THEIR terminal that they chose.

@krayon
Copy link
Author

krayon commented Oct 7, 2017

Looking at @jgerken 's suggestion in #106 , and reading the ECMA-48 document,
it seem that perhaps the part of his suggestion pertaining to the resetting is
the more correct way since this PR currently resets EVERYTHING, whereas his is
isolated to just the background (though still using the workaround variable etc).

You could see if this works (just changed the reset to BG only):
https://github.com/krayon/oh-my-git/tree/lastcharfix2

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

Successfully merging this pull request may close these issues.

3 participants