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

Reengineer theme application to match Sublime #209

Merged
merged 2 commits into from Sep 25, 2018
Merged

Reengineer theme application to match Sublime #209

merged 2 commits into from Sep 25, 2018

Conversation

trishume
Copy link
Owner

Fixes #133 and additional problems discovered in #203 while not noticeably regressing end-to-end performance.

This supersedes #203 and I'll close it and point to this PR. This PR does use the main piece of code from #203 to reimplement one of the highlighter methods as well as the ideas and cases from that PR so thanks @keith-hall!

It took a number of hours and a few false starts to figure out a way to be correct like #203 while not regressing performance but I came up with a way that is asymptotically the same as current syntect master and with a small difference in constant factors that is within the noise of the current benchmarks.

It does this by caching the matching styles and scores of single-scope selectors (the majority) because pushing additional things can't make them un-match. This doesn't apply to multi-scope selectors with exclusions which are applied at the end to finalize the style.

This PR removes some previously public methods that had incorrect behaviour. I don't think anyone used them but this PR should go in before the major release anyhow to be semver compliant.

After this PR is reviewed and merged that should finally be everything necessary before the 3.0 release.

Fixes #133 and additional problems discovered in #203 while not noticeably
regressing end-to-end performance.
Copy link
Collaborator

@keith-hall keith-hall left a comment

Choose a reason for hiding this comment

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

nice work, looks good to me!

Copy link
Collaborator

@robinst robinst left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with the styling code, but looks good!

Btw, do you want a PR with rustfmt run over all the code?

}

HighlightState {
styles: initial_styles,
styles, single_caches,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind putting this on its own line? I was a bit confused until I realized that this is a , not a :.

@trishume trishume merged commit e6f3e13 into master Sep 25, 2018
@trishume
Copy link
Owner Author

@robinst yah a rustfmt PR would be fine with me, although I'm somewhat inclined to wait for the "coming soon" 1.0 release where they say the formatting will be stable.

@trishume trishume deleted the fix-133 branch September 25, 2018 07:02
@robinst
Copy link
Collaborator

robinst commented Sep 25, 2018

Let's wait for that then, we're not in a hurry :)

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.

Color Scheme rules applied differently to Sublime
3 participants