[Editor] A new CurrentPointers class to store current pointers used by the editor #20213
Conversation
src/display/editor/tools.js
Outdated
| return true; | ||
| } | ||
|
|
||
| CurrentPointer.#pointerIds?.delete(pointerId); |
There was a problem hiding this comment.
It's strange to delete something in a function which is supposed to be a simple checker.
The delete is because originally the pointer was up or cancel which means that this pointer is no more active, consequently I'm not sure it makes sense to use this function here:
https://github.com/mozilla/pdf.js/pull/20213/files#diff-02919db67aef2f378464ba4c97e5f59e1e114498a147113025cd4163287f25f4R802
There was a problem hiding this comment.
I've changed the function name to isSamePointerIdOrRemove. And I also created a isSamePointerId function for the last check you were referencing to.
src/display/editor/draw.js
Outdated
| // For example, we started with a pen and the user | ||
| // is now using a finger. | ||
| return; | ||
| } | ||
|
|
||
| // For example, the user is using a second finger. | ||
| (DrawingEditor.#currentPointerIds ||= new Set()).add(e.pointerId); | ||
| CurrentPointer.initializeAndAddPointerId(e.pointerId); |
There was a problem hiding this comment.
Ideally, the comment above this line should be copied, pasted and adapted in the new class you introduced.
There was a problem hiding this comment.
I've modified the comments and put some of them in the new class.
src/display/editor/tools.js
Outdated
| @@ -42,6 +42,78 @@ function bindEvents(obj, element, names) { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Class to store current pointer used by the editor. | |||
There was a problem hiding this comment.
It isn't exactly correct: the idea is more about handling multiple pointers (fingers, pen, mouse, ...)
There was a problem hiding this comment.
I've updated the comment, and change the name of the class to CurrentPointers.
src/display/editor/draw.js
Outdated
| DrawingEditor.#currentPointerType && | ||
| DrawingEditor.#currentPointerType !== pointerType | ||
| ) { | ||
| if (CurrentPointer.isInitializedAndDifferentPointerType(pointerType)) { |
There was a problem hiding this comment.
The comment above should be updated: the variable currentPointerType isn't defined in this file.
Probably a part of it should be moved in the new class you created.
src/display/editor/tools.js
Outdated
| return false; | ||
| } | ||
|
|
||
| static isPointerType() { |
There was a problem hiding this comment.
I'm not an expert in english, but the name looks strange for me... I'd have said hasPointerType.
I just read again https://github.com/mozilla/pdf.js/pull/20213/files#diff-02919db67aef2f378464ba4c97e5f59e1e114498a147113025cd4163287f25f4R670 and I've the feeling that this class should be initialized with this the pointer type and the static methods in this class shouldn't be static. An instance could be created in startDrawing and set in a static member of DrawingEditor.
There was a problem hiding this comment.
I've changed the name.
For the logic, I feel that we need a static class outside of the editor if we want all editors to share the same pointer type. It's really important for pdf with multiple pages as each page has its own editor, and you don't want to reinitialized the pointer type on each page. Furthermore, when you use this mode for the first time, you want to initialize the pointer type, and the only way to do that is to check if the pointer type is null. Also, you will have to put the class with a getter/setter if you want to be able to clear it outside of the DrawingEditor (it's the only way to check if the mode is changed to the best of my knowledge, or perhaps through disableEditing).
src/display/editor/tools.js
Outdated
| @@ -1767,13 +1839,16 @@ class AnnotationEditorUIManager { | |||
| * edit mode. | |||
| * @param {boolean} [editComment] - true if the mode change is due to a | |||
| * comment edit. | |||
| * @param {boolean} [isFromEvent] - true if the mode change is due to an event | |||
There was a problem hiding this comment.
Sorry but this change should be in an other patch.
I've the feeling you're fixing a bug and if it's the case we should have an integration test for it.
There was a problem hiding this comment.
I'm creating a new PR #20226. This PR should be merged before.
|
Please update the title of the PR to precise its goal in few words. |
legraina
force-pushed
the
pointer-type
branch
from
2787e48 to
7e3b3ae
Compare
legraina
changed the title
legraina
changed the title
|
Hi @calixteman, everything looks good ? Or anything else to change/improve ? Thanks |
I have created a different PR as requested.
Move current pointer field of DrawingEditor to CurrentPointer class in tools.js. Also, only reset pointer type when user select a new mode.