search

readium / architecture

📚 Documents the architecture of the Readium projects
https://readium.org/architecture/
BSD 3-Clause "New" or "Revised" License
171 stars 33 forks source link

Readium 1 viewer with Readium 2 Streamer #41

Open barbender opened 7 years ago

barbender commented 7 years ago

This is a request to create repos for the project that aims at integration of Readium 1 viewer with Readium 2 Streamer.

Here are some points on the way we approach this task:

Based on the above the bulk of changes will be in readium-js and readium-shared-js. As of now we have started the work by forking readium-js and readium-shared-js into:

git@github.com:evidentpoint/readium-js.git git@github.com:evidentpoint/readium-shared-js.git

In order to add visibility to this project we probably want to make them more accessible to the team and have these repos officially under https://github.com/readium

Since these repos will contain modified readium 1 code, we probably want to keep this link in the naming, but also want to stress the fact that they are integrating with r2 streamer.

My immediate thought would be readium-js-r2-streamer and readium-shared-js-r2-streamer. Any other suggestions are welcome.

danielweck commented 7 years ago

For my own sanity / intellectual curiosity, I spent an hour investigating how to "interface" Readium2 parser-streamer with Readium1 reader / navigator. I went as far as: https://github.com/readium/readium-js/compare/develop...feature/readium2

It isn't much right now, for example the convertWebPubManifestToPackageDoc() function is far from complete. But with this small change in readium-js only (basic plumbing + incomplete model converter), it is possible to point the web/cloud reader to a r2-streamer / webpub-manifest URL (no rendering though, just basic mechanics in place). And as you said, besides passing the initial JSON data to the readium-shared-js "entry "point", there is more to do, for example generating the TOC requested by readium-js-viewer.

danielweck commented 7 years ago

...so, perhaps EvidentPoint's code contribution could be pushed into a feature branch, instead of new repositories? (to avoid the proliferation of forked repositories, especially if renamed)

danielweck commented 7 years ago

For reference, here are EvidentPoint's code diffs: readium-js: https://github.com/readium/readium-js/compare/develop...evidentpoint:r2-streamer-integration

readium-shared-js: (unfortunately, this fork does not seem to have readium as its upstream?!) https://github.com/readium/readium-shared-js/compare/develop...evidentpoint:r2-streamer-integration

barbender commented 7 years ago

Our current approach (in r2-streamer-integration) is to add WebPub functionality without breaking the existing, i.e., we should be able to point the reader to either regular epub file or webpub manifest from R2 Streamer. In particular both of these should work:

Now, r2-streamer-integration branch is on Evidentpoint fork. Would it make sense to move this branch to https://github.com/readium/readium-js https://github.com/readium/readium-shared-js , rename it to feature/r2-streamer-integration and have a branch specific README.md so that we can document the progress there ?

barbender commented 7 years ago

Daniel, I tried to work with your branch - feature/readium2 trying to use 'epub' query parameter to point to webpub manifest:

http://localhost:8090/dev/index_RequireJS_no-optimize_LITE.html?epub=http://localhost:54549/L1VzZXJzL21pY2hhZWxzL2VwX2Jvb2tzL2lncC10d3NzLmVwdWI=/manifest.json

Is this how you envisined pointing to WebPub manifest ?

This would not work without small change:

michaels@michaels-mbp:~/readium/readium-js-viewer/readium-js/readium-shared-js$ git diff
diff --git a/js/helpers.js b/js/helpers.js
index 2f167e6..cce5622 100644
--- a/js/helpers.js
+++ b/js/helpers.js
@@ -90,29 +90,30 @@ Helpers.getEbookUrlFilePath = function(ebookURL) {
     }
 };

-/**
- *
- * @returns object (map between URL query parameter names and corresponding decoded / unescaped values)
- */
-Helpers.getURLQueryParams = function() {
-    var params = {};
-
-    var query = window.location.search;
-    if (query && query.length) {
-        query = query.substring(1);
-        var keyParams = query.split('&');
-        for (var x = 0; x < keyParams.length; x++)
-        {
-            var keyVal = keyParams[x].split('=');
-            if (keyVal.length > 1) {
-                params[keyVal[0]] = decodeURIComponent(keyVal[1]);
+    /**
+     *
+     * @returns object (map between URL query parameter names and corresponding decoded / unescaped values)
+     */
+    Helpers.getURLQueryParams = function () {
+        var params = {};
+
+        var query = window.location.search;
+        if (query && query.length) {
+            query = query.substring(1);
+            var keyParams = query.split('&');
+            for (var x = 0; x < keyParams.length; x++) {
+                // assume that after splitting with '&' we only want take one '=' as separator
+                var keyValue = keyParams[x];
+                var eqSignIndex = keyValue.indexOf('=');
+                var parts = keyValue.split('=');
+                if (parts.length > 1) {
+                    params[parts[0]] = decodeURIComponent(keyValue.substring(eqSignIndex + 1));
+                }
             }
         }
-    }
-
-    return params;
-};

+        return params;
+    };

after the change above, it still fails with:

GET http://localhost:54549/L1VzZXJzL21pY2hhZWxzL2VwX2Jvb2tzL2lncC10d3NzLmVwdWI=/manifest.json/META-INF/container.xml net::ERR_CONNECTION_REFUSED send @ jquery.js:9175 ajax @ jquery.js:8656 PlainResourceFetcher.fetchFileContentsText @ plain_resource_fetcher.js:78 PublicationFetcher.getFileContentsFromPackage @ publication_fetcher.js:197 PublicationFetcher.getXmlFileDom @ publication_fetcher.js:210 PublicationFetcher.getPackageFullPath @ publication_fetcher.js:217 PublicationFetcher.getPackageDom @ publication_fetcher.js:241

I assume it is expected, as your work is WIP, right ? thanks, Michael

danielweck commented 7 years ago

Hello, first of all, IMO there is no need to patch Helpers.getURLQueryParams(). The value of the query string parameter epub must be escaped, otherwise the base64 encoding in the streamer URI (and perhaps other fields too) will interfere with URL parsing. e.g. http://domain.com/readium.html?epub=http://other.domain/BASE64/manifest.json

Using your concrete example: http://localhost:8090/dev/index_RequireJS_no-optimize_LITE.html?epub=http://localhost:54549/L1VzZXJzL21pY2hhZWxzL2VwX2Jvb2tzL2lncC10d3NzLmVwdWI=/manifest.json ==> http://localhost:8090/dev/index_RequireJS_no-optimize_LITE.html?epub=http%3A%2F%2Flocalhost%3A54549%2FL1VzZXJzL21pY2hhZWxzL2VwX2Jvb2tzL2lncC10d3NzLmVwdWI%3D%2Fmanifest.json

danielweck commented 7 years ago

Secondly, yes you are correct, my feature/readium2 branch code is very much WIP, in fact the primary purpose was to analyse technical feasibility, present the results to you and other developers, and discuss :) The time and effort required to implement a full converter / adapter "readium2 webpub-manifest" ==> "readium1 data model / JSON" does not seem trivial, but if this use-case matters to EvidentPoint then it may indeed be a nice demonstration of R1-R2 interoperability, worth sharing with everybody else. So, thank you for that. I personally have no concrete plans to continue the feature/readium2 implementation, and I would probably rather spend development cycles on a proper R2 "navigator", instead of attempting to retrofit the Readium1 reader. The rationale for this is that currently ; and for the foreseeable future ; publications are available in the EPUB2/3 format, which can be ingested directly by Readium1. Readium2's "webpub manifest" JSON format (which right now is "just" an internal data model) is in fact derived / converted from existing ebooks, so it doesn't seem to make much logical sense to convert once again from "webpub manifest" into EPUB. This is an interesting round-tripping exercise, but I am concerned about the cost / benefit ratio. Once again, if this feature is important to EvidentPoint, then of course I encourage and support your effort. To be discussed.

barbender commented 7 years ago

Thank you, Daniel, for steering up the discussion.

I fully agree with you that if we look at this project as a pure WebPub -> PackageDocument converter, this does not make much sense. However, if we reframe this into an effort to: "Create a Readium 2 compatible navigator based on Readium 1 project", it probably becomes more valuable. The advantage of this approach is that we are not starting from scratch but rather relying on a mature and feature rich Readium 1 project that already has functionality that is currently only on the Road Map of Readium 2:

The other advantage for the current users of Readium 1 is that (providing that readium-js, readium-shared-js interface will be kept the same) there will be no need to adapt to the new API in their reading systems.

There are also intrinsic advantages of having Readium 2 streamer in the picture (as I understand it), such as:

Please let us know if this makes sense, regards, Michael

barbender commented 7 years ago

I was able to get rendering and TOC working in feature/readium2 branch, see this pull request:

https://github.com/readium/readium-js/pull/179

regards, Michael

danielweck commented 7 years ago

Note that I added the https://readium-2.surge.sh deployment from the feature/readium2 branch of ReadiumJS to the r2-streamer-js list of "reader" links. For example: https://readium2.herokuapp.com/pub/L2FwcC9taXNjL2VwdWJzL2NoaWxkcmVucy1saXRlcmF0dXJlLmVwdWI%3D