[Bugfix] virtualenv prompt displaying

[ymage]

Fix #1127

[dritter]

Thx @ymage . The code LGTM.

Related to #1079 (comment)

[ymage]

Yes, it reverts #1079 and it seems to me more logical to use true/false value for this var.
And fix the problem here.
About prezto, I suppose it's not powerlevel9k job to handle prezto regression ... 🙉

[dritter]

This change seems right to me, although the rest seems like a minefield to me:
So, prezto and pure set VIRTUAL_ENV_DISABLE_PROMPT to 12 (prezto sorin-ionescu/prezto@ee885d4 , Issue sorin-ionescu/prezto#1627 , PR in Pure: sindresorhus/pure#381) , we check for unequal true. Not sure how preztos python works, but you probably get a shortcut to quickly enable/disable a python env. If so, that won't work with our segment.
Long story short:
Maybe we should check for the same magic value like prezto/pure..
WDYT?

[ymage]

Well,

As @shengje said, VIRTUAL_ENV_DISABLE_PROMPT seems to expect a true/false value.
And I am not fond of magic values especially considering they comes from an other project.

Other idea : we could rely on another variable and leave VIRTUAL_ENV_DISABLE_PROMPT on its own ...

[dritter]

If a user wants to not show that segment, he could simply remove that segment. But we want to interact with other tools that set VIRTUAL_ENV_DISABLE_PROMPT. So we should look at that value.
Another solution could be the one you described, to look at another variable.

Could you describe your workflow with virtualenv, so we (or at least I) get a better picture? How you use this variable?
Do you activate a virtualenv and at some point set VIRTUAL_ENV_DISABLE_PROMPT by hand? Or do you activate/deactivate a virtualenv with another wrapper (like pretty python module) and that should show/hide the segment automatically?

[ymage]

My issue was I was unable to display the segment whatever the value set for VIRTUAL_ENV_DISABLE_PROMPT since the other fix.

I was using powerlevel9k with zplug as zsh framework. I switched to zplugin. But it didn't fix anything.
I tried to activate virtualenv by hand and with pew. None of them allowed to display the segment until I found this variable was erroneously tested.

But I am open if you have some good idea for me :)

[dritter]

But I am open if you have some good idea for me :)

Unfortunately not yet.

I am asking all this questions, because I want to figure out if we need to change the way we look at this variable. As you described your workflow, you seem to set the variable by hand. But I am not sure if this is the right thing to do. There might be users that use a plugin that sets that variable automatically. In this case, we should interpret the value.

There are multiple cases we should look at:

1. Users that set VIRTUAL_ENV_DISABLE_PROMPT by hand
2. Users that use frameworks that set VIRTUAL_ENV_DISABLE_PROMPT, by using a wrapper script that enables/disables a virtualenv.

So in the first case we are free to use any value, because we can tell that value to the user. But in the second case we are not free.

To end this argument, I'd like to hear some other voices. I can't tell if this is right or wrong.

[ymage]

I just saw that oh my zsh virtualenv plugin set VIRTUAL_ENV_DISABLE_PROMPT=1
But it is for 5 years.
And the virtualenv prompt segment is broken only for months

[ymage]

So I tried to disable the oh my zsh virtualenv plugin and it worked.
I suppose the #1079 fix conflict with it.
Not sure #1079 is the best way to do but now, it's ok for me.

[dritter]

Maybe we should change the code to something like (from the next branch):

# Early exit; $virtualenv_path must always be set.
[[ -z "$virtualenv_path" ]] && return

# Check if VIRTUAL_ENV_DISABLE_PROMPT is set to false, or is a numerical value
if [[ "$VIRTUAL_ENV_DISABLE_PROMPT" == "false" ]] || (( VIRTUAL_ENV_DISABLE_PROMPT )); then
  p9k::prepare_segment "$0" "" $1 "$2" $3 "${VIRTUAL_ENV:t}"
fi

Could you check if this would work with all combinations (like 12, 1, false)?

[ymage]

I can't find p9k::prepare_segment function/method ?

[ymage]

But this did the trick :

  [[ -z "$virtualenv_path" ]] && return

  # Check if VIRTUAL_ENV_DISABLE_PROMPT is set to false, or is a numerical value
  if [[ "$VIRTUAL_ENV_DISABLE_PROMPT" == "false" ]] || (( VIRTUAL_ENV_DISABLE_PROMPT )); then
    "$1_prompt_segment" "$0" "$2" "blue" "$DEFAULT_COLOR" "$(basename "$virtualenv_path")" 'PYTHON_ICON'
  fi

[dritter]

Awesome! @josselinauguste and @shengje could you confirm this works?

[t0mab]

I confirm it works great fix !

[dritter]

Okay. I digged some more into that topic and I finally understand how that virtualenv is set and what VIRTUAL_ENV_DISABLE_PROMPT is meant for. I always thought it is set by a wrapper script that wants to disable the prompt (in our case the segment) if the virtualenv was disabled. But it turns out the opposite is the case. That variable is meant to disable the virtualenvs own prompt rendering. So we probably want to set that variable, if the segment is used, and we don't need to worry about any value it holds for disabling the segment. Instead we can solely rely on $VIRTUAL_ENV. If this is set, we display the segment, if not, hide the segment.
@ymage Could you change your PR once more, and remove that condition entirely and set that variable instead? Sorry for the forth and backs.

Code History

Interestingly that condition took several turns, true/false, zero, non-zero, we had them all (ae035e5 , 5c412b4 ). But it was in P9K since the very first commit.

[ymage]

If I get it right, it means disable the abilty to use wrapper variables to manage segment displaying and let it only set via theme configuration. Right ?

[dritter]

There are three aspects:

1. Showing/Hiding the P9K virtualenv segment
   No. Showing/Hiding the segment is still controlled via $VIRTUAL_ENV. If that variable is empty, we hide the segment, otherwise the segment is displayed.

2. Prevent activate from writing a prompt
   What we want to avoid is that the activate script, that enables the virtualenv, also tries to write a prompt (which would interfere with P9K). For this we need to set VIRTUAL_ENV_DISABLE_PROMPT=1. That is the variable that controls showing a prompt from activate. Ideally this is done in the segment, because here we know that the user wants to use it.

3. Interoperability with ZSH frameworks
   Prezto sets VIRTUAL_ENV_DISABLE_PROMPT=12 for compatibility with pure. If we set now 1, this might break something in prezto, if they use that variable, and rely on that value (may be unlikely). But to make it perfect, we could set it, if it was not defined beforehand, like this:

export VIRTUAL_ENV_DISABLE_PROMPT=${VIRTUAL_ENV_DISABLE_PROMPT:-1}

[ymage]

I understand. Even if I found pretty sad we have to deal with other framework mess today ... And probably tomorrow. :(
Until now, there is no global exported variable in p9k theme and I can't bring myself to be the first to do it 🥇
Maybe we can just get it documented to ensure it is set in .zshrc if needed. WDYT ?

[ymage]

I mean I am not sure it's the theme job to set such a variable ...

[dritter]

Well. On one hand I agree with you, but on the other hand, if we don't do this, I sense a lot of complaints coming towards us.. So I think it is better to set that variable..

[ymage]

Not sure.
#1128 (comment) seems to do the job without to set anything.

[dritter]

Not quite. This code you mentioned works in the wrong direction. It reads $VIRTUAL_ENV_DISABLE_PROMPT and disables the segment based on that information. But what this variable is intended to do is to disable the activate scripts (that one the user uses to enable a new virtual env) own prompt rendering mechanism. If this variable is not set, activate modifies $PROMPT and prepends it with the name of the virtual env. It might be, that P9K overwrites the value for $PROMPT then, but at least setting $PROMPT twice (first time by activate, second time by P9K) could be avoided.

[ymage]

I tried raw virtualenv w/activate script.
I tried virtualenvwrapper
I tried pew
Each of them seems to work fine with this code and the OMZ virtualenv plugin which is in charge to set $VIRTUAL_ENV_DISABLE_PROMPT.
I am sorry but I don't think it's the theme responsibility to set this variable.

Can you share some context in which it is an issue ?

[dritter]

Here is a test for that segment. I hope that makes it a bit clearer what I am trying to say..

#!/usr/bin/env zsh
#vim:ft=zsh ts=2 sw=2 sts=2 et fenc=utf-8

# Required for shunit2 to run correctly
setopt shwordsplit
SHUNIT_PARENT=$0

function setUp() {
  export TERM="xterm-256color"
  __P9K_HOME="${PWD}"
  # Load Powerlevel9k
  source powerlevel9k.zsh-theme

  # Test specific
  # Backup old virtualenv
  __OLD_VIRTUAL_ENV="${VIRTUAL_ENV}"
}

function tearDown() {
  unset VIRTUAL_ENV

  [[ -n "${__OLD_VIRTUAL_ENV}" ]] && VIRTUAL_ENV="${__OLD_VIRTUAL_ENV}"

  unset __OLD_VIRTUAL_ENV
}

function mockVirtualenv() {
  VIRTUAL_ENV="$1"

  # Taken from https://github.com/pypa/virtualenv/blob/685b1af0405e0c94411951389310960ff7ac8e1e/virtualenv_embedded/activate.sh
  # With added forced fail.
  if [ -z "${VIRTUAL_ENV_DISABLE_PROMPT-}" ] ; then
      _OLD_VIRTUAL_PS1="${PS1-}"
      if [ "x__VIRTUAL_PROMPT__" != x ] ; then
          PS1="__VIRTUAL_PROMPT__${PS1-}"
      else
          PS1="(`basename \"$VIRTUAL_ENV\"`) ${PS1-}"
      fi
      export PS1

      fail "Executing unnecessary code!"
  fi
}

function testVirtualenvDoesNotExecuteUnnecessaryCodeWithVirtualEnvDisablePromptTruthy() {
  local -a POWERLEVEL9K_LEFT_PROMPT_ELEMENTS
  POWERLEVEL9K_LEFT_PROMPT_ELEMENTS=(virtualenv)

  local VIRTUAL_ENV_DISABLE_PROMPT="true"
  # Activate virtualenv
  mockVirtualenv "mv/super/venv"
  assertEquals "Unable to set VIRTUAL_ENV_DISABLE_PROMPT=true" "%K{004} %F{000}venv %k%F{004}%f " "$(build_left_prompt)"

  VIRTUAL_ENV_DISABLE_PROMPT="1"
  # Activate virtualenv
  mockVirtualenv "mv/super/venv"
  assertEquals "Unable to set VIRTUAL_ENV_DISABLE_PROMPT=1" "%K{004} %F{000}venv %k%F{004}%f " "$(build_left_prompt)"

  VIRTUAL_ENV_DISABLE_PROMPT="12"
  # Activate virtualenv
  mockVirtualenv "mv/super/venv"
  assertEquals "Unable to set VIRTUAL_ENV_DISABLE_PROMPT=12" "%K{004} %F{000}venv %k%F{004}%f " "$(build_left_prompt)"

  VIRTUAL_ENV_DISABLE_PROMPT="xx"
  # Activate virtualenv
  mockVirtualenv "mv/super/venv"
  assertEquals "Unable to set VIRTUAL_ENV_DISABLE_PROMPT=xx" "%K{004} %F{000}venv %k%F{004}%f " "$(build_left_prompt)"
}

function testVirtualenvDoesNotExecuteUnnecessaryCodeWithVirtualEnvDisablePromptFalsy() {
  local -a POWERLEVEL9K_LEFT_PROMPT_ELEMENTS
  POWERLEVEL9K_LEFT_PROMPT_ELEMENTS=(virtualenv)

  local VIRTUAL_ENV_DISABLE_PROMPT="false"
  # Activate virtualenv
  mockVirtualenv "mv/super/venv"
  assertEquals "Unable to set VIRTUAL_ENV_DISABLE_PROMPT=false" "%K{004} %F{000}venv %k%F{004}%f " "$(build_left_prompt)"

  VIRTUAL_ENV_DISABLE_PROMPT="0"
  # Activate virtualenv
  mockVirtualenv "mv/super/venv"
  assertEquals "Unable to set VIRTUAL_ENV_DISABLE_PROMPT=0" "%K{004} %F{000}venv %k%F{004}%f " "$(build_left_prompt)"
}

function testVirtualenvDoesNotExecuteUnnecessaryCodeWithVirtualEnvDisablePromptUnset() {
  local -a POWERLEVEL9K_LEFT_PROMPT_ELEMENTS
  POWERLEVEL9K_LEFT_PROMPT_ELEMENTS=(virtualenv)

  # Activate virtualenv
  mockVirtualenv "mv/super/venv"
  assertEquals "Unable to set VIRTUAL_ENV_DISABLE_PROMPT=0" "%K{004} %F{000}venv %k%F{004}%f " "$(build_left_prompt)"
}

source shunit2/shunit2

[ymage]

Thank you @dritter for the time you took to clarify. I think I better understand your point and the issue to solve.

My point is I am not sure this is the right way to fix this because from my point of view it's not the theme responsibility to do it.
Why this variable can't simply be set in .zshrc according to the zsh framework (or none) each user choose ?

[dritter]

Why this variable can't simply be set in .zshrc according to the zsh framework (or none) each user choose ?

Well It can. And some frameworks may do that. But others probably not. And users need to know that variable or the impact of not setting that variable. And that is my point. BUT I want to end this argue. Having looked at the activate code, I think that the performance impact is not that big and hope that it does not interfere with P9K. So I am willing to merge this PR as is.

[ymage]

Thank you @dritter
You probably are more wisdom than me

[dritter]

Thx @ymage .