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 Ambiguous Highlights #275

Merged
merged 28 commits into from
Nov 19, 2024

Conversation

thecoolwinter
Copy link
Collaborator

@thecoolwinter thecoolwinter commented Nov 17, 2024

Description

Fixes some bad syntax highlighting caused by overlapping captures returned from tree-sitter. Previously the last value returned took precedence, but with the new highlighting system that's not the case. This filters highlights on duplicate ranges, and prioritizes the best capture for any range so this is no longer dependent on the highlighting system's semantics.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

Disambiguated highlights:

Screenshot 2024-11-17 at 2 50 28 PM

Ambiguous highlights:

Screenshot 2024-11-17 at 2 52 04 PM

@thecoolwinter thecoolwinter added the bug Something isn't working label Nov 17, 2024
Copy link
Collaborator

@austincondiff austincondiff 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, and very well documented! I found a few potential issues. Let me know your thoughts.

Sources/CodeEditSourceEditor/Enums/CaptureModifier.swift Outdated Show resolved Hide resolved
Sources/CodeEditSourceEditor/Enums/CaptureModifier.swift Outdated Show resolved Hide resolved
@thecoolwinter
Copy link
Collaborator Author

Nice work, and very well documented! I found a few potential issues. Let me know your thoughts.

@austincondiff most of these changes are from #273 so I think we need to wait to review this until that is merged so the actual changes are visible here. Sorry I made it convoluted. The things you commented on are from that PR

@thecoolwinter thecoolwinter marked this pull request as draft November 17, 2024 21:19
@tom-ludwig
Copy link
Member

Would it be better to set the #273 branch as the target and merge into it instead?

@thecoolwinter
Copy link
Collaborator Author

thecoolwinter commented Nov 18, 2024

I think actually I'll just remake this PR entirely once #273 is merged. I'd rather not muck up that PR anymore since it's so large. Leaving this open and as a draft for now just to bookmark it.

@thecoolwinter thecoolwinter marked this pull request as ready for review November 18, 2024 20:41
@thecoolwinter
Copy link
Collaborator Author

Okay the changes shown here now reflect the actual diff. I'm not too worried about the messy commit history since we squash PRs anyways. Ready for review now!

Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

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

Looks good!

@thecoolwinter thecoolwinter merged commit 696a7f1 into CodeEditApp:main Nov 19, 2024
2 checks passed
@thecoolwinter thecoolwinter deleted the fix/ambiguous-highlights branch November 19, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐞 tree-sitter Returns Overlapping Highlights
3 participants