-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Only apply one document info change for linked documents #71220
Conversation
62d2816
to
680014b
Compare
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it could break things that should work. Say, for example, the user performs a fix-all on code like:
#if DEBUG
// code that is active in some project
#else
// other code that is not.
#endif
with the normal batch fixer, this would work, right? Or maybe it wouldn't? I'm not exactly sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it won't be affected. This change would only affect things like file name, path change. For content change, each document would still be changed even they are linked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good ot know! probably worth a test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine we might end up actually taking the ApplyChangedDocument path more than once, but we'll end up writing the same text each time -- the documents should have had merged texts. If we look under a debugger and see we are indeed doing that, that might be a nice potential win here to only process that once.
That said, I'm not sure how we exactly fit that in here -- we're just calling ApplyProjectChanges for both projects and we can't just pass additional parameters since that's a protected API. So either we have to break the API, or maybe pre-filter the ProjectChanges to remove the duplicate changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it sounds like a future work? (e.g. we have to see how things work for text changes)
84a95f9
to
82d7d4c
Compare
82d7d4c
to
dd66b72
Compare
This reverts commit dd66b72.
OK, tag @jasonmalinowski to review. |
@Cosifne Definitely a good start, let's get integration tests working before merge. @CyrusNajmabadi's point about duplicates does make me think there might be a nice win too around applying text edits that we might want to do as well, if you can make the code work without breaking APIs too much. |
@jasonmalinowski Review for second review |
Fix https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1903953/
So the story is, when renaming a class (also rename the containing type) in multiple TFMs (let's say two TFMs), there would be
for each of the linked documents.
In current Implementation
We will apply the document change and document info (rename file) change for the first documents. And the
info change
will call this line . Start from here we are asking shell -> project system -> our project system listeners to update the document inWorkspace.CurrentSolution
. As a result, both of the linked documents will be updated.And when Workspace tries to apply the change for the second document, it can't find it (because it get changed because of the first document rename) therefore the rename operation just fails.
Changes in this PR
Separate the info change into a sperate method and do that only after applying project changes. Also, for linked document, we only need to apply one info change since it's going to rename all the linked document from the file system.
TODO:(Done)I want to add test coverage. But right now the integration test harness is broken so tests can't be launched from VS. Will need to wait it to be resolved first.