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

Delete Roslyn's ICommandHandler interface and all support code #32623

Conversation

jasonmalinowski
Copy link
Member

@jasonmalinowski jasonmalinowski commented Jan 18, 2019

Now that the editor has their own public version of this API, and all code has been migrated to use the new system, we can delete all of the code that was supporting the old handlers. It may be surprising that much of this code was dead, but the command handler list that was being invoked by all of this was indeed empty.

Things still to do:

  • Test debugger intellisense
  • Test F#
  • Test TypeScript

@jasonmalinowski jasonmalinowski added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 18, 2019
@jasonmalinowski jasonmalinowski added this to the 16.1 milestone Jan 18, 2019
@jasonmalinowski jasonmalinowski self-assigned this Jan 18, 2019
@jasonmalinowski jasonmalinowski requested a review from a team as a code owner January 18, 2019 19:31
@jasonmalinowski
Copy link
Member Author

FYI to @ivanbasov, here's what all seems to be dead code in the area we were looking at.

To everyone: I'm not going to be trying to merge this for 16.0 as indeed there is much testing needed and perhaps also some shims that need to be added for F# and TypeScript. This was mostly done because @ivanbasov and I were looking at a bug and got very confused once we realized that almost all of what it was doing was dead, and this confirms it. So if you want to take a peek sure go ahead, but this is on the backburner for a few weeks.

break;

// If we see a RETURN, and we're in the immediate window, we'll want to rebuild
// spans after all the other command handlers have run.
case VSConstants.VSStd2KCmdID.RETURN:
ExecuteReturn(subjectBuffer, contentType, executeNextCommandTarget);
result = NextCommandTarget.Exec(ref pguidCmdGroup, commandId, executeInformation, pvaIn, pvaOut);
Copy link
Member Author

Choose a reason for hiding this comment

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

The existing code here has some subtle bugs: the Execute* methods that took executeNextCommandTarget would update result, but the code that didn't would fail to update result and we'd always return S_OK. At this point, I'm leaving the bugs in place.

}
}

break;

default:
return base.ExecuteVisualStudio2000(ref pguidCmdGroup, commandId, executeInformation, pvaIn, pvaOut, subjectBuffer, contentType);
return base.ExecuteVisualStudio2000(ref pguidCmdGroup, commandId, executeInformation, pvaIn, pvaOut);
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this appears to be a bug where this would return and we'd never do the SetStateFlags below. Leaving the bug here for now.

Copy link
Contributor

@ivanbasov ivanbasov left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -90,33 +63,11 @@ void executeNextCommandTarget()
switch ((VSConstants.AppCommandCmdID)commandId)
{
case VSConstants.AppCommandCmdID.BrowserBackward:
ExecuteBrowserBackward(subjectBuffer, contentType, executeNextCommandTarget);
ExecuteBrowserBackward(executeNextCommandTarget);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add editor commanding support for these commands so you could switch to modern command handlers and get rid of this command filter altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not even sure why we have this in the first place. I think this is wiring up back/forward mouse commands to invoke the buttons on the UI. Why that's just not handled in the shell I don't know.

Copy link
Member

Choose a reason for hiding this comment

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

it would be great to start a discussion on that so we can remove this entirely.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Awesome.

@RikkiGibson RikkiGibson changed the base branch from dev16.1-preview1 to master February 8, 2019 05:43
@jasonmalinowski jasonmalinowski force-pushed the delete-legacy-commanding-infrastructure branch from 4c3a164 to 9df750a Compare March 6, 2019 03:10
@sharwell
Copy link
Member

sharwell commented Mar 6, 2019

@jasonmalinowski integration test failures look like true failures for this PR

@jasonmalinowski
Copy link
Member Author

@sharwell, great, thanks!

@jasonmalinowski jasonmalinowski force-pushed the delete-legacy-commanding-infrastructure branch from 9df750a to d420694 Compare March 6, 2019 20:27
@jasonmalinowski jasonmalinowski force-pushed the delete-legacy-commanding-infrastructure branch from d420694 to d61705d Compare April 15, 2019 18:28
@jinujoseph jinujoseph modified the milestones: 16.1, 16.3 Jun 9, 2019
@jasonmalinowski jasonmalinowski modified the milestones: 16.3, 16.4 Sep 6, 2019
@jasonmalinowski jasonmalinowski force-pushed the delete-legacy-commanding-infrastructure branch 2 times, most recently from 1d31614 to 858746b Compare September 25, 2019 22:32
@jasonmalinowski jasonmalinowski removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 25, 2019
@jasonmalinowski jasonmalinowski force-pushed the delete-legacy-commanding-infrastructure branch from 858746b to 50d10b8 Compare September 26, 2019 19:27
…tation

Now that TypeScript has moved to the newer editor commanding APIs,
the legacy implementation is no longer needed.
The command was ultimately being made available not because our command
handler said it should be, but our implementation of QueryStatus
in AbtractOleCommandTarget was unilterally saying the command is
available. This moves that into here.
Now that the editor has their own public version of this API, and all
code has been migrated to use the new system, we can delete all of the
code that was supporting the old handlers. It may be surprising that
much of this code was dead, but the command handler list that was being
invoked by all of this was indeed empty.

The intent of this particular commit is to be a fairly mechanical change
that leaves other bugs or strangeness in place. In particular,
DebuggerIntelliSenseFilter had a special ExecuteVisualStudio2000 that
removes the buffer's read-only flag, but in many codepaths we never
set that back. In other cases, we also ignored the HRESULT that came
from the next filter in the chain. These all appear to be bugs, but I
won't touch them for now.

There's also some remaining work happening in
AbstractOleCommandTarget.Execute.cs that probably should either be moved
to the core editor code (if it wasn't already), or into the command
handlers of Roslyn itself.
My previous commit deleted this interface and a constructor that
consumes it. But TypeScript is still MEF importing the interface just
to pass it to the constructor; this adds both of those back. Once
TypeScript is off of this constructor this commit can be reverted
verbatim.
Now that the old code is dead, we don't need to qualify things anymore.
@jasonmalinowski jasonmalinowski force-pushed the delete-legacy-commanding-infrastructure branch from 50d10b8 to 559f6ff Compare September 26, 2019 22:43
@jasonmalinowski jasonmalinowski merged commit 796a5d2 into dotnet:master Sep 27, 2019
@jasonmalinowski jasonmalinowski deleted the delete-legacy-commanding-infrastructure branch September 27, 2019 20:34
@sharwell sharwell mentioned this pull request Jul 30, 2021
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants