-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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 support for printing newly added Ink annotations #15047
Conversation
@Snuffleupagus would you have time to have a look on this patch ? |
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.
r=me, with comments addressed and passing tests; thank you!
Sorry about the delay, this one slipped my mind given the amount of recent PRs and the fact that my time isn't unlimited :-)
src/core/annotation.js
Outdated
@@ -294,6 +294,27 @@ class AnnotationFactory { | |||
dependencies, | |||
}; | |||
} | |||
|
|||
static async createNewAnnotationForPrinting(evaluator, task, annotations) { |
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.
The method just above is called saveNewAnnotations
, hence the following (and shorter) name seems more appropriate here:
static async createNewAnnotationForPrinting(evaluator, task, annotations) { | |
static async printNewAnnotations(evaluator, task, annotations) { |
src/core/annotation.js
Outdated
@@ -3509,6 +3546,16 @@ class InkAnnotation extends MarkupAnnotation { | |||
|
|||
results.push({ ref: inkRef, data: buffer.join("") }); | |||
} | |||
|
|||
static async createAnnotationForPrinting(annotation, xref) { |
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.
Similar to existing naming, how about:
static async createAnnotationForPrinting(annotation, xref) { | |
static async createNewPrintAnnotation(annotation, xref) { |
src/core/core_utils.js
Outdated
@@ -548,6 +549,27 @@ function numberToString(value) { | |||
return value.toFixed(2); | |||
} | |||
|
|||
function getNewlyAddedAnnotations(annotationStorage) { |
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'd prefer the following instead, since it tells you what sort of data is returned (and it's slightly shorter):
function getNewlyAddedAnnotations(annotationStorage) { | |
function getNewAnnotationsMap(annotationStorage) { |
src/core/document.js
Outdated
]).then(function ([pageOpList, annotations, newParsedAnnotations]) { | ||
if (Array.isArray(newParsedAnnotations)) { | ||
annotations = annotations.concat(newParsedAnnotations); |
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.
Maybe slightly shorter names (again), and the isArray
-check might be overkill (since it's either null
or an Array):
]).then(function ([pageOpList, annotations, newParsedAnnotations]) { | |
if (Array.isArray(newParsedAnnotations)) { | |
annotations = annotations.concat(newParsedAnnotations); | |
]).then(function ([pageOpList, annotations, newAnnotations]) { | |
if (newAnnotations) { | |
annotations = annotations.concat(newAnnotations); |
f4332b4
to
65fa567
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/ea895476e2873f3/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/caca2b960a38060/output.txt |
65fa567
to
ad5dad4
Compare
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/ea895476e2873f3/output.txt Total script time: 26.19 mins
Image differences available at: http://54.241.84.105:8877/ea895476e2873f3/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/caca2b960a38060/output.txt Total script time: 28.28 mins
|
ad5dad4
to
f27c8c4
Compare
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @calixteman received. Current queue size: 1 Live output at: http://54.241.84.105:8877/eaefba805b682d3/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @calixteman received. Current queue size: 2 Live output at: http://54.193.163.58:8877/1a72311e884deb1/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/eaefba805b682d3/output.txt Total script time: 22.86 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/1a72311e884deb1/output.txt Total script time: 22.13 mins
|
No description provided.