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

[Bugfix] Resolve #918 Transparent background #925

Merged
merged 2 commits into from Aug 31, 2018

Conversation

JulienPivard
Copy link
Contributor

@JulienPivard JulienPivard commented Jul 21, 2018

Before function getColorCode consider value 'none' like a bad value and search for another value that could match.
Now function getColorCode consider value 'none' like a good value and no longer seeks to replace it.
When it's use to set background the segment become transparent.

Now function getColorCode consider value 'none' like a good value.
When it's use to set background the segment become transparent
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 @JulienPivard !
Good catch there. Thanks for submitting a PR.

@@ -79,6 +79,10 @@ function getColorCode() {
else
echo -n "$1"
fi
# Check if value is none with any case.
elif [[ $1 = [nN][oO][nN][eE] ]]
Copy link
Member

Choose a reason for hiding this comment

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

It would be more readable to write that as elif [[ "${(L)1}" == "none" ]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I add to the next commit

@bhilburn
Copy link
Member

@julienfalque - Thanks so much for putting together this PR! We'll be able to get this in our next release, v0.6.6, which, I'm really happy about.

@onaforeignshore - Do you mean outside of the if / elif / else structure? I'm a fan of keeping that conditional flow to prevent unneeded evaluations, although the lines couldn't be concatenated to:

  # Check if value is none with any case.
  elif [[ "${(L)1}" == "none" ]]; then
    echo -n 'none'

@dritter dritter added this to To do in v0.6.6 Jul 23, 2018
@JulienPivard
Copy link
Contributor Author

You're welcome
@bhilburn you missed your tag ^^

@onaforeignshore
Copy link
Contributor

@bhilburn Just FYI.

Testing: [[ "${(L)s}" == "none" ]]
( repeat 1000000; do; [[ "${(L)s}" == "none" ]]; done; )  0.84s user 0.00s system 99% cpu 0.845 total

Testing: [[ ${s} == [Nn][Oo][Nn][Ee] ]]
( repeat 1000000; do; [[ ${s} == [Nn][Oo][Nn][Ee] ]]; done; )  1.37s user 0.00s system 99% cpu 1.370 total

@JulienPivard
Copy link
Contributor Author

it's already changed in 5ce384f

@dritter dritter mentioned this pull request Aug 5, 2018
@dritter dritter moved this from To do to Done in v0.6.6 Aug 9, 2018
@JulienPivard
Copy link
Contributor Author

JulienPivard commented Aug 18, 2018

@dritter If I change the line

if [[ "$1" == "NONE" || "$2" == "NONE" ]]; then

to

if [[ "${(L)1}" == "NONE" || "${(L)2}" == "NONE" ]]; then

I have that result :
test_back_sans_considerer_la_casse

And if I remove the test I have the same result. For a better rendering I think we should keep that test,
or use an other keyword to designate the first segment.

@onaforeignshore
Copy link
Contributor

@JulienPivard The (L) in ${(L)1} means convert to lowercase, which means your test will always fail as NONE will never be equal to none. To test for NONE you need ${(U)1} instead.

@JulienPivard
Copy link
Contributor Author

JulienPivard commented Aug 18, 2018

@onaforeignshore thanks
It's worse with :

if [[ "${(U)1}" == "NONE" || "${(U)2}" == "NONE" ]]; then

result :
test1

@dritter
Copy link
Member

dritter commented Aug 18, 2018

I fixed a problem in combination with newlines in #960 . Could you try out that PR, and and see if the problem exists there?
And if it does, could you share your configuration with us?

@JulienPivard
Copy link
Contributor Author

@dritter It's work.
test_conf_zsh

@bhilburn bhilburn merged commit 5ce384f into Powerlevel9k:master Aug 31, 2018
@bhilburn
Copy link
Member

Merged into master as part of #944, and will be part of the v0.6.6 release!

@JulienPivard JulienPivard deleted the transparent_back_fix branch August 31, 2018 21:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
v0.6.6
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants