Decouple a bit of RenameTrackingTaggerProvider #31890
Open
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
This addresses a timing issue, where we would call UpdateTrackingSessionIfRenamable even the session had already been dismissed; this function implicitly assumed the session making the call was still active. Another helper function OnTrackingSessionUpdated which raises the session update events already had checks for this, but UpdateTrackingSessionIfRenamable bypassed that. The core of this fix is to remove the offending code in UpdateTrackingSessionIfRenameable and just have the TrackingSession call OnTrackingSessionUpdated. That's already done in many other places and is much simpler. Upon further inspection of UpdateTrackingSessionIfRenamable it was also suspicious because it was having to grovel in the state of the active session to know if it should call CheckNewIdentifier. Moving that check into the function directly simplifies the code and decouples the types just a little bit.
jasonmalinowski
force-pushed
the
decouple-a-bit-of-rename-tracking
branch
from
0d462e9
to
a603471
Compare
JoeRobich
approved these changes
Jan 9, 2019
|
@jasonmalinowski do we need this PR anymore? Can we close it out or bring it to completion? |
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.
This addresses a timing issue, where we would call UpdateTrackingSessionIfRenamable even the session had already been dismissed; this function implicitly assumed the session making the call was still active. Another helper function OnTrackingSessionUpdated which raises the session update events already had checks for this, but UpdateTrackingSessionIfRenamable bypassed that. The core of this fix is to remove the offending code in UpdateTrackingSessionIfRenameable and just have the TrackingSession call OnTrackingSessionUpdated. That's already done in many other places and is much simpler.
Upon further inspection of UpdateTrackingSessionIfRenamable it was also suspicious because it was having to grovel in the state of the active session to know if it should call CheckNewIdentifier. Moving that check into the function directly simplifies the code and decouples the types just a little bit.
This implicitly fixes #30661. The work of me understanding this code eventually led to us realizing that our move to JTF broke this and #31787 is probably necessary as a more general fix. I didn't want to lose this refactoring independent of that change, as to make this a bit cleaner the next time we need to make a change here.