Improve interpreter ordering and star placement. #16972
Conversation
karrtikr
left a comment
karrtikr
left a comment
There was a problem hiding this comment.
"Star" should be next to the autoselected interpreter (the interpreter we suggest) in the list. I don't think this PR does that.
|
@karrtikr what do you mean. The |
@karthiknadig Ah I see, I missed that first interpreter will always be the autoselected one. But I don't think we should rely on this logic, instead we should get the autoselected interpreter by searching in the list. That way we're not duplicating the "first interpreter = autoselected interpreter" logic in two places. Also, I don't see the point in the "star" icon if it's always going to be on the first one. Earlier, that was not the case, which is when the work item was created. |
|
I don't see a point to star as well. I agree with you on that. So let use see what @luabud thinks. |
|
@karthiknadig about this example:
if the terminal isn't activated, "python" would not be the .venv one, right? In that case wouldn't it show the same as in the "defaultInterpreterPath set to some python executable" example, but with the path being the same as what "python" is set to on PATH? The main reason why I think adding the star is helpful is for cases where users who are getting started with/learning Python may have a couple of environments on their machine but they don't know which one to use. It was kind of a partner's ask/suggestion, the idea of adding some sort of "suggested" tag to environments to help users with that. |
|
What "python" means in a given scenario is not something we can tell without running it. This is especially true in the case of |
src/client/common/utils/localize.ts
Outdated
| ); | ||
| return `${Octicons.Star} ${labelText()}`; | ||
| }, | ||
| label: localize('InterpreterQuickPickList.defaultInterpreterPath.label', 'Use default Python interpreter path'), |
There was a problem hiding this comment.
A reminder that this text needs to be changed.
karthiknadig
force-pushed
the
interpreter-order
branch
from
3f33a63 to
5588f7e
Compare
|
|
||
| quickPick.items = newSuggestions; | ||
| quickPick.activeItems = [interpreterSuggestions[0]]; | ||
| quickPick.activeItems = |
There was a problem hiding this comment.
Can you show how it looks? Also, I feel Enter interpreter path... should be pinned if we're scrolling to the active item.
There was a problem hiding this comment.
is there a way to pin it? From the chat in slack, it looks like there is no option to make it "sticky" right now.
There was a problem hiding this comment.
Yeah sorry, doesn't look like it. But it's weird then, I don't like us scrolling to the selected item in that case.
There was a problem hiding this comment.
@luabud What is your opinion?
This is what it looks like when you select items at the top:
This is what it looks like when you select something in the bottom:
essentially we don't see the 'enter path' and 'default path'.
I can revert back to the previous state. if we don't like this.
There was a problem hiding this comment.
After an offline discussion with Luciana, we are going to keep this as we have in the latest commit. That is, the blue highlight is on the item that is selected and it scrolls to that item on opening the quick pick.
3 participants
Closes #16971
Closes https://github.com/microsoft/vscode-python-internalbacklog/issues/277
No
defaultInterpreterPathset:defaultInterpreterPathset topython:defaultInterpreterPathset to some python executable in the user settings:defaultInterpreterPathset to some python executable: