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

Better change tracking for Semantic highlighting for CSL #8099

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lkishalmi
Copy link
Contributor

@lkishalmi lkishalmi commented Dec 29, 2024

Instead of tracking changes in a series of Edit records between parsing there could be Document Position used, which automatically track the offset changes in the edited document.

This version further simplifies the highlighter.

Also resolves a minor issue, that would not extend the previous highlight when the new text is just appended after a whole token.

Before:
Screencast From 2024-12-29 15-53-25.webm

After:
Screencast From 2024-12-29 15-50-57.webm

This may have better performance as well. There was some minor flickering on highlights when the edit list grew "large" (basically that needed a lot of monkey style keyboard clamping, to be noticeable)

I have a minor aversion to put Position which is a mutable thing info the SequenceElementrecord, though it is used to grouping data together, also changes in the document, shall not change the order of the instances when put in a collection, so I could live with that. Please tell me if you have something better in mind.

@lkishalmi lkishalmi added CSL [ci] enable web job Editor labels Dec 29, 2024
@lkishalmi lkishalmi added this to the NB25 milestone Dec 29, 2024
@mbien
Copy link
Member

mbien commented Dec 30, 2024

hmm. I like the simplifications. Although Comparable doesn't mention anything regarding mutability, i believe some sort methods will throw an exception when they notice that something changed during sort (somewhat analog to ConcurrentModificationException). But if it is accessed under the right locks - everything should be fine.

@lkishalmi lkishalmi force-pushed the even-better-csl-semantic-highlighter branch from 73af1c8 to 419b0a4 Compare December 30, 2024 03:58
@lkishalmi
Copy link
Contributor Author

At the end removed Comparable interface and added a static Comparator, in order to express that SequenceElements are not really compared, and make the usage of the required comparison more explicit.

@lkishalmi lkishalmi force-pushed the even-better-csl-semantic-highlighter branch 2 times, most recently from b9a60d7 to 7c133c3 Compare December 30, 2024 19:55
@matthiasblaesing matthiasblaesing added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Dec 31, 2024
@matthiasblaesing
Copy link
Contributor

Before this is progressed further a regression needs to be fixed first. See #8102

@lkishalmi lkishalmi force-pushed the even-better-csl-semantic-highlighter branch from 7c133c3 to 4cc8d6f Compare January 1, 2025 21:49
@lkishalmi lkishalmi removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 1, 2025
@lkishalmi
Copy link
Contributor Author

Rebased the code from master with @matthiasblaesing fix, added one more unit test to check the skipping of empty highlights.

@lkishalmi lkishalmi force-pushed the even-better-csl-semantic-highlighter branch from 4cc8d6f to 2c140c7 Compare January 1, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSL [ci] enable web job Editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants