pghilardi / atom-python-virtualenv

Python virtualenv support for Atom
MIT License
16 stars 6 forks source link

Status bar added #19

Closed Gijom closed 7 years ago

Gijom commented 7 years ago

Currently the status bar only displays the current virutalenv TODO: clic on venv status -> list venvs for selection

Gijom commented 7 years ago

This allows to have the name of the current virtualenv in the status bar. The changes are mostly additions and should not modify current package behaviors. One code modification was done in virtualenv-manager.coffee: the env is set to null prior to emitting the 'virtualenv:changed' message in the case of deactivation. Otherwise it is not possible to know if the virtual-env was deactivated when receiving the message. Note that the env will not be set to null if is was already null before, which should not have any side effect...

pghilardi commented 7 years ago

@Gijom ,

Many thanks for this! It worked perfectly :)

I will merge it in the master branch and will release a new version but I just have one question and I want to know what do you think.

Currently it only shows the virtualenv name, for example, venv1. It wouldn't be better to show some label describing that this is a virtualenv? For example: current virtualenv: venv1 when we have a virtualenv activated, otherwise still show the no virtualenv status. What do you think?

Gijom commented 7 years ago

The modification you request is quite straigthforward and easy to implement. However, I wonder if it would not clutter a bit the environement. Other packages typicaly do not write any explanation. For instance there is nothing like 'filename: myfile.py' or 'line 4: column 2' but instead 'myfile.py' and '1:2'. Since with atom you quickly install many packages, having long status would quickly become unmanagable.

Alternatives could be:

I would be more in favor of the second solution as it seems to intergrate in the standard of other packages (e.g. line numbers). However I do not know how to do it and would need some time to implement it.

Also note that I would really like to list virtual envs when the status is cliked. But again my knowledge is too limited for that now.

Any ideas / strategies on all this ?

Gijom commented 7 years ago

Just complementing. Adding a tooltip seems so easy that I think it is the way to go. Example for the line/column status element: https://github.com/atom/status-bar/blob/38be1d7357b9e3d36baf586596fe62ad86c45155/lib/cursor-position-view.coffee

Gijom commented 7 years ago

Sorry for the spaming: there is also the way to implement the click idea in the previous link. As soon as I have your ok, I can start implementing all this. Should I commit them both in a single commit or do you prefer to have two (one for tooltip and one for click) ?

Gijom commented 7 years ago

I added the above mentionned updates in two commits. I kind of like the current state but there is still one "strange" behavior. One should right click to deactivate the venv. I am wondering if it would be better to include "deactivate" in the select list ?

pghilardi commented 7 years ago

@Gijom , I liked the idea of the tooltip :D

I think that it is much better! I will test your code today and I will return to you.

pghilardi commented 7 years ago

@Gijom ,

Maybe it's a good idea to add the "Deactivate" command in the list of virtualenvs (select command), but if we opt to do this, we should show the "Deactivate" with a different style, red color, in bold or something like this. And separate the "Deactivate" from the virtualenv lists with an horizontal separator, for example. What do you think?

pghilardi commented 7 years ago

@Gijom ,

Maybe we can remove the "Deactivate" action and when the user clicks in the current activated virtualenv we will deactivate the current virtualenv. Something like this:

User clicks in "Virtualenv: Select" and the dialog is opened with the envs:

The user selects the env2. When the user clicks again in the "Virtualenv: Select" the dialog will be show like this:

What do you think?

Gijom commented 7 years ago

I really like the idea of re-selecting to deactivate, it make perfect sense. Not sure what the (v) is in you example but probably indicating the already selected venv with a different style is the way to go. I would opt for simple bold. If you implement that then I can remove the right click action on the venv status.

pghilardi commented 7 years ago

Perfect!

The (v) is the active css class that is current being added to the current activated virtualenv :)

So, you can remove the "right-click" behavior to deactivate from this pull request and the message in the tooltip that says to right click to deactivate. After that I will approve this review, merge it and then implement the behavior to deactivate clicking in the current activated virtualenv.

Gijom commented 7 years ago

Done + happy to help.

One side note: the venv now always shows in the status bar even if you are using another grammar than python. Would there be a way to only activate the whole package only for python grammar ?

pghilardi commented 7 years ago

@Gijom ,

I think it's possible. But maybe will not work all the times. Think in an atom user that has a lot of projects opened: python, javascript, java, etc. So the behavior would be to only show the virtualenv when he is working in a python file extension? I think that this may be a bit undesired for some users.

I have approved the pull request and merged in the master branch :) I will release a new version: 0.14.0 soon with the changes.

Many thanks for the contribution! It was a really good job!