fix(missing-annotations): Fix annotations on zoom issue #701
Conversation
| managers.forEach(manager => { | ||
| if (!manager.exists(pageEl)) { | ||
| destroyManagers = true; | ||
| } |
There was a problem hiding this comment.
What if the third manager in the list doesn't exist but there are 6 managers. That means the first 2 won't get destroyed. Is that the behavior we want?
There was a problem hiding this comment.
Ah my mistake, updated. Good catch.
src/document/DocumentAnnotator.ts
Outdated
| manager.destroy(); | ||
| }); | ||
| managers.clear(); | ||
| destroyManagers = false; |
There was a problem hiding this comment.
What happens if we don't reset the destroyManagers values here?
There was a problem hiding this comment.
Should be the same result, but I don't see any downsides to resetting the value since it should not be true anymore.
There was a problem hiding this comment.
Yea it should have no effects but it's not needed though because the value won't be used again after that. Resetting it makes it seem like that value is relevant even after we clear the managers.
Due to changes in PDFjs v3.6.172, annotations would dissapear upon zooming in/out on the pdf. This is because the page elements that our annotation managers were referencing would be removed when zooming in/out. The textLayer pdfJS uses would not be removed however, which would make one of our managers not get cleaned up. In order to reinstantiate our managers, we need all managers to be cleaned up. Because of our current logic for adding managers, we need to make sure every manager is cleaned up when at least one manager is cleaned up. Which makes sense since the annotation managers are coupled together and cannot function correctly without the others.
Annotation Drawing mode
Annotation Highlight mode
