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

Run Selected Text in Active Terminal should not execute when nothing is selected #19375

Closed
jaywryan opened this issue Jan 25, 2017 · 12 comments
Closed
Assignees
Labels
feature-request Request for new features or functionality terminal Integrated terminal issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@jaywryan
Copy link

this may be related to issue #9715

  • VSCode Version: 1.8.1
  • OS Version: Windows 7 x64

Steps to Reproduce:

  1. Open as script
  2. I am using Powershell extension, Powershell Script and Powershell Active Terminal
  3. Ensure nothing is selected and invoke "Terminal: Run Selected Text in Active Terminal"

This runs the entire script (line by line or character by character as noted in #9715) even if there is no selection. Even if the script has a break or return at the beginning, the code afterward still gets executed.

Suggestions:

  • make this behave like the Powershell_ISE (execute the line of code where the cursor is) - would have to be in the Powershell Extension
  • not execute any code in the active terminal when nothing is selected
@mousetraps mousetraps added the terminal Integrated terminal issues label Jan 26, 2017
@Tyriar
Copy link
Member

Tyriar commented Jan 26, 2017

This was actually a feature request with subsequent pull request to run the whole file if nothing was selected which I like personally. I'm hesitant to add a setting for this as it would probably see minimal use and cause bloat. Thoughts @daviwil @gerane? Is running a line over the whole file a common scenario?

Looking into #9715 might be a better solution?

@Tyriar Tyriar added the info-needed Issue requires more information from poster label Jan 26, 2017
@jaywryan
Copy link
Author

Thanks for the quick response. I agree, fixing #9715 would at least correct the unforseen behavior below.

Break
Delete-allthethings

Which deletes all the things instead of breaking. I'm happy to close this issue, perhaps the ISE parity aspect of it is better suited for the extension and the plans their.

Thanks.

@gerane
Copy link

gerane commented Jan 26, 2017

@Tyriar I think there should be a separate key combo/command for run file and keep it separate from run selection. This could be potentially damaging if accidentally ran.

If we look at PowerShell as an example, it's often code dealing with managing systems. It could lead to dangerous code being run by an accident. This has happened to me but luckily wasn't anything damaging. You think you have code selected but you didn't, or you unselect code right as you run the selection.

The run selection is mostly broken for me and I can't really use it a whole lot currently due to the PSReadline line ending issues. I know from a PowerShell perspective, if you compare to ISE, it used f8 for run selection, and f5 would run the file, and would debug if a break point was set. You kind of get this currently with the PowerShell Extension, except it isn't currently connected to the internal terminal. Have you and @daviwil spoken about how things might be handled once those two are working together? If the debugger output was directed to the internal terminal window instead of the debugger window, this might replace the need of having a run whole file of nothing is selected.

@gerane
Copy link

gerane commented Jan 26, 2017

What makes it worse with psreadline, is you have to watch each line run individually without being about to ctrl+c to break execution. So as it's scrolling through erroring on most lines you start frantically thinking about what code is being executed and if it had anything horrible in it that might break something lol. Just praying you didn't have any recursive deletes that might point at the wrong location due to a variable not getting populated properly or something.

I associate running the file with f5. I'm already expecting the file to be ran, so less chance of an accident. If no break points are set, you could just have the file execute with output in terminal without debugging opening. Add a break point and it opens debugging but allows you to interact inside the terminal. This sort workflow seems like the direction you'd want to go.

@Tyriar
Copy link
Member

Tyriar commented Jan 26, 2017

I definitely can't hijack the F5 keybinding for terminal-related stuff, maybe adding another command to run the file and removing the run if empty behavior would be the best way to go.

Users could always bind it like this to get them running under the same keybinding:

{ "key": "somekey", "command": "workbench.action.terminal.runSelectedText", "when": "editorHasSelection" }
{ "key": "somekey", "command": "workbench.action.terminal.runFile", "when": "!editorHasSelection" }

@Tyriar Tyriar added help wanted Issues identified as good community contribution opportunities feature-request Request for new features or functionality and removed info-needed Issue requires more information from poster labels Jan 26, 2017
@Tyriar Tyriar added this to the Backlog milestone Jan 26, 2017
@Tyriar Tyriar added the good first issue Issues identified as good for first-time contributors label Jan 26, 2017
@gerane
Copy link

gerane commented Jan 26, 2017

@Tyriar how exactly does debugger work in code? I wasn't necessarily saying to hijack it, but more that it would be nice to see more integration between them.

@daviwil
Copy link
Contributor

daviwil commented Jan 26, 2017

When I get the PowerShell console integration going, I'll take care of rerouting "run selection" behavior to the right place in a way that makes sense for PowerShell development.

@Tyriar
Copy link
Member

Tyriar commented Jan 26, 2017

@gerane I don't know too much about the debugger, I do know 1.9.0 adds the ability to run/debug via F5 javascript files without needing a launch.json file. So that leads me to believe @daviwil could handle that in the PS extension.

@daviwil
Copy link
Contributor

daviwil commented Jan 26, 2017

Yep, can definitely handle it. I've actually got it working correctly in a private branch of the PS extension, just need to get that working with a new approach for exposing the PS console through the terminal UI.

@Tyriar
Copy link
Member

Tyriar commented Feb 3, 2017

@daviwil @gerane if a "run file in terminal" command was added to replace this, would it make sense to send the absolute file path to the terminal instead of sending the file's contents? Would PS behave if I sent:

C:\\blah\\file.ps1\n

Would that also work in cmd (would file associations work)?

@daviwil
Copy link
Contributor

daviwil commented Feb 3, 2017

I don't think it works in cmd, on my machine it just causes the associated editor to open that .ps1 file. Works in PowerShell, though.

@Tyriar Tyriar closed this as completed in e325dec Feb 4, 2017
@Tyriar Tyriar removed good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities labels Feb 4, 2017
@Tyriar Tyriar modified the milestones: February 2017, Backlog Feb 4, 2017
@Tyriar
Copy link
Member

Tyriar commented Feb 4, 2017

I've changed the behavior to send the active line (#19863) and added a new command workbench.action.terminal.runActiveFile that runs the active file. It does this by sending the file name and a \n to the terminal if it's a file on disk (not untitled files). Testing this out on Windows would be appreciated, should land in Insiders on Monday 😃

We may want to add some special cases for Windows that add the program to run based on the file name, Unix-like OS' have the shebang syntax which solves this problem. I'm a little weary on adding explicit support for this sort of stuff unless it's just totally broken on Windows, which I think it may be except for .ps1 scripts in the default shell.

@Tyriar Tyriar added the verification-needed Verification of issue is requested label Feb 21, 2017
@roblourens roblourens added the verified Verification succeeded label Feb 24, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality terminal Integrated terminal issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants