bookalope / InDesign-CEP

Adobe CEP extension for InDesign to use the Bookalope cloud services. You can download the extension from Adobe Exchange.
http://bookalope.net/
MIT License
15 stars 6 forks source link

Adding the "Push active document" button #13

Closed nGolubeva closed 2 years ago

gregoriopellegrino commented 3 years ago

I've fixed some bugs. Now it seems ok, I receive a green status after the process, but I can't find the bookflow in Bookalope (beta).

Maybe because there is a previous upload stucked in processing: https://beta.bookalope.net/books/d7c7a29552c8483f9bbe922819136109

jenstroeger commented 3 years ago

Great, thanks @gregoriopellegrino, I’ll take a look. Also, I just checked: your Book d7c7a295 has one Bookflow e750bf12 in convert step, so all should be well (I didn’t check the result).

jenstroeger commented 3 years ago

@gregoriopellegrino I refactored/restructured your code a little without changing its functionality. It’s still working for me.

Would you mind taking a look? I left a few TODO comments and would like to know your thoughts. Happy to open individual conversations using PR comments on the code, if that helps?

gregoriopellegrino commented 3 years ago

Sure, I'll work on it next week

gregoriopellegrino commented 3 years ago

TODO doc.links.forEach(), line #250

Fine for me

gregoriopellegrino commented 3 years ago

TODO Consider using a UUID4 filename, line #272

I don't know: Extend Script doesn't seem to have a native function to generate UUID4. There are some work arounds (for example: https://stackoverflow.com/questions/56289308/need-to-generate-uuid-for-windows-system-in-extendscript/56473568#56473568).

The solution that's there now, although "homemade", seems pretty robust to me.

gregoriopellegrino commented 3 years ago

TODO Notify caller/user that exporting the image failed., line #369

Okay... The thing I find difficult is explaining to the user which image is causing problems. These are copy-pasted images in the file, so they don't have a filename. At most you can tell them the page, but if there are multiple images on the page it might not be crystal clear... What do you think?

gregoriopellegrino commented 3 years ago

TODO Ignore or handle other link statuses?, line #470

They can be ignored.

gregoriopellegrino commented 3 years ago

TODO What about text frames at index 1 and more?, line #478

This part of the script anchors the images to the text on the page (if they are not already manually anchored by the user). I haven't implemented any particular logic (such as anchoring the image to the nearest text box), which seems hyper-complex to me. For now I have used it on my files and it seems to give satisfactory results.

The idea is that each image must be anchored in order to be exported correctly to the RTF. So I anchor all the images to the first text box on the page.

We can tell the user that if they have unexpected results (images anchored in incorrect places) they can anchor them manually before running the script.

gregoriopellegrino commented 3 years ago

TODO Revisit, line #493

Repositioning the images for our purpose is not significant (we lose this information in the RTF), so lines 485-496 can be deleted.

jenstroeger commented 3 years ago

TODO doc.links.forEach(), line #250

Fine for me

Looks like forEach() isn’t supported, gives me an error. So let’s just stick with a simple loop.

TODO Consider using a UUID4 filename, line #272

[…] The solution that's there now, although "homemade", seems pretty robust to me.

Yes, fair enough. In case you’re curious, the JavaScript client has a makeUUID4() function:

https://github.com/bookalope/InDesign-CEP/blob/067bf1947753958a11024d02cd6c9a6b1688e4fe/extensions/Bookalope/js/bookalope.js#L263-L275

TODO Notify caller/user that exporting the image failed., line #369

Okay... The thing I find difficult is explaining to the user which image is causing problems. […]

Makes sense.

TODO Ignore or handle other link statuses?, line #470

They can be ignored.

Ok.

TODO What about text frames at index 1 and more?, line #478

This part of the script anchors the images to the text on the page (if they are not already manually anchored by the user). […]

Thank you for the explanation!

TODO Revisit, line #493

Repositioning the images for our purpose is not significant (we lose this information in the RTF), so lines 485-496 can be deleted.

You mean the entire content of this if statement can be removed?

https://github.com/bookalope/InDesign-CEP/blob/067bf1947753958a11024d02cd6c9a6b1688e4fe/extensions/Bookalope/jsx/bookalope.jsx#L484-L496

I changed the comments accordingly in commit https://github.com/bookalope/InDesign-CEP/pull/13/commits/b43a6e43b69c7298ecad9421c8f375aa9b4a3ffa.

gregoriopellegrino commented 3 years ago

TODO doc.links.forEach(), line #250

Fine for me

Looks like forEach() isn’t supported, gives me an error. So let’s just stick with a simple loop.

I think you should use everyItem()

TODO Revisit, line #493

Repositioning the images for our purpose is not significant (we lose this information in the RTF), so lines 485-496 can be deleted.

You mean the entire content of this if statement can be removed?

https://github.com/bookalope/InDesign-CEP/blob/067bf1947753958a11024d02cd6c9a6b1688e4fe/extensions/Bookalope/jsx/bookalope.jsx#L484-L496

I changed the comments accordingly in commit b43a6e4.

Right

jenstroeger commented 3 years ago

You mean the entire content of this if statement can be removed? https://github.com/bookalope/InDesign-CEP/blob/067bf1947753958a11024d02cd6c9a6b1688e4fe/extensions/Bookalope/jsx/bookalope.jsx#L484-L496

Right

…except the call to anchorImage() at the beginning of the if block 😉

Also, @gregoriopellegrino, do you develop & test on Windows or on Mac?

gregoriopellegrino commented 3 years ago

You mean the entire content of this if statement can be removed? https://github.com/bookalope/InDesign-CEP/blob/067bf1947753958a11024d02cd6c9a6b1688e4fe/extensions/Bookalope/jsx/bookalope.jsx#L484-L496

Right

…except the call to anchorImage() at the beginning of the if block 😉

Yes

Also, @gregoriopellegrino, do you develop & test on Windows or on Mac?

Mac

jenstroeger commented 3 years ago

…except the call to anchorImage() at the beginning of the if block 😉

Yes

Commit https://github.com/bookalope/InDesign-CEP/pull/13/commits/e5ac682f6c1b1e018094ebc1e06cf5eda4841e04.

Also, @gregoriopellegrino, do you develop & test on Windows or on Mac?

Mac

I asked because one tester on Windows gets a result.err of 3 from the extension when the extension panel tries to read the generated RTF:

https://github.com/bookalope/InDesign-CEP/blob/e5ac682f6c1b1e018094ebc1e06cf5eda4841e04/extensions/Bookalope/js/bookalope.js#L595-L599

Unfortunately, exportFile() (on the InDesign side) doesn’t document failure — does it throw an exception, perhaps? Should we set forceSave=true?

gregoriopellegrino commented 3 years ago

Maybe the problem is that it doesn't find the destination path? I would have to do some testing... But I don't have a windows...

jenstroeger commented 3 years ago

Hmm, well the path to the generated RTF is an absolute path

https://github.com/bookalope/InDesign-CEP/blob/e5ac682f6c1b1e018094ebc1e06cf5eda4841e04/extensions/Bookalope/js/bookalope.js#L720-L722

but there are details with path separators:

https://github.com/bookalope/InDesign-CEP/blob/e5ac682f6c1b1e018094ebc1e06cf5eda4841e04/extensions/Bookalope/jsx/bookalope.jsx#L45-L58

I wonder if the JavaScript panel receives some incorrect information here from the InDesign side, or if Windows access control gets in the way? Hard to tell without reproducing the problem. Let me talk with that tester, and maybe install Windows in a VM on my Mac for further testing.

jenstroeger commented 3 years ago

@gregoriopellegrino so we definitely have a problem on Windows. I just spoke with a tester, and running the script inside the old ExtendScript Toolkit the code errored here

https://github.com/bookalope/InDesign-CEP/blob/e5ac682f6c1b1e018094ebc1e06cf5eda4841e04/extensions/Bookalope/jsx/bookalope.jsx#L490-L492

One problem is the use of "/" as a path separator, which should be

https://github.com/bookalope/InDesign-CEP/blob/e5ac682f6c1b1e018094ebc1e06cf5eda4841e04/extensions/Bookalope/jsx/bookalope.jsx#L55

but also there was a problem with accessing the temporary path of the app. Tomorrow I’ll talk with another dev on Windows, but I’ll also install Windows locally to be able to reproduce this issue, and fix it.

gregoriopellegrino commented 3 years ago

Here you have the screen recording of the bug in uploading the document (it is a new document, never uploaded before): https://drive.google.com/file/d/1UOyzoC6n274zay91w6ri8wih6THRdHDN/view?usp=sharing

jenstroeger commented 3 years ago

Here you have the screen recording of the bug in uploading the document (it is a new document, never uploaded before): https://drive.google.com/file/d/1UOyzoC6n274zay91w6ri8wih6THRdHDN/view?usp=sharing

Did you build that extension yourself?

With the new workflow I renamed the endpoint and the internal Bookflow steps. That change isn’t part of this PR 😉 The old endpoints forward to the new endpoints, so that clients don’t break but this InDesign extension has one test in js/uses/BookalopeClient.js:1370 (at the bottom):

diff --git a/extensions/Bookalope/js/uses/BookalopeClient.js b/extensions/Bookalope/js/uses/BookalopeClient.js
index 46215ac..1e66cad 100644
--- a/extensions/Bookalope/js/uses/BookalopeClient.js
+++ b/extensions/Bookalope/js/uses/BookalopeClient.js
@@ -1246,7 +1246,7 @@ Bookflow.prototype.getImage = function(name) {
   var bookalope = bookflow._bookalope;

   return new Promise(function(resolve, reject) {
-    var url = bookflow.url + "/files/image";
+    var url = bookflow.url + "/upload/image";
     var params = {
       "name": name
     };
@@ -1298,7 +1298,7 @@ Bookflow.prototype.addImage = function(name, filename, file) {
     if (bookflow.step !== "convert") {
       reject(new BookalopeError("Unable to add image if Bookflow is not in 'convert' step."));
     } else {
-      var url = bookflow.url + "/files/image";
+      var url = bookflow.url + "/upload/image";
       var params = {
         "file": btoa(file),
         "filename": filename,
@@ -1329,7 +1329,7 @@ Bookflow.prototype.getDocument = function() {
   var bookalope = bookflow._bookalope;

   return new Promise(function(resolve, reject) {
-    var url = bookflow.url + "/files/document";
+    var url = bookflow.url + "/upload/document";
     var params = undefined;
     var options = {
       "responseType": "blob"
@@ -1366,10 +1366,10 @@ Bookflow.prototype.setDocument = function(filename, file, filetype, skip_analysi
   var bookalope = bookflow._bookalope;

   return new Promise(function(resolve, reject) {
-    if (bookflow.step !== "files") {
+    if (bookflow.step !== "upload") {
       reject(new BookalopeError("Unable to set document because one is already set"));
     } else {
-      var url = bookflow.url + "/files/document";
+      var url = bookflow.url + "/upload/document";
       var params = {
         "file": btoa(file),
         "filename": filename,

The

if (bookflow.step !== "files") {

line should be "upload".

gregoriopellegrino commented 3 years ago

I'm using the source from the btn-push-active-doc branch

jenstroeger commented 3 years ago

I'm using the source from the btn-push-active-doc branch

Yes, then you need to change that test

https://github.com/bookalope/InDesign-CEP/blob/4daddea57165a28163b6918d6bee743950ab7fd3/extensions/Bookalope/js/uses/BookalopeClient.js#L1369

to "upload".

jenstroeger commented 2 years ago

@gregoriopellegrino, I’ve added a few minor adjustments (see the three commits above) and also changed this code

https://github.com/bookalope/InDesign-CEP/blob/84db3cee45e973c3c9acb1f0030e1926379636a3/extensions/Bookalope/jsx/bookalope.jsx#L395-L398

to this:

    // Create a temporary copy of the document that we want to export.
    const docFile = doc.fullName;  // This is a File object.
    const tmpPath = Folder.temp;
    const tmpFileName = tmpPath + "/" + docFile.name;  // TODO Create a unique non-existent filename?
    if (!docFile.copy(tmpFileName)) {
        alert("Unable to create a temporary copy of the document");
        return JSON.stringify(false);
    }
    const tmpFile = new File(tmpFileName);
    const tmpDoc = app.open(tmpFile, false);

Waiting for feedback from the tester on Windows; my local Windows copy has expired for now.

But, there’s also a problem with the sequence of page breaks. Take a look at this screenshot:

pb

The page breaks were extracted from the RTF that this extension pushed to Bookalope… 🤔

If you need the latest extension, you can download it here.

gregoriopellegrino commented 2 years ago

Really strange. Can you share the InDesign file, or the RTF?

jenstroeger commented 2 years ago

Really strange. Can you share the InDesign file, or the RTF?

I’ll ask the tester, yes.

gregoriopellegrino commented 2 years ago

Okay, I've partially found the problem.

Page breaks 3 and 4 do not exist in the original InDesign, but are inserted by the script when it joins all the text frames.

However, this character is not a page break, but a frame break (at least that's how it is inside InDesign). I can't figure out how this separation character is exported to the RTF.

Eventually in the script I can insert a simple carriage return, so it shouldn't cause us any problems, but can you inspect the RTF first?

jenstroeger commented 2 years ago

Eventually in the script I can insert a simple carriage return, so it shouldn't cause us any problems, …

The carriage return would be ingested into Bookalope as a soft linebreak, that would be ok.

… but can you inspect the RTF first?

You mean some form of preprocessing before Bookalope works through the RTF? What would you want to check during preprocessing?

gregoriopellegrino commented 2 years ago

… but can you inspect the RTF first?

You mean some form of preprocessing before Bookalope works through the RTF? What would you want to check during preprocessing?

My question is: can you check why the import script interprets the frame break character as a page break?

jenstroeger commented 2 years ago

My question is: can you check why the import script interprets the frame break character as a page break?

When Bookalope ingests the RTF and the RTF text contains a number that is formatted with the "bookalope-page-number" character style then Bookalope ingests a page break with that number:

https://github.com/bookalope/InDesign-CEP/blob/92cd5ba64b2d21f30b2e45356e440522489d38f6/extensions/Bookalope/jsx/bookalope.jsx#L417-L427

So, here

https://github.com/bookalope/InDesign-CEP/blob/92cd5ba64b2d21f30b2e45356e440522489d38f6/extensions/Bookalope/jsx/bookalope.jsx#L450-L451

I wonder if a frame break is also included? But if so, why are the numbers out of order? Unfortunately, I’m not too familiar with InDesign’s workings 😕

gregoriopellegrino commented 2 years ago

142717735-0ba3097a-ebe6-4a26-9da8-cb06bd3a5d11

From my analysis:

So my question is, can the system distinguish the frame break from the page break? If not, I'll modify the script...

gregoriopellegrino commented 2 years ago

For the sake of completeness, the frame break character is inserted here:

https://github.com/bookalope/InDesign-CEP/blob/92cd5ba64b2d21f30b2e45356e440522489d38f6/extensions/Bookalope/jsx/bookalope.jsx#L560-L561

jenstroeger commented 2 years ago

So my question is, can the system distinguish the frame break from the page break? If not, I'll modify the script...

Hmm, good question: how does that FRAME_BREAK translate into RTF? I’ll take a look at the Bookalope side. You want the frame break to become a simple soft linebreak, correct? Or ignored?

gregoriopellegrino commented 2 years ago

Hmm, good question: how does that FRAME_BREAK translate into RTF? I’ll take a look at the Bookalope side. You want the frame break to become a simple soft linebreak, correct? Or ignored?

Turning it into a paragraph break I think would be the best thing: this character is only inserted if the frames are not linked, so it can't be that in InDesign there is a paragraph that is common to two unlinked frames.

jenstroeger commented 2 years ago

Oh 😳 I think this is my fault: the RTF is a paged document and it contains the InDesign page breaks (marked up using the "bookalope-page-number" character style) plus the actual page breaks when the RTF is rendered onto a page. Bookalope ingests both.

In the above screenshot page breaks 3, 4, 5 are the actual document page breaks, and 1, 2 are the InDesign breaks inserted into the RTF. We probably want to ignore the actual document breaks and extract only the ones that InDesign inserted?

On second thought, I think we should find a way of combining the physical page numbers and the main matter printed page numbers (these are the frame numbers)? And there also seems a slight mismatch between Hmm 🤔

To compare, here is the InDesign document with the first chapter’s spread:

idsn

Then the generated RTF opened with Word:

word

and with LibreOffice 7:

lo

and with Apple Pages:

pg

Looks like RTF is rendered more like a flow text, and if I change the page layout for the document then the RTF will reflow. Also, notice how LibreOffice and Apple Pages render the page number 1 right before “Chapter 1”.

So… if we could add all InDesign page information into the RTF then Bookalope could ignore the actual rendered page breaks and ingest only the InDesign page break information.

gregoriopellegrino commented 2 years ago

In the above screenshot page breaks 3, 4, 5 are the actual document page breaks, and 1, 2 are the InDesign breaks inserted into the RTF. We probably want to ignore the actual document breaks and extract only the ones that InDesign inserted?

Yes

So… if we could add all InDesign page information into the RTF then Bookalope could ignore the actual rendered page breaks and ingest only the InDesign page break information.

The script will insert the page information if there is a text frame on the page. If the page is empty, then there will be no number.... That's correct in my opinion, what do you think?

jenstroeger commented 2 years ago

I agree that it’s correct, though perhaps incomplete looking at it from a “digital first” viewpoint: ideally we’d like to enumerate all print pages into the page list.

Perhaps we should use the InDesign page breaks not as an absolute number but as a marker to increment? If we assume that the RTF contains forced page breaks then the markers could continue to increment based on the InDesign markers — ignoring the flow breaks that occur through rendering by the app… Hmm…

gregoriopellegrino commented 2 years ago

From an accessibility point of view these are the requirements: https://www.w3.org/TR/epub-a11y-11/#sec-page-list

Eventually blank pages can be listed as displayed here: https://github.com/w3c/epub-specs/issues/1502#issue-800500840

jenstroeger commented 2 years ago

Let me explore the RTF format some more: it has explicit \page and \pagebb control words that require a page break when rendered. That, together with the InDesign markers might be enough to reconstruct a page list* — maybe not complete but hopefully almost 🤞🏼

————— * Let’s ignore front/main/back matter numbering for now.

jenstroeger commented 2 years ago

@gregoriopellegrino, the example RTF above contains two forced page breaks (i.e. \page control words) which are recognized correctly.

If I turn off page-break ingestion (the rendered page breaks that are introduced by flowing the RTF across pages) then we get a document that contains only the InDesign page breaks (this is after a slight rearranging of paragraphs) —

rtf

So perhaps it’s a good idea to turn off page-break import for RTF, although Bookalope won’t know whether an RTF arrives from InDesign or from a user. Or ignore page breaks if InDesign page breaks exist in the document? 🤔


Regarding the code change to generating a temporary copy of the active document (mentioned above) — that change works on Windows for me. Huzza!

gregoriopellegrino commented 2 years ago

If I turn off page-break ingestion (the rendered page breaks that are introduced by flowing the RTF across pages) then we get a document that contains only the InDesign page breaks (this is after a slight rearranging of paragraphs) —

Great!

So perhaps it’s a good idea to turn off page-break import for RTF, although Bookalope won’t know whether an RTF arrives from InDesign or from a user. Or ignore page breaks if InDesign page breaks exist in the document? 🤔

The InDesign RTF can only arrive from the InDesign extension. Maybe in the creation of the bookflow we can define a flag or similar to tell the system to ignore page-breaks. There is (or was) a flag for that in the web interface, isn't it?

Regarding the code change to generating a temporary copy of the active document (mentioned above) — that change works on Windows for me. Huzza!

Hurra!

jenstroeger commented 2 years ago

The InDesign RTF can only arrive from the InDesign extension. Maybe in the creation of the bookflow we can define a flag or similar to tell the system to ignore page-breaks. There is (or was) a flag for that in the web interface, isn't it?

See commit https://github.com/bookalope/InDesign-CEP/pull/13/commits/3538aef961afa74efe9e9617e0f96b47029e7ce7: when the extension uploads the RTF for the active InDesign document, it explicitly disables pagebreaks. I had to make a few changes to the server API for this, but the feature makes a lot of sense. These changes will roll out to Beta next week.

gregoriopellegrino commented 2 years ago

Great!

gregoriopellegrino commented 2 years ago

Great!

Il giorno sab 22 gen 2022 alle 22:21 Jens Tröger @.***> ha scritto:

@.**** approved this pull request.

@nGolubeva https://github.com/nGolubeva and @gregoriopellegrino https://github.com/gregoriopellegrino, it’s time to merge, I think.

— Reply to this email directly, view it on GitHub https://github.com/bookalope/InDesign-CEP/pull/13#pullrequestreview-860242642, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGUTJJ3Q2V7ULSFWIE5OWDUXMNVTANCNFSM44DS5Y2A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

-- Gregorio Pellegrino

Effatà Editrice T 0121 353452 F 0121 353839 E @.*** W www.effata.it/ TW @g_pellegrino http://twitter.com/g_pellegrino FB facebook.com/EffataEditrice ME about.me/gregorio.pellegrino IN linkedin.com/in/gregoriopellegrino https://it.linkedin.com/in/gregoriopellegrino