Support epub and other downloads from ZIM in jQuery mode #462
Conversation
|
I quickly tested on Firefox 64 on Ubuntu : it seems to work well. |
|
I suppose this branch needs to be rebased on master. In particular to re-use the bootstrap alerts code that has already been merged? |
|
I agree, but I'm a bit short of time to do that right now. Can we leave this to 2.6? I should have more time in a couple of weeks. |
|
Of course, no problem! |
Jaifroid
force-pushed
the
Support-epbub-downloads-from-Gutenberg-ZIMs
branch
from
02f2dac to
9c12337
Compare
|
I have mechanically rebased this branch / PR on master, but it is not yet ready for review, as I need to ensure it is re-using the existing bootstrap alert code first. |
Jaifroid
force-pushed
the
Support-epbub-downloads-from-Gutenberg-ZIMs
branch
from
8c03964 to
29df2dc
Compare
|
I've squashed this branch to make review easier. I think it's ready for review, though I've run out of time to test on all browsers today, can test more at w/e. I've tested the latest code on Edge and IE11. I tested the first few iterations of the code on all browsers, and the code has only been re-organized, not changed in any substantial way. I'm keen to get your feedback, @mossroy . We may still need to think further about the issue of creating bootstrap alerts in code, but it's all tucked into uiUtil.js. |
|
Tested and working on Firefox ESR 52.5.2. |
|
It seems to work well on Firefox 65 too (on Linux). I'll have a look at the code |
Thank you! I hadn't seen examples of PDFs before, so it's good to know that file type is also supported. It's just a small edit in two places to enable PDF download. I can push a commit with that now. |
|
It now seems to work well for PDF too, thanks! NB : It's not related to this PR, but I opened openzim/gutenberg#79 for some non-working hyperlinks in this book (I suppose it's a problem inside the ZIM file itself) |
|
Downloading epub files works perfectly well on my Firefox OS device too, and can be read with an epub reader. |
There was a problem hiding this comment.
It looks good.
But we really need to investigate on #470 because the code of these display* functions in uiUtil.js is partly redundant and mixes presentation (which should be in HTML files) and code (that should stay in this js file)
www/js/app.js
Outdated
| // For Boolean values, getAttribute can return any of the following: download="" download="download" download="true" | ||
| // So we need to test hasAttribute first: see https://developer.mozilla.org/en-US/docs/Web/API/Element/getAttribute | ||
| // However, we cannot rely on the download attribute having been set, so we also need to test for known download file types | ||
| var downloadAttribute = this.hasAttribute('download') || regexpDownloadLinks.test(href); |
There was a problem hiding this comment.
I find this variable name misleading : it should indicate that it's a boolean meaning that this link should be downloaded.
Something like "isDownloadableLink" or "shouldBeDownloaded"?
There was a problem hiding this comment.
OK, thanks, I'll rename.
www/js/app.js
Outdated
| var downloadValue = this.hasAttribute('download') || regexpDownloadLinks.test(href); | ||
| if (downloadValue) { | ||
| downloadValue = this.getAttribute('download') || true; | ||
| downloadValue = /download|true/i.test(downloadValue) || downloadValue; |
There was a problem hiding this comment.
@mossroy Because the download attribute can be Boolean, or it can contain a string value representing the filename to use when saving the file in the filesystem, I've unified the two separate variables I had (downloadAttribute and downloadValue) into a single variable (downloadValue). I've tested this code with different values of the download attribute, and it seems to function as intended, i.e., if the attribute contains "", "download" or "true", it will be set to Boolean true (also true if there is no download attribute, but regexpDownloadLinks matches). But if the download attribute contains any other string value, it will be set to the string. However, I could only test it indirectly, as I can't manipulate the actual value of the attribute at debug time, and can only manipulate the variable itself (downloadValue). I'm 99% sure it works, but could you have a look and see if the code looks logically correct to you?
There was a problem hiding this comment.
I still find it complicated to read/understand.
You're right about the way to interpret the download attribute, and about the way to generate a file name. But maybe this code (that generates a filename) could be moved completely in parseAnchorsJQuery function, so that it then passes a filename (as a String) to goToArticle and displayFileDownloadAlert. It would simplify the understanding of the parameters of these functions (even if they were well documented).
Then, I would suggest to avoid reusing a variable for different purposes/types of content (except if it's really necessary, maybe for optimization).
For example, there could be a "isDownloadableLink" (boolean), then a "downloadAttributeValue" (string), and finally a "downloadFileName" (string). The first two ones are just temporary variables, used to generate the third one. As only the third one is used afterwards, this logic might even be externalized in a separate function, like "getDownloadFileName(this, decodedURL)", but maybe it's not necessary as there is not that much code to put in it.
There was a problem hiding this comment.
OK, I'll have another go!
There was a problem hiding this comment.
Thinking about this, I'm not keen on only passing a string in the download parameter of goToArticle because I think that function should remain generic in line with the HTML5 spec. In the spec, a download can be triggered either with a Boolean or with a filename. So it should be possible to trigger a download with just download=true passed to goToArticle . There might be other cases in the future where we want to trigger a download of content in line with the HTML5 spec. Given that, I think the only logical place to construct a filename if one is missing is in the appropriate function in uiUtil.js.
What I've done is to simplify the code in parseAnchorsJQuery as best I can following your advice above, except that I haven't included a donwloadFileName string here for the reasons stated. I have also moved the generation of a contentType, if it is missing from the HTML, into uiUtil.js so that we have consistent behaviour: if the download attribute is missing, we just pass a Boolean or null; if a contentType is missing, we just pass null. Then the download function in uiUtil.js takes care of constructing any missing data. I believe this is now better, and has decluttered parseAnchorsJQuery.
Let me know if you agree.
There was a problem hiding this comment.
All right, it can make sense.
Regarding the test /download|true/i.test(downloadAttrValue), maybe it should be fixed for the case where downloadAttrValue contains one of those words without being equal to it.
I suppose you only want to match equality.
For example, if the download attribute is "truelle.zip", I suppose you want downloadAttrValue to be "truelle.zip" and not true.
Appart from that, I think you can squash/rebase/merge.
Thanks for this PR
I've checked this and the html for this blue arrow is:
I can't see any JavaScript in the html, or any script tag, so I guess that We probably need to add code to |
I agree. We will have to change the function of clicking on the bootstrap alert's close button (the x top right) so that it hides the alert rather than removing it from the DOM. Anyway, I'll tackle this in #470. |
|
Yes, handling # hyperlinks is a separate issue, it would be better to fix it in another PR. |
Thank you, yes, I hadn't thought of that. I'll fix with:
|
|
All right, that should be good. |
Jaifroid
force-pushed
the
Support-epbub-downloads-from-Gutenberg-ZIMs
branch
from
fd1e947 to
b69e371
Compare
This code addresses issue #439 . Currently the epub links in Guttenberg ZIMs are useless in jQuery mode. This PR handles the "download" of the epub from the ZIM to the OS's filesystem, or to open the file using the browser's open method. The code is tested and working with
gutenberg_es_all_2018-10.zimin Edge, Chromium 66, Firefox ESR, FFOS simulator, and IE11.Since JS doesn't work, to choose a book to test, type a space in the search field, and choose any title with the word "(cover)" in the name. Then click on the big red download icon (not the HTML5 icon, which is a simple html/text-based version of the book). In some browsers the file will automatically be saved in the download location set in the browser. In others, the user is offered a choice to save the file or open it.
The user will need to have an epub app/reader installed unless the OS provides one. In Windows 10, Edge acts as the system epub reader, with a nice resizable interface and page flipping (unless the user has installed a different epub app).
In FFOS Simulator, a message pops up at the top of the app screen to say that the file has been successfully downloaded, but when I tap on the message (the one at the top of the screen, not the alert box at the bottom), I then get a message that the file does not exist in the file system. I guess this is a quirk of the simulator not having a physical filesystem, or else that there is no installed app to handle epub files.
There is no rush to merge this before #457 (2.5 Release), unless it's felt desirable to add to the feature set of 2.5. I've tested the code fairly extensively, but it could do with testing on other systems (e.g. Linux), and I don't know how downloads are likely to be handled in extensions.