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

Ignore LineDecoration order when comparing #108379

Merged
merged 3 commits into from Nov 20, 2020
Merged

Ignore LineDecoration order when comparing #108379

merged 3 commits into from Nov 20, 2020

Conversation

KapitanOczywisty
Copy link
Contributor

@KapitanOczywisty KapitanOczywisty commented Oct 9, 2020

This PR fixes #80666 . I was really hoping that this will be fixed for September update, but no-surprisingly, was pushed back.

Disclaimer

Be aware that I was unable to test this in vscode build itself, because It's very problematic to get build running. I've tested similar code in debugger's conditional breakpoint and this code in simplified test scenario. But yeah, real-life test is necessary.

The problem

LineDecorations often are changing order when there is after decorator, which messes up drag&drop. With Error Lens extension this is annoying as... something very annoying. More info #80666 (comment)

The solution

There are 2 approach for this: first to sort every LineDecoration array, but I've settled for second: make comparison order-proof. For starters changes are done in one place, but also future changes will less likely break this.
I've tried to optimize this as good as I could, but maybe this may be improved further.

The elephant

Isn't order important somehow?
No. Which style is used should be determinated by order of CSS rules, so even in different order, decorators should show-up in the same way. If this wasn't the case, this would cause weird style flashing.

The performance

For arr.length <= 1 this could be even faster. For arrays in correct order above 2 elements, this will have overhead of additional array (bSeen), but still should be the same number of comparisons (w/ _equals). On the other hand usually there isn't that many visible elements with 2+ decorators.

However for arrays with different order this will also prevent re-rendering these lines - which happens more often than you may think.

@alexdima This is probably for you to take a look. Or maybe @rebornix ?

@KapitanOczywisty
Copy link
Contributor Author

KapitanOczywisty commented Oct 23, 2020

@alexdima @rebornix can you take a look at this before end of current iteration? And tests are failing because of commits on master...

@alexdima alexdima added this to the November 2020 milestone Nov 20, 2020
@alexdima
Copy link
Member

Thank you! I decided to sort the line decorations in the end.

@alexdima alexdima merged commit cbeaf4f into microsoft:master Nov 20, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary code and decoration with :after prevents dnd
2 participants