[api-minor][Editor] When switching to editing mode, redraw pages containing editable annotations (bug 1883884) #18134
Conversation
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/848b5ec2a6372b9/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/faccdf624756a1a/output.txt |
Snuffleupagus
changed the title
calixteman
force-pushed
the
hide_annotations
branch
from
c30d0f8
to
47ab32a
Compare
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/848b5ec2a6372b9/output.txt Total script time: 28.31 mins
Image differences available at: http://54.241.84.105:8877/848b5ec2a6372b9/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/faccdf624756a1a/output.txt Total script time: 43.23 mins
Image differences available at: http://54.193.163.58:8877/faccdf624756a1a/reftest-analyzer.html#web=eq.log |
calixteman
force-pushed
the
hide_annotations
branch
from
47ab32a
to
c19136e
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.241.84.105:8877/f5671dedc2e3b62/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.193.163.58:8877/e5817c6d4a755b2/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/f5671dedc2e3b62/output.txt Total script time: 28.63 mins
Image differences available at: http://54.241.84.105:8877/f5671dedc2e3b62/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/e5817c6d4a755b2/output.txt Total script time: 43.12 mins
Image differences available at: http://54.193.163.58:8877/e5817c6d4a755b2/reftest-analyzer.html#web=eq.log |
|
@calixteman Given the size of the patch would it be an idea, to make reviewing easier by reducing the number of changes in this PR, to extract the test refactorings into a separate PR? I'd be happy to review that part and we can then land it first so @Snuffleupagus can check the remainder of the PR later. Those test refactorings (introduction of all |
calixteman
force-pushed
the
hide_annotations
branch
2 times, most recently
from
901c6b9
to
2b5eac1
Compare
Yep, it has been done in #18165. |
There was a problem hiding this comment.
Sorry to say, but I'm finding this somewhat difficult to wrap my head around w.r.t. both the API and viewer (which is why I've not done a proper review yet). Any chance that it could be somehow split into smaller parts and/or simplified somehow?
Given that re-rendering of a page can in some cases be slow, how does this perform when switching to/from editing-mode?
Also, I really don't at all understand how the API works since it seems that you're "mixing" page-specific and global state on the worker-thread; i.e. the EndEditingSession functionality.
Can't this lead to intermittent issues if that changes while a page is parsing/rendering?
To me it seems that the editing-state needs to be constant for a getOperatorList invocation.
There's also the way that the intentState is now handled in the API, adding complexity in the render-method, which suggests to me that the cacheKey isn't correctly computed in the getRenderingIntent-helper.
calixteman
force-pushed
the
hide_annotations
branch
2 times, most recently
from
bf93f39
to
d18df8f
Compare
If a page with an editable annotation is slow to render then switching to editing mode will be slower.
That said if you've any good ideas, as usual please share. |
marco-c
changed the title
|
This is the first step for bug 1883884. |
calixteman
force-pushed
the
hide_annotations
branch
from
d18df8f
to
5b234c9
Compare
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/9cab3b8d503873c/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/c13dc56b765177d/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/9cab3b8d503873c/output.txt Total script time: 7.77 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/c13dc56b765177d/output.txt Total script time: 18.41 mins
|
There was a problem hiding this comment.
Edit: Given that we have a pending release (hopefully soon), we should probably not land this PR just prior to that.
I've not tested this very much, so I'm counting on you having tested this thoroughly during development :-)
Also, the code feels a lot more approachable now compared to the initial version; thanks for that!
r=me, with a couple of final comments addressed; thank you!
calixteman
force-pushed
the
hide_annotations
branch
from
f9d63b4
to
dee4005
Compare
|
Moreover, since this will probably land as the first |
calixteman
force-pushed
the
hide_annotations
branch
from
dee4005
to
31472a8
Compare
|
/botio integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/c5e4786d5ec3077/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/77b2cda57cb3a6a/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/77b2cda57cb3a6a/output.txt Total script time: 7.87 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/c5e4786d5ec3077/output.txt Total script time: 18.81 mins
|
| this.#onPageRenderedCallback = ({ pageNumber }) => { | ||
| idsToRefresh.delete(pageNumber); | ||
| if (idsToRefresh.size === 0) { | ||
| eventBus._off("pagerendered", this.#onPageRenderedCallback); | ||
| this.#onPageRenderedCallback = null; | ||
| this.#switchAnnotationEditorModeTimeoutId = setTimeout(updater, 0); | ||
| } | ||
| }; | ||
| eventBus._on("pagerendered", this.#onPageRenderedCallback, { signal }); |
There was a problem hiding this comment.
What if the following order of events occur:
- The user switches from editing mode NONE to e.g. INK.
- The pages are complex enough that re-rendering takes a little while.
- The user immediately switches from INK to e.g. STAMP, before rendering has finished.
It seems, unless I'm misunderstanding things, that in such a case there's nothing that'd remove/stop the previously registered "pagerendered" event listener and that one would thus remain forever.
Hence, don't we need to essentially run https://github.com/mozilla/pdf.js/pull/18134/files#diff-4238fc7e96d72f4095859850f3d24a9298ec2dc8bd96437214a02dccbd48385cR2299-R2306 before registering this event listener here?
There was a problem hiding this comment.
I think this case is already handled thanks to:
https://github.com/mozilla/pdf.js/pull/18134/files#diff-4238fc7e96d72f4095859850f3d24a9298ec2dc8bd96437214a02dccbd48385cR2315-R2317
We only re-render when we switch to/from NONE. Here in your case we're switch from INK to STAMP.
There was a problem hiding this comment.
@calixteman Are you sure, since in the case I'm outlining rendering has not finished before the INK to STAMP switch which means that we've still not updated the this.#annotationEditorMode-field yet and the if you're referencing will thus be true?
There was a problem hiding this comment.
Yep you're right: the user should first double click on a freetext and then while waiting double click on a ink... and then we're in the case you mentioned: I'm fixing.
…aining editable annotations Right now, editable annotations are using their own canvas when they're drawn, but it induces several issues: - if the annotation has to be composed with the page then the canvas must be correctly composed with its parent. That means we should move the canvas under canvasWrapper and we should extract composing info from the drawing instructions... Currently it's the case with highlight annotations. - we use some extra memory for those canvas even if the user will never edit them, which the case for example when opening a pdf in Fenix. So with this patch, all the editable annotations are drawn on the canvas. When the user switches to editing mode, then the pages with some editable annotations are redrawn but without them: they'll be replaced by their counterpart in the annotation editor layer.
calixteman
force-pushed
the
hide_annotations
branch
from
31472a8
to
64635f3
Compare
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/c8812a133dff9a8/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/80dac6aea917a1d/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/c8812a133dff9a8/output.txt Total script time: 7.93 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/80dac6aea917a1d/output.txt Total script time: 18.40 mins
|
|
/botio-windows integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/d465e80d2b9e083/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/d465e80d2b9e083/output.txt Total script time: 18.60 mins
|
Right now, editable annotations are using their own canvas when they're drawn, but it induces several issues:
So with this patch, all the editable annotations are drawn on the canvas. When the user switches to editing mode, then the pages with some editable annotations are redrawn but without them: they'll be replaced by their counterpart in the annotation editor layer.