Adds PDFComprezzor and optimization upload #81

henrixapp · 2020-09-21T18:54:47Z

No description provided.

fosterfarrell9

That is is really a nice addition! The Optimization does somehow not yet work in my docker_development setup. I tried it with the pdf I uploaded to basecamp. The console logs look like this:

File {name: "some_name.pdf", lastModified: 1600688288000, lastModifiedDate: Mon Sep 21 2020 13:38:08 GMT+0200 (Mitteleuropäische Sommerzeit), webkitRelativePath: "", size: 7773739, …}
worker.js:29 MessageEvent {isTrusted: true, data: {…}, origin: "", lastEventId: "", source: null, …}
worker.js:30 DedicatedWorkerGlobalScope {name: "", onmessage: null, onmessageerror: null, postMessage: ƒ, close: ƒ, …}
worker.js:1 Uncaught (in promise) CompileError: WebAssembly.instantiateStreaming(): function size > maximum function size: 272039936 < 7654321 @+27540

fosterfarrell9 · 2020-09-22T16:47:02Z

app/assets/javascripts/upload.coffee

+          $('#upload-userManuscript').val("")
+          $('input[type="submit"]').prop('disabled',false)
+          $('#submission_detach_user_manuscript').val('false')
+          $('#userManuscript-uploadButton-call').text("Upload sucessfull")


I think it would be good if we could localize this via I18n. One way would be to put the localized text in a data-attribute in the rails partial and retrieve the text here from that data-attribute.

Ok. Forgot them and did not know, how to solve this. Maybe using a gem like i18n-js would make this cleaner.

That is also a possibility. I used that for a while but then removed it as I was not happy with the behaviour in regards to sprockets (see known issues). Also, for some reasons, a large file of (internal) rails translations (~1 MB) ended up in each response cycle. But that is probably only due to configuration errors on my side, so do not let that stop you if you prefer to use it.

Ok for now I switched to data-tr-* attributes.

fosterfarrell9 · 2020-09-22T16:47:44Z

app/assets/javascripts/upload.coffee

-
-  uppy
-
+      alert("You must consent.")


Localization as above.

fosterfarrell9 · 2020-09-22T16:48:10Z

app/assets/javascripts/upload.coffee

+          $('#userManuscript-uploadButton-call').text("Upload sucessfull")
+          console.log(xhr.responseText)
+        else
+          alert "Fehler beim Upload " + xhr.responseText


Localization as above.

fosterfarrell9 · 2020-09-22T16:49:01Z

app/assets/javascripts/upload.coffee

-      uppy.info('Falscher MIME-Typ:' + data.metadata.mime_type, 'error', 5000)
-      uppy.reset()
+  result = undefined
+  progressOptimize=0


Add some spaces.

fosterfarrell9 · 2020-09-22T16:50:39Z

app/assets/javascripts/upload.coffee

+  progressOptimize=0
+  $('#userManuscript-status').hide()
+  uploadButton = $('#userManuscript-uploadButton-btn')
+  $('#userManuscript-uploadButton-call').on 'click', (e)=>


Add space after (e) . Not sure if we need the fat arrow here (linter says no),

fosterfarrell9 · 2020-09-22T17:25:21Z

app/views/submissions/_form.html.erb

+			<div id="userManuscript-optimize-log" style="display:none"></div>
+			</div>
+			</div>
+			<div class="col-12"><input type="checkbox" id="file-permission-checkbox"/> <%= t('submission.assure_third_party') %></div>


I would add some vertical spacing for the div like my-1.

fosterfarrell9 · 2020-09-22T17:29:46Z

app/views/submissions/_form.html.erb

-	    <div class="col-12" id="userManuscript-informer">
-  	  </div>
+	    <div class="col-12" id="userManuscript-status">
+			<div class="col-12" id="file-size-correct" style="display:none"><i class="fas fa-check"></i> <%=t('submission.correct_file_size')%></div>


I guess I am throwing stones in the glass house here as I often neglect this in my own code, but it would be nice if these display: none; styles for the elements in the next four lines would be put in submissions.scss (which should then be imported in application.scss

fosterfarrell9 · 2020-09-22T17:34:25Z

app/views/submissions/_form.html.erb

+			<div class="col-12" id="file-size-too-big" style="display:none"> <i class="fas fa-exclamation-triangle"></i> <%=t('submission.file_size_too_big')%></div>
+			<div class="col-12" id="file-size-way-too-big" style="display:none"><i class="fas fa-bomb"></i> <%=t('submission.file_size_way_too_big')%></div>
+			<div class="col-12" id="file-optimize" style="display:none">
+			<small><%=raw t("submission.optimization_help")%><%= helpdesk(t('submission.optimization_info'),true)%></small>


I think the railsier way to do this would be <%= t('submission.optimization_help_html') %> instead of <%=raw t("submission.optimization_help")%> and then rename submission.optimization_help to submission.optimization_help_html in the locale files (see here).

fosterfarrell9 · 2020-09-22T17:38:23Z

config/locales/de.yml

+      Ich versichere, dass durch die Datei keine Rechte Dritter verletzt werden.
+    removal_notice: >
+      <b>Hinweis:</b> Dateien werden nach Ablauf des Semesters gelöscht.
+      Wir übernehmen keine Haftung für mögliche Datenverluste.


See the remarks about missing translations in the review for the submissions form partial.

fosterfarrell9 · 2020-09-22T17:39:36Z

public/pdfcomprezzor/worker.js

+    });
+    console.log(e);
+    console.log(self);
+}, false);


I cannot say much about this due to lack of competence.

tynsh · 2020-09-22T17:46:11Z

Hey, could you explain, how to build the web assembly binary?

henrixapp · 2020-09-22T19:07:39Z

Hey, could you explain, how to build the web assembly binary?

You can find it in its Readme:
https://github.com/henrixapp/pdfcomprezzor

The most important step is that you comment out /deactivate the init function after you go mod vendorit. Because it calls an Dir() command that is not supported on wasm. Probably this should be reported to Golang.

henrixapp · 2020-09-22T19:13:49Z

That is is really a nice addition! The Optimization does somehow not yet work in my docker_development setup. I tried it with the pdf I uploaded to basecamp. The console logs look like this:

File {name: "some_name.pdf", lastModified: 1600688288000, lastModifiedDate: Mon Sep 21 2020 13:38:08 GMT+0200 (Mitteleuropäische Sommerzeit), webkitRelativePath: "", size: 7773739, …}
worker.js:29 MessageEvent {isTrusted: true, data: {…}, origin: "", lastEventId: "", source: null, …}
worker.js:30 DedicatedWorkerGlobalScope {name: "", onmessage: null, onmessageerror: null, postMessage: ƒ, close: ƒ, …}
worker.js:1 Uncaught (in promise) CompileError: WebAssembly.instantiateStreaming(): function size > maximum function size: 272039936 < 7654321 @+27540

Hmmm. Some googling brings up this as an explanation. What browser are you using?

fosterfarrell9 · 2020-09-22T19:28:50Z

Hmmm. Some googling brings up this as an explanation. What browser are you using?

Chrome on Windows. Will try tomorrow with other browsers.

henrixapp · 2020-09-22T19:41:59Z

Ok. That sounds strange, because I tested it with chrome on macOS. I tried to optimise the build with wasm-opt -O3, see the attached file (as zip, unzip it, GitHub supports no attachments of type wasm). Maybe you can try this one out tomorrow.
pdfcomprezzor.wasm.zip

henrixapp · 2020-09-22T20:08:03Z

I've downloaded the file from GitHub and checked with wasm-objdump -h First the local version, working on my system:

 wasm-objdump pdfcomprezzor.wasm  -h

pdfcomprezzor.wasm:     file format wasm 0x1

