Skip to content

Conversation

@DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Jun 16, 2023

Addresses posit-dev/positron#681

This really shot up to Private Alpha priority for me because it was happening all the time, especially when typing options() in the console for some reason.

This is really a follow up to this commit posit-dev/positron@6c0f1a9, which is related to posit-dev/positron#340.

In that commit, we handle out of order changes by storing them until we see the next consecutive change, but we could run into a scenario like this:

got version 1
apply version 1

got version 4
defer version 4 

version 2
apply version 2
apply version 4 (here is the issue!)

In other words, once we hit the next consecutive change we just unload all the changes without checking that they are also consecutive.

This PR fixes this by only applying as many consecutive changes as possible, and then we re-push the remaining changes that we can't apply right now as pending again.

Here is me catching the problem with some extra logging

Screen Shot 2023-06-16 at 10 06 29 AM

And here it is now with the fix

Screen Shot 2023-06-16 at 11 02 11 AM

@DavisVaughan DavisVaughan requested a review from kevinushey June 16, 2023 15:23
@DavisVaughan DavisVaughan changed the title Only apply consecutive changes when updating Only apply consecutive changes when updating document states Jun 16, 2023
@DavisVaughan DavisVaughan requested a review from lionel- June 20, 2023 14:02
Copy link
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

This seems to make good sense from an exterior point of view.

My only comment is that it seems the flow would be a bit simpler by removing the take() call and splitting pending into changes, pending directly, if possible.

@DavisVaughan
Copy link
Contributor Author

@lionel- and I discussed offline and it seems like take() is probably the cleanest way to do this still

@DavisVaughan DavisVaughan merged commit e12608b into main Jun 22, 2023
@DavisVaughan DavisVaughan deleted the fix/consecutive-document-update branch June 22, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants