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

Fix color boxes not updating in some cases #1899

Merged
merged 3 commits into from
Nov 18, 2021
Merged

Conversation

rchl
Copy link
Member

@rchl rchl commented Nov 16, 2021

As was noticed in #1839 (comment), it was incorrect to use _when_selection_remains_stable_async for the features that relied on view changes being reported first.

The _when_selection_remains_stable_async would cause the request to be skipped if the selection has changed since it was called. That means that changing the document and then quickly moving the cursor would result in color boxes not being requested/updated.

And on top of that, we might have also requested color boxes before the didChange notification has been triggered thus resulting in out of sync positioning of boxes.

Moved the logic to SessionBuffer where it can be called after the didChange notification and added checks if view is still alive and whether the document has remained the same on getting results from session.

We also shouldn't need any extra debouncing as didChange notifications are already debounced.

As a side effect, we'll request color boxes from all active sessions that support them rather then only the first one. This might be good or bad but if it's an issue, the user can disable relevant capability for one of the sessions.

@rchl rchl mentioned this pull request Nov 16, 2021
Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

Shouldn’t the PhantomSets go into the SessionView? I think the color regions are correctly stored in SessionBuffer but now clones won’t show the same phantoms right? Maybe they didn’t before as well.

@rchl
Copy link
Member Author

rchl commented Nov 18, 2021

Shouldn’t the PhantomSets go into the SessionView? I think the color regions are correctly stored in SessionBuffer but now clones won’t show the same phantoms right? Maybe they didn’t before as well.

How do we create clones again? Was it "Split View"? If so then it works as is. My best guess is that even though ST provides the api on the View, it internally ties phantoms to buffers and we are doing the right thing by creating them once instead of for each view clone.

Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

In that case lgtm

@rchl rchl merged commit 2b98639 into main Nov 18, 2021
@rchl rchl deleted the fix/color-boxes-updates branch November 18, 2021 21:17
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