Skip to content

Double clicking variable selects variable AND $ prefix - this is not desirable #3378

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

Closed
rkeithhill opened this issue May 28, 2021 · 21 comments · Fixed by #3413
Closed

Double clicking variable selects variable AND $ prefix - this is not desirable #3378

rkeithhill opened this issue May 28, 2021 · 21 comments · Fixed by #3413
Assignees
Labels
Area-UI Issue-Discussion Let's talk about it.

Comments

@rkeithhill
Copy link
Contributor

System Details

System Details Output

I had to run this from pwsh. PSIC just hangs when I try to run this command. But PSES is:

=====> PowerShell Preview Integrated Console v2021.5.0 <=====

Issue Description

Double-clicking a variable has changed to now selecting the $ prefix. This is a problem when pasting over the variable when used in splatting e.g. @iwrArgs becomes @$iwrArgs. Ditto with string interpolation "${smtpServer}" becomes "${$smtpServer}" after pasting. This is highly undesirable IMO.

Expected Behaviour

Double-click select of a variable should not include the $ in the selection.

Actual Behaviour

Double-click select of a variable does include the $ in the selection.

@ghost ghost added the Needs: Triage Maintainer attention needed! label May 28, 2021
@andyleejordan
Copy link
Member

Is this sufficient for you? Since it's only a supplied default, the behavior is easily overridden.

"[powershell]": {
    "editor.wordSeparators": "`~!@#$%^&*()-=+[{]}\\|;:'\",.<>/?"
}

This was a small change I made based on feedback gathered during a VS Code PowerShell extension meetup. See #3359 It seemed like most users would prefer the ISE-like behavior were - and $ are not word separators.

What if we put $ back but kept - removed? Then we'd still get PowerShell symbols like Foo-Bar treated as a word, but with $Foo-Bar being two words.

@andyleejordan andyleejordan added Area-UI Issue-Discussion Let's talk about it. and removed Needs: Triage Maintainer attention needed! labels Jun 1, 2021
@rkeithhill
Copy link
Contributor Author

I've got no problem leaving - out as that is not supported in a variable name unless you're willing to use {} e.g. ${foo-bar} = 42. IF I weren't going to use camel/Pascal-casing, I'd use _ instead e.g. $foo_bar = 42. But not having $ in there is messing me up on a daily basis. I wonder if the wordSeparators could be different depending on whether you have a modifier key pressed?

@ghost ghost added the Needs: Maintainer Attention Maintainer attention needed! label Jun 1, 2021
@andyleejordan
Copy link
Member

Just playing around with this, the included change makes the VS Code experience consistent with PowerShell itself. That is, if you put the cursor on $Foo-Bar the PowerShell extension (correctly) highlights the whole thing, and double-clicking it with this change then selects the whole thing. Without the included change, double clicking Bar is really confusing, as it gets selected, and slightly more highlighted, will the reset of $Foo-Bar remains highlighted. It's very confusing how it was. I'm going to keep the new default in the extension for the stable release today so we can collect more feedback, but I think in general most people will be happy with this, and the embedded changelog includes exactly how to revert the change.

@poshcodebear
Copy link

poshcodebear commented Jun 2, 2021

I'm definitely not one of those happy with this change.

It messes with Ctrl+Backspace, so erasing and retyping a variable name means also having to retype the $, and erasing a parameter also erases the trailing -.

It messes with quick refactoring, so Ctrl+F2 to change the name of a variable means, again, retyping the $, even though it makes no sense that I'd want to change that part since all variables must be decorated with a $ (not to mention, it means splats won't be updated in a quick Ctrl+F2 refactor, which can actually introduce bugs/break scripts)

Not to mention, it makes refactoring multiple functions in the same module to change the noun or prefix a bigger pain, because now I either have to do it for every verb variant or write a regex find/replace for it.

In any event, these are behaviors that I've come to expect from the editor in the several years I've been using Code to write my PowerShell, and this change has already caused a significant amount of aggravation in just the very short time since it's been implemented, to the point I had to dig in to the settings to figure out why it wasn't working as expected, and even went to the hassle of searching the commit history for this repo just to verify I wasn't going mad and this had actually changed.

This is a breaking change to existing user workflows. I don't see the old behavior as confusing in the slightest, and personally feel that existing users who have already come to expect certain behavior should take precedence over new users who can learn the existing behavior more easily than we can re-learn it. Four plus years of muscle memory don't fade overnight.

Anyway, I know there's a workaround and will be using it, but I really think this was a poorly thought out decision.

@andyleejordan
Copy link
Member

andyleejordan commented Jun 2, 2021

Per the changelog (which should show up automatically when the extension updates) you can revert with:

"[powershell]": {
    "editor.wordSeparators": "`~!@#$%^&*()-=+[{]}\\|;:'\",.<>/?"
}

@rkeithhill
Copy link
Contributor Author

and personally feel that existing users who have already come to expect certain behavior should take precedence over new users who can learn the existing behavior more easily than we can re-learn it. Four plus years of muscle memory don't fade overnight.

💯 Amen brother!

@dragonwolf83
Copy link

I don't think its that simple. I've hated it for years that you can't double click to select the whole variable with the $ when trying to just copy and paste it. That triggers more work to add it back in or go select it again without double-clicking so that it captures it. Either way, the user gets a bad experience.

If triple-clicking would have selected the whole word instead of the line, it would be a good compromise. I don't know if that is possible in VS Code.

So what scenario should be optimized here? Refactoring, Copying, transforming to splatting or string interpolation? Is there a way to paste with special options like Office does to auto-refactor it? That would be a cool feature.

@poshcodebear
Copy link

The way I see it, with copying, you have three options:

  • Double-click and move your mouse over until it selects everything you want (it will automatically select whole words if you start with a double-click, so it's pretty easy to just do that, and since doing that makes selecting multiple words faster and easier in general, it's quick to get used to doing this)
  • Use the autocomplete/Intellisense system and type the first few characters if you're just entering it in one or two places
  • Modify the word separation characters for your own system if the default really bugs you that much; it's been user configurable in VSCode since, I believe, at least 2016

Since PowerShell, as far as I can see (at least of the languages installed on my system), is the only language provider that now modifies the default list of word separation characters, that means even other languages that use the $ variable decorator don't mess with word separator list.

And I will say, for me personally, the minor issues caused by the default word separator list from VSCode are easily overcome with small adjustments to expectations to match, while the issues caused by the modified list means the built in refactoring tools become noticeably less useful, forcing manually using find/replace in a lot of scenarios that a simple Ctrl+F2 should have been able to handle.

And, again, this is a breaking change to existing user workflows to, as far as I can tell, reduce the learning curve, though it seems to me that it doesn't do much on that end, especially compared to the price of making experienced users' tools less functional.

As for the changelog showing up automatically...I haven't seen one in a long while myself, didn't even realize it had updated until I tracked this issue down to it, and just to verify, I launched it on a system I've not coded on for the last few weeks and all I got was a pop-up notification that went away after ten seconds, something that I'm likely not to notice or even bother looking at when I'm in the middle of getting something done. You can say that's my own fault, but I don't think it's an unreasonable expectation that existing workflows won't break because of a change in default behavior as opposed to adding support for changing our own workflows.

Since this update also marks the third time I've had to chase down why variable properties are no longer colored differently than the variables themselves because the name they've been given for TextMate rules has changed, again, I'm genuinely confused by the direction the team running this project have chosen. I don't know about you, but having my tools break in updates costing me an hour or two hunting down what changed and how to fix it isn't exactly a stable, production ready experience. This attitude of "move fast and break things" doesn't belong in professional tooling.

Anyway, I've said my piece, I've fixed my tools and can go about getting my work done and can put this behind me (at least until the team running this project decides to break things again), so I'll be off. Please consider changes like this more carefully in the future. And please consider reversing this one as I'm obviously not the only existing user frustrated by this change, especially since anyone who likes this new behavior could have easily set up their own install to do it had they wanted to at any point in the last five years.

