PR: Improve selection of custom interpreter #7566

dalthviz · 2018-07-27T06:16:56Z

Pull Request Checklist

Read and followed this repo's Contributing Guidelines
Based your PR on the latest version of the correct branch (master or 3.x)
Followed PEP8 for code style
Ensured your pull request hasn't eliminated unrelated blank lines/spaces,
modified the spyder/defaults directory, or added new icons/assets
Wrote at least one-line docstrings for any new functions
Added at least one unit test covering the changes, if at all possible
Described your changes and the motivation for them below
Noted what issue(s) this pull request resolves, creating one if needed
Included a screenshot, if this PR makes any visible changes to the UI

Description of Changes

Improve custom interpreter selection.

Issue(s) Resolved

ccordoba12 · 2018-07-27T15:41:45Z

Very nice @dalthviz! However, I'd like to have something similar to the working directory toolbar: a combobox to be able to enter by hand or with a file dialog a Python interpreter. The one in the working directory is called PathComboBox, so you'd need to create another class called FileComboBox for files.

Besides, you'd need to create another method in SpyderConfigPage that creates and deals with the option selected by users in a FileComboBox.

dalthviz · 2018-07-28T03:48:41Z

A new preview:

ccordoba12 · 2018-07-28T04:03:41Z

Much, much better! Thanks @dalthviz!

One last thing: Could you hide and/or remove the validation widget next to the file name (i.e. the one with the blue check mark)? Then I'd say this is ready.

…t in the file combobox.

dalthviz · 2018-07-28T04:37:01Z

New preview:

ccordoba12 · 2018-07-28T15:17:12Z

spyder/plugins/maininterpreter.py

@@ -222,5 +230,20 @@ def set_umr_namelist(self):
                fixed_namelist = []
            self.set_option('umr/namelist', fixed_namelist)

+    def set_custom_interpreters_list(self, executable):
+        """Update the list of interpreters used and the current one."""
+        custom_list = self.get_option('custom_list')


We need to add a method that takes this list and verifies the entries that still exist (with a simple osp.isfile). The ones that don't need to be removed and a new list of interpreters needs to be saved with set_option.

Sorry, this probably doesn't need to be done here, but above, where you create the filecombobox.

ccordoba12 · 2018-07-28T15:18:10Z

spyder/plugins/maininterpreter.py

+    def set_custom_interpreters_list(self, executable):
+        """Update the list of interpreters used and the current one."""
+        custom_list = self.get_option('custom_list')
+        if (executable, executable) not in custom_list:


This looks confusing, i.e. naming two variables with the same name (execuatble, executable). Please change that.

ccordoba12 · 2018-07-28T15:20:34Z

spyder/plugins/maininterpreter.py

-        pyexec_layout.addWidget(pyexec_file)
+        self.cus_exec_combo = self.create_file_combobox(
+                                            _('Recent custom interpreters'),
+                                            self.get_option('custom_list'),


So here, before setting the custom list of interpreters, we need to validate that they still exist and remove the ones that don't.

Maybe it would be better to do the validation in the __init__ method?

Sure, that's ok too.

ccordoba12 · 2018-07-30T15:11:37Z

spyder/plugins/maininterpreter.py

@@ -53,10 +54,11 @@ def __init__(self, parent, main):
            # the Python executable has already been set with pythonw.exe:
            self.set_option('executable',
                            executable.replace("pythonw.exe", "python.exe"))
+        self.set_custom_interpreters_list(executable, executable)


I think this should be

if not self.get_option('default'): self.set_custom_interpreters_list(executable, executable)

because we don't want the default interpreter to be part of the list of custom ones.

After trying this change I notice that we need at least one entry in the combobox. If not this triggers and error when loading the config (since there is no option to give to the current index in the combobox). The line where this happens

Mmmh, I don't like this because the default interpreter is going to be part of the list of custom ones too. Besides, users would find strange that there's one entry in the custom interpreters list if they haven't selected none.

Couldn't you find a way to make configdialog work with no interpreter?

I could add a validation in configdialog on the line that I linked above to not raise and error, and also change a statement in python_executable_changed since currently if an invalid interpreter is found it will put the default one as an option, and fallback to select the default interpreter option when the settings are applied with an empty custom interpreter probably.

Should I do this changes?

ccordoba12 · 2018-07-30T15:14:22Z

spyder/plugins/maininterpreter.py

@@ -152,6 +161,7 @@ def python_executable_changed(self, pyexec):
                      "make sure to select a valid one."), QMessageBox.Ok)
            self.pyexec_edit.setText(def_pyexec)
            return
+        return True


If you make this method to return True at the end, then you need to change the behavior of the previous return's to be return False, and not simply return (as it is right now). That's to be consistent.

ccordoba12 · 2018-07-30T15:16:23Z

spyder/plugins/maininterpreter.py

+        if (display_value, value) not in custom_list:
+            custom_list.append((display_value, value))
+            self.set_option('custom_list', custom_list)
+        self.set_option('executable', value)


I think this line shouldn't be here but applied after this method is called.

This is true specially due to the comment I left in the __init__ method.

…'executable' option.

ccordoba12 · 2018-08-10T03:43:45Z

Ok, let's merge this one. Thanks @dalthviz!

Fixes #7529

CAM-Gerlach · 2018-08-12T18:07:06Z

@ccordoba12 @dalthviz Just tested this on the latest 3.x on Windows 8.1, and there appears to be a UI bug with this: when selecting a new interpreter from the file browser (i.e. rather than choosing it from the dropdown list), and then hitting "Apply" and "OK", just "OK", "Apply" then closing the dialog, etc., the new setting does work (i.e. new consoles are opened in the environment the user selected). However, if I re-open preferences, the interpreter path displayed in the combobox is that of the interpreter I previously had selected before the aforementioned change, rather than the actual one. However, so long as one does not actually interact with this field, making other changes in the preferences and then saving makes the interpreter for new consoles stay the one the user selected, rather than the one displayed, and selecting interperters from the list does make them display correctly in the box once preferences is re-opened.

Also, conversely, when selecting one or multiple new interpreters from the file browser dialog that aren't currently in the list, their path(s) is (are) not added to the list, even if the Apply button is clicked to set them, until the preferences dialog is closed and re-opened.

ccordoba12 · 2018-08-12T18:08:40Z

@CAM-Gerlach, please open a new issue about it.

CAM-Gerlach · 2018-08-12T18:08:57Z

@ccordoba12 In work.

Improve selection of custom interpreter.

569d9f3

dalthviz added type:Enhancement component:Preferences labels Jul 27, 2018

dalthviz added this to the v3.3.1 milestone Jul 27, 2018

dalthviz self-assigned this Jul 27, 2018

dalthviz requested a review from ccordoba12 July 27, 2018 06:16

Improve combobox. Merge file chooser and combobox.

c0b0cd4

Improve interpreter path handling. Add option to use default QLineEdi…

7fc8bd4

…t in the file combobox.

Prevent saving paths of invalid interpreters.

cadb58d

ccordoba12 reviewed Jul 28, 2018

View reviewed changes

Add validation for custom interpreters list.

211e641

ccordoba12 reviewed Jul 30, 2018

View reviewed changes

dalthviz added 2 commits July 30, 2018 13:54

Improvements for consistent type return in methods and handle of set …

d548cdf

…'executable' option.

Change management of default interpreter in the custom interpreters.

e5b02bd

CAM-Gerlach changed the title ~~PR: Improve selection of custom interpreter.~~ PR: Improve selection of custom interpreter Aug 8, 2018

Improve validation and default value for custom_executable config.

4b85686

ccordoba12 merged commit 77cf810 into spyder-ide:3.x Aug 10, 2018

ccordoba12 added a commit that referenced this pull request Aug 10, 2018

Merge from 3.x: PR #7566

716fe0a

Fixes #7529

CAM-Gerlach mentioned this pull request Aug 12, 2018

New Python interpreter selection combobox displays previous, not actual executable path if selected from file browser #7674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Improve selection of custom interpreter #7566

PR: Improve selection of custom interpreter #7566

dalthviz commented Jul 27, 2018 •

edited

Loading

ccordoba12 commented Jul 27, 2018

dalthviz commented Jul 28, 2018 •

edited

Loading

ccordoba12 commented Jul 28, 2018

dalthviz commented Jul 28, 2018

ccordoba12 Jul 28, 2018

ccordoba12 Jul 28, 2018

ccordoba12 Jul 28, 2018

ccordoba12 Jul 28, 2018

dalthviz Jul 28, 2018

ccordoba12 Jul 28, 2018

ccordoba12 Jul 30, 2018 •

edited

Loading

dalthviz Jul 30, 2018

ccordoba12 Jul 30, 2018

dalthviz Jul 31, 2018 •

edited

Loading

ccordoba12 Jul 30, 2018 •

edited

Loading

ccordoba12 Jul 30, 2018

ccordoba12 Jul 30, 2018

ccordoba12 commented Aug 10, 2018

CAM-Gerlach commented Aug 12, 2018 •

edited

Loading

ccordoba12 commented Aug 12, 2018

CAM-Gerlach commented Aug 12, 2018

PR: Improve selection of custom interpreter #7566

PR: Improve selection of custom interpreter #7566

Conversation

dalthviz commented Jul 27, 2018 • edited Loading

Pull Request Checklist

Description of Changes

Issue(s) Resolved

ccordoba12 commented Jul 27, 2018

dalthviz commented Jul 28, 2018 • edited Loading

ccordoba12 commented Jul 28, 2018

dalthviz commented Jul 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalthviz Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

ccordoba12 Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Aug 10, 2018

CAM-Gerlach commented Aug 12, 2018 • edited Loading

ccordoba12 commented Aug 12, 2018

CAM-Gerlach commented Aug 12, 2018

dalthviz commented Jul 27, 2018 •

edited

Loading

dalthviz commented Jul 28, 2018 •

edited

Loading

ccordoba12 Jul 30, 2018 •

edited

Loading

dalthviz Jul 31, 2018 •

edited

Loading

ccordoba12 Jul 30, 2018 •

edited

Loading

CAM-Gerlach commented Aug 12, 2018 •

edited

Loading