Sections:

   Custom start=0x0000000e end=0x00000080 (size=0x00000072) "go.buildid"
     Type start=0x00000086 end=0x000000c8 (size=0x00000042) count: 12
   Import start=0x000000ce end=0x0000033d (size=0x0000026f) count: 23
 Function start=0x00000343 end=0x00001190 (size=0x00000e4d) count: 3659
    Table start=0x00001196 end=0x0000119b (size=0x00000005) count: 1
   Memory start=0x000011a1 end=0x000011a5 (size=0x00000004) count: 1
   Global start=0x000011ab end=0x000011d4 (size=0x00000029) count: 8
   Export start=0x000011da end=0x000011fb (size=0x00000021) count: 4
     Elem start=0x00001201 end=0x00002e36 (size=0x00001c35) count: 1
     Code start=0x00002e3c end=0x003b9554 (size=0x003b6718) count: 3659
     Data start=0x003b955a end=0x005efae8 (size=0x0023658e) count: 69708
   Custom start=0x005efaee end=0x005efb31 (size=0x00000043) "producers"
   Custom start=0x005efb37 end=0x0060cd86 (size=0x0001d24f) "name"

And now downloaded from GitHub:

pdfcomprezzor.wasm:     file format wasm 0x1
0006b00: error: function body must end with END opcode
03b9556: error: invalid section code: 66

Sections:

   Custom start=0x0000000e end=0x00000080 (size=0x00000072) "go.buildid"
     Type start=0x00000086 end=0x000000c8 (size=0x00000042) count: 12
   Import start=0x000000ce end=0x0000033d (size=0x0000026f) count: 23
 Function start=0x00000343 end=0x00001190 (size=0x00000e4d) count: 3659
    Table start=0x00001196 end=0x0000119b (size=0x00000005) count: 1
   Memory start=0x000011a1 end=0x000011a5 (size=0x00000004) count: 1
   Global start=0x000011ab end=0x000011d4 (size=0x00000029) count: 8
   Export start=0x000011da end=0x000011fb (size=0x00000021) count: 4
     Elem start=0x00001201 end=0x00002e36 (size=0x00001c35) count: 1
     Code start=0x00002e3c end=0x003b9554 (size=0x003b6718) count: 3659

Placing it in my folder leads to:

Uncaught (in promise) CompileError: WebAssembly.instantiateStreaming(): function size > maximum function size: 272039936 < 7654321 @+27540

Something is wrong with the compression, or maybe a line ending thing?

henrixapp · 2020-09-22T20:22:55Z

The error was because git didn't know that wasm is a binary format. Added that to the .gitattributes.

henrixapp · 2020-09-23T05:21:08Z

Maybe we shouldn't include the binary (because its size is >5 MB) and create it on the fly or download from the releases page. What are your opinions about that?

fosterfarrell9

What we should maybe discuss about (but we can postpone that also after the merge as it has technically nothing to do with pdf compressing and discuss it with more people on basecamp) is if the workflow is clear enough for users and what we can do about it: After one has successfully uploaded a file, one still has to press "Save". People might (and some probably will) forget that (and then say, "Hey folks, I uploaded my submission, but you claim I haven't...").
Maybe we should communicate this better to the users (like something in red: Press! This! Button!) ;-) .

fosterfarrell9 · 2020-09-23T13:25:21Z

app/views/submissions/_form.html.erb

+						<%= t('submission.optimize') %>
+					</button>
+				</div>
+			<!--<div class="col-1"></div>-->


Is there a reason for this? If yes, maybe add a comment why, or there is the danger that it will be deleted by some one in the future who thinks it is not necessary. (I sometimes use col-11 instead of col-12 because bootstrap occasionally messes up line breaking and overflow in col-12).

Nope I will remove it.

fosterfarrell9 · 2020-09-23T13:32:28Z

app/views/submissions/_form.html.erb

+				<div class="col-11">
+					<b id="log-btn"><%=t('submission.optimization_log')%></b>
+					<div id="userManuscript-optimize-log" class="submission-initially-hidden">
+					</div>


There seems to be a bug (which I do not understand as the code seems perfectly fine): #userManuscript-optimize-log does not pick up .submission-initially-hidden so it is visible from the beginning and the logs are displayed. It works if you add display: none; here, don't know why.

As i played around with that I also came up with some design suggestion:

<span id="log-btn" class="badge badge-light clickable"> <%=t('submission.optimization_log')%> </span> div id="userManuscript-optimize-log" class="submission-initially-hidden border bg-light p-2"> </div>

fosterfarrell9 · 2020-09-23T13:51:00Z

app/views/submissions/_form.html.erb

+				<small>
+					<%= t("submission.optimization_help_html")%>
+					<%= helpdesk(t('submission.optimization_info'),true)%>
+				</small>


The <small> block should IMO be hidden after successful optimization as it refers to a button that does not exist afterwards.

Also the button?

Hmm, the button contains the size information, so I think it should stay (or you find another place to put the size information, like in parentheses after #file-size-correct but I do not know if this is totally consistent with the other cases).

fosterfarrell9 · 2020-09-23T13:52:19Z

config/locales/de.yml

+    file_size_too_big: Die Datei ist eigentlich zu groß, kann aber hochgeladen werden.
+    file_size_way_too_big:  Die Dateigröße überschreitet das Limit. Bitte optimiere sie.
+    optimization_help_html: >
+      <b>Optimieren:</b>Verwende diesen Knopf, um automatisch 


Add space after </b>

fosterfarrell9 · 2020-09-23T14:02:06Z

Maybe we shouldn't include the binary (because its size is >5 MB) and create it on the fly or download from the releases page. What are your opinions about that?

I am neutral because I don't know enough about this WASM stuff. @tynsh @christian-heusel What do you think?

tynsh · 2020-09-23T14:11:21Z

I don't have time for this today, sorry. If possible, I would recommend compiling the binary from source, since you at least get security updates for go and don't have a huge binary in git. But if you want me to look at it, I can do that tomorrow

henrixapp · 2020-09-23T14:21:48Z

I would agree with @tynsh, I would suggest using a git submodule and add following to the Dockerfile:

FROM golang:1.14 AS build-pdfcomprezzor
WORKDIR /go/src
COPY <submodule>/go.mod .
COPY <submodule>/go.sum .
COPY <submodule>/main.go .
RUN go mod vendor
RUN sed -i 's/init(/init2(/g' vendor/github.com/pdfcpu/pdfcpu/pkg/font/metrics.go
RUN GOOS=js GOARCH=wasm go build -o pdfcomprezzor.wasm 

FROM ...

COPY --from=build-pdfcomprezzor /go/src/pdfcomprezzor.wasm <path/to/public/pdfcomprezzor.wasm>

EDIT: As far as I know you would not get any security updates/improvements, because it is locked to Golang:1.14. But I think the file size consideration suffices as reason.

fosterfarrell9 · 2020-09-23T15:48:25Z

Okay, git submodule is fine with me.

P.S.: Just tried the (approved) pull request on a different (crappy) monitor. Turns out that text-warning in the foreground and bg-mdb-color-lighten-7 in the background for the <%= t('submission.submission_upload_notice_html')%> block can combine to an unreadable mess. Using text-danger seemed to work a lot better.

fosterfarrell9 · 2020-09-23T16:45:22Z

@henrixapp Should the git submodule stuff be done in a different pull request (aka should I merge this one) or should this pull request remain open until this git submodule stuff is integrated?

henrixapp · 2020-09-23T17:07:29Z

I don't know how many files have to be edited for production, etc. Nevertheless, I would suggest working on this branch and squashing it afterwards, so that the pdfcomprezzor.wasm is not committed to master.

Who will edit the Dockerfiles @tynsh @christian-heusel ?

tynsh · 2020-09-25T12:28:43Z

The Dockerfiles are done and tested in development. I would like to rebase this branch onto mampf-next, since there currently is a bug preventing deployment of this branch in experimental. Experimental has a similar setup as production and allows testing the production part of the docker setup

henrixapp · 2020-09-25T12:36:34Z

go ahead and rebase and then squash it, in order to remove pdfcomprezzor.wasm out of its history. @tynsh the wasm is still in public?

tynsh · 2020-09-25T12:59:25Z

Yes, the wasm file is still in public/pdfcomprezzor/pdfcomprezzor.wasm

henrixapp · 2020-09-25T13:01:15Z

I removed it from history with bfg. You can rebase and merge ;)

tynsh · 2020-09-25T19:25:11Z

Seems to work fine if you rebase onto master again. But please rebase onto mampf-next and squash it yourself :)

I'm only affected as a user, but this is a pretty great feature :D Thanks!

tynsh · 2020-09-25T19:25:33Z

The rebased version is currently in experimental

More user guidance

henrixapp requested a review from fosterfarrell9 September 21, 2020 18:55

fosterfarrell9 requested changes Sep 22, 2020

View reviewed changes

henrixapp mentioned this pull request Sep 22, 2020

Investigate function size limit problem henrixapp/pdfcomprezzor#2

Closed

henrixapp requested a review from fosterfarrell9 September 23, 2020 08:57

fosterfarrell9 requested changes Sep 23, 2020

View reviewed changes

fosterfarrell9 approved these changes Sep 23, 2020

View reviewed changes

tynsh changed the base branch from master to mampf-next September 25, 2020 12:22

henrixapp force-pushed the pdf_compression_upload branch from fa507ab to af2c6ba Compare September 25, 2020 13:00

henrixapp force-pushed the pdf_compression_upload branch from af2c6ba to 79da734 Compare September 25, 2020 13:17

Adds PDFComprezzor and optimization upload

9a17093

henrixapp and others added 6 commits September 25, 2020 23:58

Adds info box for optimization & more feedback

b169d16

Ammends .gitattributes+ new build of pdfcomprezzor

837be51

Adds locale, refactors code to obey to line limit

dc86f25

More user guidance

Remove not needed logs.

0e7a1de

Fixes change requests by @fosterfarrell9

e93b22d

Automatically build pdfcomprezzor web assembly during docker build

0ae07ce

henrixapp force-pushed the pdf_compression_upload branch from 79da734 to 0ae07ce Compare September 25, 2020 21:58

henrixapp merged commit 6bf47c0 into mampf-next Sep 25, 2020

henrixapp deleted the pdf_compression_upload branch September 25, 2020 21:59

henrixapp mentioned this pull request Oct 14, 2020

Windows line endings #57

Closed

Adds PDFComprezzor and optimization upload #81

Adds PDFComprezzor and optimization upload #81

Conversation

henrixapp commented Sep 21, 2020

fosterfarrell9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynsh commented Sep 22, 2020

henrixapp commented Sep 22, 2020

henrixapp commented Sep 22, 2020

fosterfarrell9 commented Sep 22, 2020 • edited Loading

henrixapp commented Sep 22, 2020 • edited Loading

henrixapp commented Sep 22, 2020

henrixapp commented Sep 22, 2020

henrixapp commented Sep 23, 2020

fosterfarrell9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fosterfarrell9 Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

fosterfarrell9 commented Sep 23, 2020

tynsh commented Sep 23, 2020

henrixapp commented Sep 23, 2020 • edited Loading

fosterfarrell9 commented Sep 23, 2020

fosterfarrell9 commented Sep 23, 2020

henrixapp commented Sep 23, 2020 • edited Loading

tynsh commented Sep 25, 2020

henrixapp commented Sep 25, 2020 • edited Loading

tynsh commented Sep 25, 2020

henrixapp commented Sep 25, 2020 • edited Loading

tynsh commented Sep 25, 2020

tynsh commented Sep 25, 2020

fosterfarrell9 commented Sep 22, 2020 •

edited

Loading

henrixapp commented Sep 22, 2020 •

edited

Loading

fosterfarrell9 Sep 23, 2020 •

edited

Loading

henrixapp commented Sep 23, 2020 •

edited

Loading

henrixapp commented Sep 23, 2020 •

edited

Loading

henrixapp commented Sep 25, 2020 •

edited

Loading

henrixapp commented Sep 25, 2020 •

edited

Loading