@rkeithhill
Copy link
Contributor Author

The gift that keeps on giving:
image

🤦‍♂️

@ThubLives
Copy link

ThubLives commented Jun 4, 2021

I'm kind of puzzled about the trend to keep changing defaults to more closely mimic the ISE when there is a palette command "PowerShell: Enable ISE Mode" that was added specifically to make it easy to change a number of settings to values that more closely mimic the ISE. "PowerShell: Disable ISE Mode" returns those values to the extension defaults, but if those defaults are already ISE-like and foreign to non-PowerShell VSCode users, what is the point?

The main reason I started using VSCode was because I was sick of having different editors for different languages that behaved in unique and cognitively conflicting ways. With VSCode I didn't have to mentally switch back and forth between the text selection behaviours of two DE's and it still did sensible language-specific things like code highlighting and IntelliSense. Common debug shortcut keys and predictable keyboard-based navigation was a huge reason for that.

Obviously, having the option to change this particular setting back is welcome in this case, as other changes ISE-ward have not provided configuration options to restore the old behaviour(#2523). In general I question the wisdom of this overall approach. I completely understand that people either like the way ISE works but want the more advanced features of VSCode, or simply want an easier transition to a more modern tool. I think a separate extension for a PowerShell ISE Mode sounds very sensible and something similar has been done for plenty of other languages. But again, the "Enable ISE Mode" command already exists, supposedly for this very purpose.

More relevant to the discussion at hand -- take this code, a scenario I run into quite often:

$BaseName = 'CSL'
$Seperator = '-'
$Date = Get-Date -Format 'yyyyMMdd'
$Extension = '.dx1'

$NewReportFileName = "$BaseName$Seperator$Date$Extension"

The new default value makes selecting a portion of that string unwieldy. As a bonus, if I want to correct the spelling of that $Seperator variable name this won't work either, as mentioned in an earlier comment. Previously I could just click somewhere in the middle of the name on the second line (notice that it visually highlights the variable as expected), then press + to highlight all instances of that variable name. With the current default value, the string on the last line is not updated with the rest.

Furthermore, the exact same problems occur if the last line uses a literal '-' character:

$NewReportFileName = "$BaseName-$Date$Extension"

tl;dr:
I think the extension defaults should reflect sensible, familiar values for general VSCode users. If you want it to act like the ISE, that's literally what ISE Mode is for.

@dragonwolf83
Copy link

As far as I can tell, this has nothing to do with ISE. ISE does not include $ when you double click or use keyboard shortcut. Including this in ISE-Mode wouldn't make sense.

So why this change? Because without further documentation, to alot of users this could appear to be a bug or quirk. Check the following issues about missing $ when selecting: #240, #2546, #1358. This makes a lot of sense because copying variables is likely one of the first things anyone does in a tool. Most of the scenarios listed in this issue are a bit more advanced use cases. So if you never used those features, its quite easy to understand how someone would be confused as to why a variable is not being fully highlighted when trying to select it. After all, the $ is part of the variable name, it is one word semantically.

I think including $ is semantically correct but has pitfalls as everyone has mentioned. If this change is reverted, this should be documented in the FAQ explaining the issues found here with instructions on how to add it back if desired.

Ultimately, this issue is a set of trade-offs. Maybe one day more advanced solutions can be found so we can satisfy all use cases like I mentioned before. Until then, I would suggest using the established behavior from ISE for both $ and -.

@ThubLives
Copy link

@andschwa sited ISE-like behaviour as the goal for this change, so that was the basis of my comment. If this seems like a bug for new users, then it would likely do so for other languages as well and would perhaps make more sense to address it in vscode itself, rather than making this an exception for only PowerShell.

Again, consistency is a valuable feature of vscode that helps those who work in multiple languages to be more efficient and that means being judicious in choosing seemingly trivial things such as default settings.

That said, at least the default can be changed back.

@dragonwolf83
Copy link

If this seems like a bug for new users, then it would likely do so for other languages as well and would perhaps make more sense to address it in vscode itself, rather than making this an exception for only PowerShell.

What is considered a word is different per language. VS Code did address the issue by allowing editor.wordSeparators to be configured at the language level which is what was done here. Many languages don't have an issue because $ is not mandated for use in variables and thus not part of the word (hence word separator).

Again, consistency is a valuable feature of vscode that helps those who work in multiple language...

Consistency is a relative term. Consistent to what? I would say it is very inconsistent that double selecting a variable in PowerShell does not include $ considering the purpose of double clicking or word select is to select the whole word. The $ is part of the word because without it can't be used. That is why it appears to be broken or incomplete behavior.

Now, we learn through examples in this issue that not including $ has benefits that may be better than including it. Valid feedback. But one that should be documented so even long-time PowerShell users like me could reference why.

As for consistency of default values between builds, that is never guaranteed by VSCode itself. New defaults are chosen all the time in VS Code releases that change behaviors. They just have the benefit of alot of people testing insider builds to catch feedback early on and even then, they may get negative feedback on release. That is expected for this editor.

I think we need to remember that alot of people have different ways of using software and it often conflicts with each other. The comments on this issue do not reflect the general community because its just 3 users reporting an issue out of the 3 issues that were opened to include $. That is not enough feedback for anyone to make a good decision. about what is a sensible default Without a poll and more people testing it out, we just don't know. This again goes back to good documentation so that people can make up their own mind, maybe add some telemetry and see what the default should be.

I do hope some exploration is done to make it possible to solve the problem for all users and workflows but that I don't think will happen anytime soon given the complexity.

@ThubLives
Copy link

The consistency I'm talking about is basically this: High level languages don't differ much in how they are constructed semantically. Even in what they do and do not consider to be a "word". Therefor there should be few exceptions, if any, with how one is to navigate text of any high level coding language. It really is just text.

PowerShell in particular isn't unique in terms of the semantic constructions that make up the language. The use of a special character to identify a variable is pretty common in fact. I've looked at a few languages with this general approach to variables that I could think of off the top of my head and they all behave the same way in vscode; the special character is not treated as part of the variable name. In fact, semantically I would argue that the $ is not part of the variable name anyway, and certainly not part of a word, but that's really beside the point.

If a higher concept such as "this text represents a variable and should be grouped like a word" is thought to be valuable, I think the best approach would be to design it into vscode and implemented as an optional feature for any language, not just in the PowerShell extension. This could simplify things like code auto-formatting for example. There is already code (in I think the code highlighting) that does a great job of auto-highlighting complex variable tokens such as this, but I don't know how it relates, if at all: ${A ridiculous variable name, I think you'll agree.}. If changing how variables are selected is desirable in PowerShell, surely it's desirable in other languages as well. There must be better, more agnostic ways to address this if it is a problem worth solving.

I have particular opinions about UX design with respect to code editors, but ultimately if features or specific behaviour are worth implementing, they should be implemented at the appropriate place to provide better code efficiency and UX consistency. That's all I'm really advocating here. If a lot more rambling that I was hoping.

@rkeithhill
Copy link
Contributor Author

@dragonwolf83 Changing three years of EXISTING behavior was probably not a good idea IMO. That is one issue I have with certain OSS projects. Folks seem a little cavalier with making changes to well established behavior. At the very least, the change in behavior should have been made "opt-in".

@dragonwolf83
Copy link

If a higher concept such as "this text represents a variable and should be grouped like a word" is thought to be valuable, I think the best approach would be to design it into vscode and implemented as an optional feature for any language

That is exactly how this works. Nothing is hard-coded here. The VS Code feature is a global setting called editor.wordSeparators. It defines a list of characters that should be treated as the boundary to separate a word from another so that you can do word navigation and selection. It defaults to the most common characters, but that can break things for other languages or even in terminal usage. Hence, why it is a setting.

The setting can be customized by user, workspace, or language. All of what you are asking for or suggesting here is already done. All the PowerShell Extension team did is default to what appeared to make sense based on general feedback they got. You can undo it by changing the setting yourself.

. I've looked at a few languages with this general approach to variables that I could think of off the top of my head and they all behave the same way in vscode;

It behaves the same across languages within VS Code because editor.wordSeparators is a global setting. I don't know how many extensions change this behavior for their language, but there is probably more than one.

To understand proper behavior, you would have to compare how it works with other IDEs and ideally native IDEs for a specific language. I just compared 3 different ones: Padre for Perl, PowerShell Studio which I believe predates ISE, and PyCharm for Python. Both Padre and PowerShell Studio include $ in selection and PowerShell Studio also includes -. Python does not. Makes sense because Perl and PowerShell both require $ in their variable names.

Changing three years of EXISTING behavior was probably not a good idea IMO. That is one issue I have with certain OSS projects. Folks seem a little cavalier with making changes to well established behavior.

@rkeithhill I would agree we need far more data and polling in the PS community about sensible defaults and behaviors. The fact is, we don't have hardly any representation in this issue to really know what the user base wants here. We should appreciate how hard it is to accommodate conflicting feedback from users that wanted $ to be included vs users like yourself that do not.

We should also appreciate that we are not always going to know what is the best solution until we have enough users trying it out. I certainly never thought of the issues you cited, but you raised valid use cases. What probably needs to happen next time is make sure a clear issue is open for feedback so that people can discuss. Next problem is how to get more discussion happening in this repo so that more than just 3-4 users are dictating the law for everyone.

@PowershellNinja
Copy link

PowershellNinja commented Jun 10, 2021

I found this issue after noticing the select behavior of the PowerShell extension changed (to the worse in my perception), but I'm really thankful there is a workaround that brings back the old behavior.
I have been using the extension on a daily basis since it was first published and it may look like a simple change, but if you're used to typing the extra $ sign after the copy/paste of a variable name, this change really breaks your working flow because you'be got extra $ signs everyhwere they dont belong 😮

So if I may suggest something for the future: changing default behavior that has been consistently the same for years is something that needs more consideration, and maybe an opt-in possibility instead of an opt-out would be friendlier on the regular users.

@andyleejordan
Copy link
Member

Perhaps we move the $ exclusion only into ISE mode, and keep the - exclusion? That seems to be a good option that would satisfy the most people.

@dragonwolf83
Copy link

From what I tested ISE doesn't include $ in selection so anyone relying on that behavior and using ISE mode would run into this same issue.

It just needs to default with $ as a word separator like it was. This is already a setting controllable by users but it is clearly not understood what setting does it and what change to make. If you can provide a section in PowerShell settings that make it easier to configure or at least explain how to use editor.wordSeparators to get the desired behaviors then that should be enough.

@andyleejordan
Copy link
Member

Sounds good, thanks all for the feedback. I'll make the change to put $ back into word separators, but continue to exclude - as everyone seems to like that, and update the documentation with a tip on how to exclude have double-click select all of $foo instead of just foo.

@andyleejordan andyleejordan self-assigned this Jun 14, 2021
@andyleejordan andyleejordan added this to the Committed-vNext milestone Jun 14, 2021
@andyleejordan andyleejordan removed the Needs: Maintainer Attention Maintainer attention needed! label Jun 14, 2021
@ThubLives
Copy link

I don't think we should keep the hyphen exclusion either. We've been focusing on the dollar sign, but I think most of the arguments against that change apply to both. Certainly PowerShell cmdlets are explicitly formed as Verb-Noun pairs and as such there is a lot of value in handling them as separate words.

For my own workflow, I regularly replace just the "verb" portion of the cmdlet. For example, I'll duplicate a line and change the verb on the new line to perform a complimentary operation with similar arguments.

As for arguments marked by hyphens, PowerShell isn't unique here either and none of those other languages make this particular exception. Again, I advocate for using the global default for word separators, both for the sake of $ and -.

@ghost ghost added the Needs: Maintainer Attention Maintainer attention needed! label Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UI Issue-Discussion Let's talk about it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants