8000 Make the 'PSESRemoteSessionOpenFile' a support event by daxian-dbw · Pull Request #652 · PowerShell/PowerShellEditorServices · GitHub
[go: up one dir, main page]
Skip to content

Make the 'PSESRemoteSessionOpenFile' a support event #652

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 16, 2018
Merged

Make the 'PSESRemoteSessionOpenFile' a support event #652

merged 1 commit into from
Apr 16, 2018

Conversation

daxian-dbw
Copy link
Member

Make the PSESRemoteSessionOpenFile a support event, so Get-EventSubscriber won't show up that subscriber. Get-EventSubscriber -Force can still show the support events. Unregister-Event -Force needs to be used to remove a support event.

The event subscriber for PSInternalRemoteDebuggerStopEvent and PSInternalRemoteDebuggerBreakpointUpdatedEvent are already made support events, see ServerRunspacePoolDriver.cs.

@daxian-dbw daxian-dbw mentioned this pull request Apr 10, 2018
Merged
11 tasks
@TylerLeonhardt
Copy link
Member

@daxian-dbw will this still work on Windows PowerShell?

@daxian-dbw
Copy link
Member Author

@tylerl0706 Yes, I believe so, but we surely should test it before moving forward. I would need your help to build and deploy powershell extension locally to test this.

SeeminglyScience
SeeminglyScience approved these changes Apr 11, 2018
View reviewed changes
Copy link
Collaborator
@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

I don't have a remote machine to test with atm but I can confirm support events work fine in Windows PowerShell. Definitely a good idea.

@daxian-dbw
Copy link
Member Author

Actually, I have a question: why don't we use the existing PSISERemoteSessionOpenFile event in PowerShell? I guess (not sure) PowerShell will take care most the setup on the remote side if PowerShell extension register PSISERemoteSessionOpenFile on the local event manager. See https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/remoting/server/ServerRemoteHost.cs#L360

@TylerLeonhardt
Copy link
Member

@daxian-dbw I'm not sure. Checking with @daviwil on this.

@TylerLeonhardt
Copy link
Member

@daxian-dbw from David:

"
not sure why we didn't use it

my implementation for that came from the ISE codebase, so it's possible it was meant to be used in the ISE and never was
thus I never knew about it
"

@rjmholt
Copy link
Contributor
rjmholt commented Apr 12, 2018

I don't fully understand what a support event is, but this seems like a good change.

@TylerLeonhardt
Copy link
Member

I'm with Rob on this.

@SeeminglyScience
Copy link
Collaborator

It just makes it hidden by default, harder to accidentally unsubscribe it with something like Get-EventSubscriber | Unregister-Event . Good practice for events not set directly by the user.

@TylerLeonhardt
Copy link
Member
TylerLeonhardt commented Apr 12, 2018 •
edited
Loading

I'm interested in hearing what @daxian-dbw has to say about PSISERemoteSessionOpenFile. From what David was saying, the code isn't actually used anywhere?

I wonder if it's worth considering the removal of PSISERemoteSessionOpenFile from PowerShell.

I could be misunderstanding.

@TylerLeonhardt
Copy link
Member

@PaulHigin, @daviwil mentioned that you might know something about this PSISERemoteSessionOpenFile event.

@PaulHigin
Copy link

I believe the PSESRemoteSessionOpenFile event is similar to the PSISERemoteSessionOpenFile event and does the same thing. The question is why PowerShell Editor Services uses a different event and if it could just use the built-in PSISERemoteSessionOpenFile event.

The PSISERemoteSessionOpenFile event implements 'psedit' function in a remote session that fires the PSISERemoteSessionOpenFile event over the remoting channel back to the host. The host (currently ISE) uses this event to copy the file from the remote target and allow users to edit it in ISE, and Save will copy the file back to the remote target.

I believe PSESRemoteSessionOpenFile event does the same thing so it might make sense to update PS Editor Services to use it. Feel free to contact me offline for more information.

@TylerLeonhardt
Copy link
Member
TylerLeonhardt commented Apr 12, 2018 •
edited
Loading

ok then we can merge this in for now and then open an issue to update it to use PSISERemoteSessionOpenFile.

I think we should also open an issue on PowerShell to rename/shim that (in a nice way) to something that doesn't have "ISE" in it.

... Unless @daxian-dbw would like to give it a try in this PR of course :) if not it's ok

TylerLeonhardt
TylerLeonhardt approved these changes Apr 16, 2018
View reviewed changes
Copy link
Member
@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM :)

@TylerLeonhardt TylerLeonhardt merged commit e28c9eb into PowerShell:master Apr 16, 2018
@daxian-dbw daxian-dbw deleted the minor branch April 23, 2018 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerLeonhardt TylerLeonhardt TylerLeonhardt approved these changes

@rjmholt rjmholt rjmholt approved these changes

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

@rkeithhill rkeithhill Awaiting requested review from rkeithhill

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@daxian-dbw @TylerLeonhardt @rjmholt @SeeminglyScience @PaulHigin
0