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

Merge touch document actions #73399

Merged
merged 2 commits into from
May 9, 2024
Merged

Conversation

CyrusNajmabadi
Copy link
Member

I figured out what this logic was trying to do and i was able to merge the types.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 9, 2024 02:04
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 9, 2024
ProjectState oldProjectState,
ProjectState newProjectState,
DocumentState oldState,
Copy link
Member Author

Choose a reason for hiding this comment

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

passing in oldState/oldStates is redundant given that we have oldProjectState.

@CyrusNajmabadi
Copy link
Member Author

@ToddGrun ptal.

{
private readonly DocumentState _oldState = oldState;
private readonly DocumentState _newState = newState;
private readonly ImmutableArray<DocumentState> _oldStates = newStates.SelectAsArray(s => oldProjectState.DocumentStates.GetRequiredState(s.Id));
Copy link
Contributor

Choose a reason for hiding this comment

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

_oldStates

why have this as a member? Can't each entry just be calculated inside the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be. but then i needed this here and for the sequence-equals. so basically, it was just nice to have the member.

note: if there's a problem here, we could always change this to be a OneOrMany. It's highly likely that thsi will be the 'One' case for the normal mode of a user editing a document. So we'd save on the array in that case. That said. I think this is unlikely to matter in practice since the arrays will be small, and GC'ed asap. So i'd like to keep this as-is for simplicity.

if (priorAction is TouchDocumentAction priorTouchAction &&
priorTouchAction._newState == _oldState)
if (priorAction is TouchDocumentsAction priorTouchAction &&
priorTouchAction._newStates.SequenceEqual(_oldStates))
Copy link
Contributor

Choose a reason for hiding this comment

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

priorTouchAction._newStates.SequenceEqual(_oldStates)

Is this more likely to happen than I think it is? (seems like a never would happen thing to me)

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi May 9, 2024

Choose a reason for hiding this comment

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

it's very likely. this is what happens when you get a stream of edits coming in for one document (basically, the typing scenario).

we can either keep making a longer and longer chain of 'edits' where the compilation goes through N steps to get to hte end. Or we can say that the actions 'merge' and we'll jump from teh starting compilation all the way to teh end step.

So, instead of A->B->C->D, we can collapse as we're adding these and end up with just an A->D transition.

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit c99909b into dotnet:main May 9, 2024
25 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the touchDocuments branch May 9, 2024 18:09
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 9, 2024
@CyrusNajmabadi CyrusNajmabadi restored the touchDocuments branch May 9, 2024 19:52
@CyrusNajmabadi CyrusNajmabadi deleted the touchDocuments branch May 9, 2024 19:52
@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski For review when you get back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants