-
Notifications
You must be signed in to change notification settings - Fork 783
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 bad merge of LanguageService.fs
which was causing issues with the editor keystrokes
#3442
Conversation
…he editor keystrokes
@brettfo I need to understand this more.
|
@brettfo Also, are you sure this was a "bad merge"? thanks |
I was thinking exactly the same questions. I can't see the "bad merge" yet
2017-08-16 12:01 GMT+02:00 Don Syme <[email protected]>:
… @brettfo <https://github.com/brettfo> Also, are you sure this was a "bad
merge"? thanks
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3442 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNKY1ArD4XpakDtZNU2JPKLUc9SHWks5sYr3ggaJpZM4O4MQA>
.
|
@KevinRansom's initial solution was to lock, but it turns out that the code he was locking wasn't even necessary. Project tracking is already done by the underlying project system. The change in #3238 introduced redundant project tracking that seems to have overwritten the existing project tracking. This was resulting in those keys sending what amounted to garbage to the Visual Studio key command handler. The change represented in #3419 simply rids us of that code because it wasn't needed in the first place. Our plan at release was to merge |
@cartermp @brettfo Looking at the changed code, I don't see how this PR is specifically related to #3419 or #3434. This PR seems to be reapplying part of #3387 which was a potential fix for #3033 but had not been properly tested so was reverted by @KevinRansom in master. I do see that the code in master was busted before this fix since it didn't have any recursive call to Anyway, as things stand I'm going to assume master fixes the arrow key problem, and in addition we seem to now have #3387 in master. That might be a good change, and it might fix #3033 (which would be great), but we should validate. |
Nah, I still don't get it, sorry :) When I look at the changes made by this PR, they are not bringing in the changes from #3419. They are instead applying part of #3387 (which happened to be in master at the time #3419 was made, but was later reverted). AFAICS the current state of master before this PR was kind of broken. After this PR #3387 has effectively been applied. That doesn't mean master is now broken - it might be great - it depends if #3387 is a good fix for #3033. |
@dsyme, the concurrent fix was not the right solution. Project construction adds the tracker, and so adding it here is not necessary, and indeed causes the problem. The is project already added test should always succeed, except occasionally it didn't hence the lock. This change changes the code to what is in currently vs2017-rtm which we know cures the keyboard problems, on new projects and multi-project solutions. |
I'm totally fine with this. I'm just pointing out that this means #3387 has effectively been applied in vs2017-rtm and master (i.e. should no longer be considered reverted), and we can now validate that it fixes #3033 as it was intended to do |
@dsyme ... it was a forward thinking bug fix :-) |
@dsyme I think it's best to just consider this merge as literally as possible. There was a previous merge that did not incorporate the changes to this file that were in the shipping branch (vs2017-rtm). Because nightlies are based on master, we needed to get those changes over. This PR completes that process. Effective application of #3387 is coincidental in nature. If it turns out that #3033 is also fixed as a result of the effective application of #3387, then that's good. If it isn't, then we are at the same position we were when #3303 was created, but with the arrow/backspace/enter keys fixed. |
@cartermp Yep, thanks :) |
No description provided.