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

PR: Improve implementation and UI of console envs menu (IPython console/Main interpreter) #22200

Merged
merged 14 commits into from
Jul 18, 2024

Conversation

ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Jun 23, 2024

Description of Changes

  • The IPython console and Main interpreter plugins have repeated functionality to keep track of conda/pyenv envs in the system. This PR consolidates that in Main interpreter, which now emits a signal that the IPython console connects to to display info related to those envs.
  • To do that it was necessary to move almost all functionality of the Main interpreter status bar widget to its container.
  • Show custom envs (i.e. those manually added by users in Preferences) in the console envs menu. That's important for people that don't use conda/pyenv for env management.
  • Break console envs menu into sections or submenus with the following actions:
    • Default, which shows the default and internal (i.e. same as Spyder) envs. In case they are the same, the internal env is not shown.
    • Conda envs
    • Pyenv envs.
    • Custom envs.
  • Except for the first section, the others are added if there are two or more envs per category. Also, submenus are used if the user has more than 20 envs and more than one category of envs.

Visual changes

IPython console envs menu

Before After
imagen imagen

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @ccordoba12

@ccordoba12
Copy link
Member Author

@dalthviz, this is ready for review. Since I have to move code around in several places, I suggest that you take a look at these changes one commit at a time.

@dalthviz
Copy link
Member

Gave this a check and seems like the menu is working as expected 👍 Also, just in case, an idea that came to my mind while checking this is to use submenus instead of sections (so Consoles > New console in environment > pyenv/conda/custom). In my current setup I have a lot of conda envs but just a couple pyenv/custom envs and I have to go all the way down to get them:

menu_envs

However, the status bar is not showing anything at all or showing a different env name for completions. So, having over preferences the default interpreter selected, I see on Spyder start an empty completions status bar:

image

If I start changing the interpreter preference between a custom interpreter and the default one, I end up with completions refering with a different interpreter than the default one even when I have set the preferences to just use the default one again:

image

The tooltip though is showing the correct path to the selected interpreter:

image

Also, I'm not totally sure to follow the reason to move the main interpreter container to be inside the widgets module, maybe the container is now also a widget? From what I see seems like it still is a PluginMainContainer 🤔

That way we'll be able to emit the list of current environments to other
plugins through a signal.
@ccordoba12 ccordoba12 force-pushed the consolidate-update-envs branch from f4eafd2 to 0e789f4 Compare July 9, 2024 16:41
@pep8speaks
Copy link

pep8speaks commented Jul 9, 2024

Hello @ccordoba12! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-07-18 01:08:15 UTC

@ccordoba12
Copy link
Member Author

ccordoba12 commented Jul 9, 2024

Also, just in case, an idea that came to my mind while checking this is to use submenus instead of sections (so Consoles > New console in environment > pyenv/conda/custom). In my current setup I have a lot of conda envs but just a couple pyenv/custom envs and I have to go all the way down to get them

That's a great idea @dalthviz! I implemented it in my last commit like this: if the user has more than 20 envs and there are at least two categories (conda/pyenv/custom) with one env or more, then we show submenus. If not, we use a single menu with sections.

  • However, the status bar is not showing anything at all or showing a different env name for completions. So, having over preferences the default interpreter selected, I see on Spyder start an empty completions status bar
  • If I start changing the interpreter preference between a custom interpreter and the default one, I end up with completions refering with a different interpreter than the default one even when I have set the preferences to just use the default one again
  • The tooltip though is showing the correct path to the selected interpreter

My idea for a follow-up PR (to be created after after this one) is to remove the current Completions status bar widget, along with the Main Interpreter one, and leave a single widget that will show the interpreter of the current console (so, that widget will be similar to the Matplotlib status one).

I decided to do that for three reasons:

  • Now that users will be able to easily open consoles in any environment, it makes little sense to show the default interpreter in the status bar.
  • The information displayed in the completions and default interpreters was almost always the same. So, it doesn't make sense to have two of them.
  • This problem is similar to the one we had in Spyder 3 with the directory shown in the Working Directory toolbar. At that time we were showing a global directory, and that was really confusing to users. After we decided to show the cwd of the current console, that confusion disappeared.

Also, I'm not totally sure to follow the reason to move the main interpreter container to be inside the widgets module, maybe the container is now also a widget? From what I see seems like it still is a PluginMainContainer 🤔

I think you're right. Technically, PluginMainContainer is a widget because it inherits from QWidget. But it's not a real widget, i.e. as a dialog or button. So, I reverted that change.

