-
Notifications
You must be signed in to change notification settings - Fork 10k
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
[Editor] Add a new base class to allow to add a drawing in the SVG layer. #19093
base: master
Are you sure you want to change the base?
Conversation
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/f45378cd6117b14/output.txt |
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/324b95df703677d/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.241.84.105:8877/37aece65741ede8/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/f45378cd6117b14/output.txt Total script time: 0.98 mins Published |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/37aece65741ede8/output.txt Total script time: 34.68 mins
Image differences available at: http://54.241.84.105:8877/37aece65741ede8/reftest-analyzer.html#web=eq.log |
3f0bc0e
to
1e03740
Compare
…yer. This patch makes a clear separation between the way to draw and the editing stuff. It adds a class DrawEditor which should be extended in order to create new drawing tools. As an example, the ink tool has been rewritten in order to use it.
1e03740
to
5ccc04e
Compare
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/324b95df703677d/output.txt Total script time: 50.74 mins
|
/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/e8b69f2a0f9b217/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/b80a2a0ba469254/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/e8b69f2a0f9b217/output.txt Total script time: 8.86 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/b80a2a0ba469254/output.txt Total script time: 19.12 mins
|
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.
A couple of quick observations based on comparing master
against the preview:
-
The Ink-editor can now be resized not just in the corners, but also along the edges; see the red circles in screen-shots below.
Was this an intentional change, since the PR description (and commit message) doesn't mention it? -
The focus outline behaves subtly different, since it now "overlays" and hides the very edges of the drawing; note e.g. the red rectangles in the screen-shots below.
Again, it's not clear to me if this was intentionally or accidentally changed?
buffer.push(`${curve} c`); | ||
for (const outline of paths.lines) { | ||
for (let i = 0, ii = outline.length; i < ii; i += 6) { | ||
if (isNaN(outline[i]) || outline[i] === null) { |
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.
Can we have the editor-code use either NaN
or null
, so that we don't have to support both formats here?
@@ -300,7 +300,7 @@ | |||
</div> | |||
<div class="editorParamsSetter"> | |||
<label for="editorInkOpacity" class="editorParamsLabel" data-l10n-id="pdfjs-editor-ink-opacity-input">Opacity</label> | |||
<input type="range" id="editorInkOpacity" class="editorParamsSlider" value="100" min="1" max="100" step="1" tabindex="0"> | |||
<input type="range" id="editorInkOpacity" class="editorParamsSlider" value="1" min="0.1" max="1" step="0.1" tabindex="0"> |
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.
This gives a lot less control over the opacity, from 100 to 10 possible values, was that intended?
(For comparison, the editorInkThickness
has 20 possible values.)
@@ -98,46 +97,6 @@ describe("Ink Editor", () => { | |||
}) | |||
); | |||
}); | |||
|
|||
it("must draw, undo/redo and check that the editor don't move", async () => { |
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.
Why is this test removed?
This patch makes a clear separation between the way to draw and the editing stuff. It adds a class DrawEditor which should be extended in order to create new drawing tools.
As an example, the ink tool has been rewritten in order to use it.