Improve code structure of the annotation code #6699
Conversation
timvandermeij
force-pushed
the
annotation-refactoring
branch
from
1a37553
to
71ef77e
Compare
|
I'm wondering if we shouldn't instead bring back the |
|
I agree that that appears to be the right approach to tackle the double bookkeeping problem. Is it okay if I do that in a follow-up PR and keep this PR as is for the minor changes in structure (as I think it helps to keep that larger refactoring separate from these minor code structure updates)? |
That is probably best, so that's fine by me! |
| }, reject); | ||
| }, function(reason) { | ||
| annotationsReadyCapability.reject(reason); | ||
| }); |
There was a problem hiding this comment.
Would it be possible to simplify this even further, by removing the PromiseCapability all together? E.g. like this:
return Promise.all(annotationPromises).then(function(datas) {
opList.addOp(OPS.beginAnnotations, []);
for (var i = 0, n = datas.length; i < n; ++i) {
var annotOpList = datas[i];
opList.addOpList(annotOpList);
}
opList.addOp(OPS.endAnnotations, []);
});
There was a problem hiding this comment.
Nice suggestion! I have done this in the new commit, also with more descriptive variable names (as datas was a bit undescriptive in my opinion).
timvandermeij
force-pushed
the
annotation-refactoring
branch
4 times, most recently
from
3559c6b
to
c89dd85
Compare
timvandermeij
changed the title
This patch improves the code structure of the annotation code. - Create the annotation border style object in the `setBorderStyle` method instead of in the constructor. The behavior is the same as the `setBorderStyle` method is always called, thus a border style object is still always available. - Put all data object manipulation lines in one block in the constructor. This improves readability and maintainability as it is more visible which properties are exposed. - Simplify `appendToOperatorList` by removing the promise capability and removing an unused parameter. - Remove some unnecessary newlines/spaces.
timvandermeij
force-pushed
the
annotation-refactoring
branch
from
c89dd85
to
8b79bec
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/dc336f224820b08/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/dc336f224820b08/output.txt Total script time: 0.80 mins Published |
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/25770b516db06ce/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/6f31cb7a594675d/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/6f31cb7a594675d/output.txt Total script time: 18.75 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/25770b516db06ce/output.txt Total script time: 19.94 mins
|
Improve code structure of the annotation code
|
Looks good, thank you for the patch. |
|
Thank you for the helpful review comments for this one! |
This patch improves the code structure of the annotation code.
setBorderStylemethod instead of in the constructor. The behavior is the same as thesetBorderStylemethod is always called, thus a border style object is still always available.appendToOperatorListby removing the promise capability and removing an unused parameter.I have tested this patch with a set of 21 PDF files with different types of annotations and the behavior is the same as before this patch.