Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

Fix icons cut off in some terminal emulators #854

Merged
merged 3 commits into from
Jul 10, 2018

Conversation

ytang
Copy link
Contributor

@ytang ytang commented Jun 1, 2018

For terminal emulators such as suckless, the most common scenario of icons being cut off is the virtual identifier before the segment content, even though there is a space between the icon and the text. The reason is that we add color only to the icon but not to the space next to it. To fix it, just add color to both the icon and its neighboring space. It allows the terminal emulator to render the icon correctly without introducing any visual side effects.

Here is a screenshot showing the cut-off before (the first line) and after (the second line) applying the patch:
powerlevel9k-fix-icons-cut-off

This is superior to existing workarounds mentioned in #231 #453 #794 since we do not need to waste an extra space.

Copy link
Member

@dritter dritter left a comment

Choose a reason for hiding this comment

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

Hi @ytang !
Awesome. This has been a pain for a long time. Interestingly this fix is quite simple. I never thought that this is a problem on our side...

The changes look good to me, but we should test this on other problematic Terminal Emulators (like Konsole) as well.

@@ -156,12 +156,12 @@ left_prompt_segment() {
if [[ -n $6 ]]; then
visual_identifier="$(print_icon $6)"
if [[ -n "$visual_identifier" ]]; then
# Add an whitespace if we print more than just the visual identifier
[[ -n "$5" ]] && visual_identifier="$visual_identifier "
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment why there is a whitespace before ending the foreground color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -240,12 +240,12 @@ right_prompt_segment() {
if [[ -n "$6" ]]; then
visual_identifier="$(print_icon $6)"
if [[ -n "$visual_identifier" ]]; then
# Add an whitespace if we print more than just the visual identifier
[[ -n "$5" ]] && visual_identifier=" $visual_identifier"
Copy link
Member

Choose a reason for hiding this comment

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

Could you here add a comment about the whitespace as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ytang
Copy link
Contributor Author

ytang commented Jun 1, 2018

Thanks for the swift approval! It works for Konsole, too.

@dritter
Copy link
Member

dritter commented Jun 1, 2018

Just tested this in iTerm2, Terminal, Konsole, hyper and PHPStorm. Works like a charm in all of them.
Thx @ytang

dritter added a commit to dritter/powerlevel9k that referenced this pull request Jun 6, 2018
Conflicts were:
- test/segments/command_execution_time.spec and
test/segments/go_version.spec: All conflicts come from renaming color
names on next (Powerlevel9k#703) and adding a whitespace to the Visual Identifier
(Powerlevel9k#854) on master.
dritter added a commit to dritter/powerlevel9k that referenced this pull request Jun 6, 2018
Conflicts were:
- test/segments/command_execution_time.spec and
test/segments/go_version.spec: All conflicts come from renaming color
names on next (Powerlevel9k#703) and adding a whitespace to the Visual Identifier
(Powerlevel9k#854) on master.
@dritter dritter mentioned this pull request Jun 7, 2018
@bhilburn
Copy link
Member

bhilburn commented Jun 8, 2018

Thanks so much, @ytang! Also, kudos for searching through our previous bugs to see where else this issue has been mentioned, and what was discussed there. Thanks for the solid PR! =)

@bhilburn
Copy link
Member

bhilburn commented Jun 8, 2018

Given @dritter's testing & approval, this PR will go into the next release.

@bhilburn bhilburn merged commit 9c4203b into Powerlevel9k:master Jul 10, 2018
@IevgenSobko
Copy link

IevgenSobko commented Aug 21, 2018

@dritter @ytang has this fix found a way to master?
It seems doesn't work for right-hand segments. After update left-hand segments got fixed under Konsole but not right-hand ones
2018-08-21__23-47-01
Here I'm using custom configuration for time icon
POWERLEVEL9K_TIME_ICON="\uF49B"
But at the same time ram segment icon I guess default ant it gets broken

@braydie
Copy link

braydie commented Aug 30, 2018

Yeah it was working for me to, but isn't now

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

Successfully merging this pull request may close these issues.

5 participants