-
Notifications
You must be signed in to change notification settings - Fork 146
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
Add cancellation to HtmlCopy GetAllClassificationSpans #243
Conversation
Hmm I fear if clicking Cancel does nothing then cancellation token might not be checked too often: Still I think it’s a good change to make but we need to keep thinking |
using (var waitContext = WaitHelper.Wait(_waitIndicator, "HTML Copy", "Formatting document for copying")) | ||
try | ||
{ | ||
using (var waitContext = WaitHelper.Wait(_waitIndicator, "HTML Copy", "Formatting document for copying")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know which component doesn't react to this cancellation token? I think I've seen a comment saying Cancel button doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's what worries me too. Eventually it's the taggers and the aggregate tagger and GetTags().
Another option is to drop AccurateTagger support entirely here. If a regular copy is working fine for them, this should too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @gundermanc has UI delay bug related to this, where LSP client makes remote call to get this information.
I'd suggest dropping IAccurateTagger altogether. The impact is that in this case you would get what you see, which should be good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, we build PPT from an internal repo for legacy reasons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote it to just delete IAccurateTagger
It is causing unacceptably long delays
1abfe8e
to
3c5341c
Compare
Fixes #242
Fixes #239