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

More work on highlight IDs #636

Merged
merged 11 commits into from Jul 4, 2023

Conversation

mindofmatthew
Copy link
Contributor

@felixroos, I've got this working! I split up the CodeMirror logic into two pieces of state: mininotationLabels, a RangeSet of all the mininotation symbols in the document that stores their ids, and then visibleMininotationLabels, a map of ids to their associated haps for a given query. The actual set of decorations is derived from these two pieces of state whenever either of them changes. This doesn't strictly change any of the behavior of your last version of the state field, but I think it makes each piece of the logic a little simpler to reason about. I can add comments as would be useful.

To test out my own understanding of the system, I went back and hooked up colored outlines, and added a feature where the opacity of the outline indicates progress through the hap. I feel like this has been discussed before, but also happy to split this piece out into a separate feature branch if you all want to tweak implementation more.

@felixroos
Copy link
Collaborator

amazing! Maybe the opacity stuff should indeed be a separate PR, as I'm not sure about the performance implications.. we'll have to do some testing to find that out. I guess the only problem left to solve is the failing tests, it seems the transpiler changes also changed some hap orderings.. I can look into it

@mindofmatthew
Copy link
Contributor Author

The opacity can definitely be a flag, extending the disable-highlights functionality. I'll play with that and do some performance tests in a separate PR.

Maybe related to tests, I noticed that the set of highlights returned from getLeafLocations seemed to sometime have duplicate locations. Does this address that too?

- dedupe flash / highlighting logic
- codemirror logic now lives only in codemirror package
- remove old highlighting logic
- use codemirror package in react package
- cleanup CodeMirror6.jsx
- pull setMiniLocations into useHighlighting
- migrate MiniRepl, nano-repl + Repl to new highlighting
@felixroos
Copy link
Collaborator

felixroos commented Jul 4, 2023

The opacity can definitely be a flag, extending the disable-highlights functionality. I'll play with that and do some performance tests in a separate PR.

👍

Maybe related to tests, I noticed that the set of highlights returned from getLeafLocations seemed to sometime have duplicate locations. Does this address that too?

the tests were only failing because the order got messed up, now that #637 is finally fixed, it now works! haven't seen doubled locations yet, I wonder how this could happen...

I've now refactored a bunch of things, so everything is nice and tidy. The old logic is gone now :)
I think that should be it, I'll merge it now and test again on #634

@felixroos felixroos merged commit c82f7bd into tidalcycles:highlight-ids Jul 4, 2023
1 check passed
@felixroos felixroos mentioned this pull request Jul 4, 2023
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.

None yet

2 participants