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

DocumentSelection#markers collection should be updated only for observed markers groups #8484

Merged
merged 9 commits into from
Nov 19, 2020

Conversation

niegowski
Copy link
Contributor

Suggested merge commit message (convention)

Other (engine): The DocumentSelection#markers collection is updated only for observed markers groups.

BREAKING CHANGE (engine): The DocumentSelection#markers collection is no longer enabled by default. DocumentSelection#observeMarkersGroup() must be used to register markers group to be observed.


Additional information

This is performance optimization.

@scofalik
Copy link
Contributor

scofalik commented Nov 19, 2020

I left some review comments, minor stuff.

I have one doubt, though. This is connected with the fact that we never forced any structure for markers and something like "marker group" was not really specified (outside of API docs). It just appeared that in 99% of cases there are multiple markers of the same type and it makes sense to handle all of them in the same way. Hence, we have comment: and suggestion: and "marker groups" were born and we try to describe what's "marker group" where possible. I am not sure if this is clear to our users.

The problem is that it is still possible to create a marker with a name that does not have :. We should handle them as well. For example, someone could implement find and replace and use marker named searchResult to highlight the current result. So, in places where we check whether we should observe given marker, we should probably check whether prefix matches and if not, whether full name matches. And include this case in API docs for observeMarkersGroup. Maybe we should also rename that method to observeMarkers or observeMarkersNames or something?

@niegowski niegowski requested a review from scofalik November 19, 2020 13:36
@scofalik scofalik merged commit 814b363 into master Nov 19, 2020
@scofalik scofalik deleted the cf/3644 branch November 19, 2020 13:52
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