@ccordoba12 ccordoba12 force-pushed the consolidate-update-envs branch 2 times, most recently from 1335988 to 01fe38f Compare July 9, 2024 16:54
ccordoba12 added 10 commits July 9, 2024 11:56
Also, make some tiny UI improvements to its confpage.
Also, use that signal in the IPython console to simplify how the console
environments menu is updated.
- Capitalize names so they look better in menus.
- Add env dir to custom env names to be able to track multiple of them.
- Add for translation env names that can be translated.
- That allows to show them in the console envs menu.
- For now this only works after Spyder is restarted. But in-place
tracking (i.e. as soon as the env is added in Preferences) will be added
next.
Add in-place tracking of custom envs (i.e. as soon as they are added in
Preferences).
- Add sections for default, conda, pyenv and custom envs.
- Also, in Main Interpreter confpage change the env's name in which
Spyder is installed to Internal. That makes it easier to distinguish
between default and internal envs.
- That method is only used in its main widget.
- Also, move the update_envs method to a better place.
The test was failing because we now require that the new interpreter set
in Preferences be really different from the current one to report that
it changed.
@ccordoba12 ccordoba12 force-pushed the consolidate-update-envs branch from 01fe38f to 7e086be Compare July 9, 2024 16:56
@ccordoba12 ccordoba12 requested a review from dalthviz July 9, 2024 16:58
@dalthviz
Copy link
Member

I implemented it in my last commit like this: if the user has more than 20 envs and there are at least two categories (conda/pyenv/custom) with one env or more, then we submenus. If not, we use a single menu with sections.

Gave this another check and seems like things for the menus are working with the extra conditional rendering idea (subsections vs submenus) 👍 The only thing that I'm not sure if needs something to be done is the handling of empty submenus. So for example if I have 20+ conda envs and one custom env I'm seeing something like:

console_menu_envs

Maybe if no envs are available in a particular submenu, that submenu should be shown in a disabled state?

My idea for a follow-up PR (to be created after after this one) is to remove the current Completions status bar widget, along with the Main Interpreter one, and leave a single widget that will show the interpreter of the current console (so, that widget will be similar to the Matplotlib status one).

I have been thinking about that and the final result makes sense. Still, is kind of weird to leave things in an incoherent/broken state (even if is just while waiting for an immediate follow up PR). So, although is just a mere review process principles technicallity, I would say I can't approve this PR (like literal leave it approved on the GitHub interface). Anyhow, I won't stop you from merging this so feel free to do so when you think is ready 👍

That way users will be able to easily go the category for which they
want to start an env.
- Due to the previous changes, that widget it's not working as expected
anymore.
- In addition, it was displaying the same information as the one
displayed by the Main Interpreter widget most of the time. So, it makes
little sense to keep showing it to users.
@ccordoba12 ccordoba12 force-pushed the consolidate-update-envs branch from 7e086be to 42f222d Compare July 14, 2024 16:22
@ccordoba12
Copy link
Member Author

ccordoba12 commented Jul 14, 2024

Maybe if no envs are available in a particular submenu, that submenu should be shown in a disabled state?

Good idea. I amended my second to last commit to implement that, so please check again.

I have been thinking about that and the final result makes sense. Still, is kind of weird to leave things in an incoherent/broken state (even if is just while waiting for an immediate follow up PR).

You're right. That's why I decided to remove the Completions status bar widget in my last commit (it'll be removed anyway but the changes in this PR don't allow it to keep working as expected).

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @ccordoba12 ! Gave this a new check and seems like things are working as expected 👍 However, I'm not seeing the menu in a disabled state, in fact, I'm literally not seeing the menu (the pyenv menu in this case). That works for me although seemed worthy to mention just in case that wasn't what you were aiming for:

image

Anyhow, I will leave this approved so feel free to merge as soon you think this is ready 👍

@ccordoba12
Copy link
Member Author

! Gave this a new check and seems like things are working as expected 👍

Great!

However, I'm not seeing the menu in a disabled state, in fact, I'm literally not seeing the menu (the pyenv menu in this case).

That's by design. We only show submenus that have at least one env listed in them. Since you have no Pyenv envs, that's why the menu doesn't show up for you.


There's one small issue in your gifs though: they don't show a Default entry above the Internal / Conda: spyder-dev (Python 3.9.19) one. And that entry should be available because it's the interpreter set in Preferences,

I tried to reproduce that on my Windows VM but I was unable to. So, could you post a screenshot of your Preferences > Python interpreter page? Also, please post the value of get_conf("executable", section="main_interpreter") to check what's saved there.

@dalthviz
Copy link
Member

There's one small issue in your gifs though: they don't show a Default entry above the Internal / Conda: spyder-dev (Python 3.9.19) one. And that entry should be available because it's the interpreter set in Preferences,

I tried to reproduce that on my Windows VM but I was unable to. So, could you post a screenshot of your Preferences > Python interpreter page? Also, please post the value of get_conf("executable", section="main_interpreter") to check what's saved there.

image

image

@ccordoba12
Copy link
Member Author

Thanks for the extra info @dalthviz! I was able to reproduce and fix the problem I mentioned above with it.

- Also, use the regular (i.e. not lower case) env path when saving the
env info.
- That's necessary to correctly create the Default and Internal entries
in the console envs menu because they require path comparisons.
@ccordoba12 ccordoba12 force-pushed the consolidate-update-envs branch from 4760fc6 to 27f447a Compare July 18, 2024 01:07
@ccordoba12 ccordoba12 merged commit 5c54fc6 into spyder-ide:master Jul 18, 2024
14 checks passed
@ccordoba12 ccordoba12 deleted the consolidate-update-envs branch July 18, 2024 02:15
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