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

Fix command handler ordering #18822

Closed
wants to merge 4 commits into from

Conversation

rchande
Copy link
Contributor

@rchande rchande commented Apr 19, 2017

Customer scenario

With automatic brace completion and intellisense active, customer types shift+enter to commit intellisense, complete the pending brace completion, and complete the statement. Completion is committed, but brace completion is not.

Bugs this fixes:

#18065

Workarounds, if any

Customer has to manually finish their braces because this keyboard shortcut does not work.

Risk

Moderately risky. It is possible that this change will introduce an unanticipated change in the interaction between completion and brace completion.

Performance impact

Low. We will do extra work to commit brace completion if the user uses the command.

Is this a regression from a previous update?

No, we shipped Dev14 like this.

Root cause analysis:

There were integration tests covering this scenario but they had the wrong baselines or were skipped.

How was the bug found?

We noticed this while porting integration tests to the new framework.

@rchande rchande added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 19, 2017
@rchande
Copy link
Contributor Author

rchande commented Apr 26, 2017

retest this please

@rchande rchande removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 26, 2017
@rchande
Copy link
Contributor Author

rchande commented Apr 26, 2017

Tag @dotnet/roslyn-ide for review.
The VSI failures are unrelated error list failures. The ETA failure is a test whose baseline needs to be updated to reflect this change.

@jasonmalinowski
Copy link
Member

@rchande: please update the commit messages and PR description to describe what this change is or what bug it's fixing or whatever. 😄

@rchande
Copy link
Contributor Author

rchande commented Apr 26, 2017

Oops! Done @jasonmalinowski

@rchande rchande force-pushed the commandhandlerordering branch from de07656 to 12f832b Compare April 26, 2017 23:02
Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

There are three references to #18065 in the repository code that need to be updated:
https://github.com/dotnet/roslyn/search?utf8=%E2%9C%93&q=18065&type=

Ravi Chande added 3 commits July 19, 2017 13:39
If completion and brace completion are both active, shift-enter should
commit both and complete the statement. This was not working because the
completion command handler never called next handler for shift-enter and
the line ender command handler was not ordered correctly.

Fixes dotnet#18065
@rchande rchande force-pushed the commandhandlerordering branch from 12f832b to 441e251 Compare July 19, 2017 20:46
@rchande rchande dismissed sharwell’s stale review July 19, 2017 21:21

Updated references

@rchande
Copy link
Contributor Author

rchande commented Jul 19, 2017

Ping @dotnet/roslyn-ide. Anyone else want to take a look?

@rchande
Copy link
Contributor Author

rchande commented Sep 27, 2017

@jasonmalinowski Do you think this is still worth taking?

@jaredpar
Copy link
Member

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!

@jaredpar jaredpar closed this Apr 24, 2018
@jaredpar jaredpar added the Resolution-Expired The request is closed due to inactivity under our contribution review policy. label Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE cla-already-signed Resolution-Expired The request is closed due to inactivity under our contribution review policy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants