Fixed DDRIT failure #25551
Merged
Fixed DDRIT failure #25551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t change. this is needed to bring workspace up to date with latest text snapshot after we don't merge 2 text snapshot to 1 source text if content is same. see this for more detail - dotnet#24849
|
@jasonmalinowski @dotnet/roslyn-ide @jinujoseph @Pilchie can you take a look? |
heejaechang
force-pushed
the
eventWithNoChange
branch
from
1b0215e
to
f297127
Compare
Pilchie
approved these changes
Mar 17, 2018
| @@ -100,18 +100,21 @@ public override SourceText CurrentText | |||
| private void OnTextContentChanged(object sender, TextContentChangedEventArgs args) | |||
| { | |||
| var changed = this.EtextChanged; | |||
| if (changed != null && args.Changes.Count != 0) | |||
| if (changed == null) | |||
There was a problem hiding this comment.
I would suggest a comment indicating that it's important to fire the event even if there are no changes so that consumers can observe the new snapshot.
Otherwise, someone is likely to re-add the optimization at some point in the future.
|
@jasonmalinowski is this right branch to checkin this change? |
|
ping? |
jinujoseph
approved these changes
Mar 18, 2018
jasonmalinowski
pushed a commit
to jasonmalinowski/roslyn
that referenced
this pull request
Mar 19, 2018
* made sure we raise workspace changed events even when there is no text change. this is needed to bring workspace up to date with latest text snapshot after we don't merge 2 text snapshot to 1 source text if content is same. see this for more detail - dotnet#24849 * added comments on why we need to process all text change events
heejaechang
pushed a commit
that referenced
this pull request
Mar 20, 2018
…apshot without changes (#25576) * Fixed DDRIT failure (#25551) * made sure we raise workspace changed events even when there is no text change. this is needed to bring workspace up to date with latest text snapshot after we don't merge 2 text snapshot to 1 source text if content is same. see this for more detail - #24849 * added comments on why we need to process all text change events * removed changes empty check. now it is possible to have empty changes. * remove non-empty from comment since set now can be empty.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixed issue workspace not raising events if ITextBuffer change doesn't include any text change.
(Fixed cases where 1:1 mapping between Roslyn and editor text buffer/s… #24849)
...
Customer scenario
No user facing change.
Bugs this fixes
N/A
Workarounds, if any
No workaround.
Risk
No risk
Performance impact
it should let us to be slightly more efficient before by not forking solution from Workspace.CurrentSolution unnecessarily.
Is this a regression from a previous update?
Yes
Root cause analysis
We used to share same SourceText for different ITextSnapshots if contents are same. which caused some issues since we expect SourceText to ITextSnapshot is 1:1 map. not 1:n map.
when that is fixed, I forgot to remove optimization in ETextChange event where it swallows events if contents are same.
How was the bug found?
Testing (DDRIT)