GNOME Bugzilla – Bug 740971
Add an ePub view
Last modified: 2016-06-24 11:36:22 UTC
From Martha's GSoC for example, this would be added to the WebKit and evince views.
The code is at: https://github.com/martamilakovic/gnome-books/tree/master/ It's mostly: - a version of epub.js (upstream is at: https://github.com/futurepress/epub.js) - a special webView (similar to the PreviewView): https://github.com/martamilakovic/gnome-books/blob/master/src/webView.js The main problem will be to hide Evince specific code inside the view object. In time, this will also help with supporting document types that aren't handled by Evince, such as CHM, Mobi and most importantly, LibreOffice documents.
Reassigning to new default assignee.
I'll start to work in this issue. I've been working trying to add epub support to evince, but now I think it's better to add it to gnome-documents. I wrote a c-glib library to work with epub, with gir annotations: https://github.com/danigm/libgepub Here is the code I have for evince with epub support: https://github.com/danigm/evince/tree/epub I don't know what is the state of this issue, if epub.js is ready to use of if it will be easy to use libgepub. I'll take a look. The big problem I've found trying to add epub support in evince was the pagination. epub is a list of html files, we can show it whell with webkitgtk, but in a paginated view it's not easy to split the view and paginate. Maybe we can show the same webkitview hidding the scroll and setting the scroll depending on the "page", but screen size will change the number of "pages". The easiest way will be to not show pages, and show the whole html in a webkitview with scroll, but we still have "chapters", there's more than one html in a epub so we need to find a way to go from one to another.
Created attachment 328313 [details] [review] Initial epub support for gnome-books Here you have a patch that adds epub preview support to gnome-documents. I'm using webkitgtk to show the epub content, and I've also using libgepub [1], a custom epub lib written in C with glib and gobject instrospection, that I wrote before to try to add epub support to evince. The gepub lib is currently in my github, but I've wrote all the code and as far as I know it is not been used by any other project, so we can add it to the gnome-documents project or as a separated lib in the gnome git / infraestructure. I've recorded [2] an example for you to view what is the current status. [1] https://github.com/danigm/libgepub [2] http://danigm.net/gnome-books-epub.ogv
Review of attachment 328313 [details] [review]: Thank you so much for this patch. Comments inline. ::: configure.ac @@ +78,3 @@ evince-view-3.0 >= $EVINCE_MIN_VERSION webkit2gtk-4.0 >= $WEBKITGTK_MIN_VERSION + libgepub >= $LIBGEPUB_MIN_VERSION Since this is only used through the introspected library, it should use the newly introduced AX_CHECK_GIRS_GJS like elsewhere in configure.ac. ::: src/documents.js @@ +624,3 @@ } + if (this.mimeType == 'application/epub+zip') { It would be nice to factor this out in the epub view class like it is done in the LOKView case. @@ +625,3 @@ + if (this.mimeType == 'application/epub+zip') { + let exception = null; You can pass null directly instead, since there's no other case here where we want to set the exception. @@ +711,3 @@ if (LOKView.isOpenDocumentFormat(this.mimeType) && !Application.application.isBooks) { this.viewType = ViewType.LOK; + } else if (this.mimeType == 'application/epub+zip') { Should we show the epub view in both Documents and Books mode? I think perhaps we should only use it for Books. ::: src/embed.js @@ +292,3 @@ break; + case WindowMode.WindowMode.PREVIEW_EPUB: + if (oldMode == WindowMode.WindowMode.EDIT) Can you ever get here from an edit mode? ::: src/epubview.js @@ +51,3 @@ + this._uri = null; + this._overlay = overlay; + this.get_style_context().add_class('documents-scrolledwin'); It's not a scrolled window - I doubt this is needed. @@ +57,3 @@ + this.add_named(this._errorBox, 'error'); + + this._sw = new Gtk.ScrolledWindow({hexpand: true, Missing space after curly braces @@ +66,3 @@ + let model = this._getPreviewContextMenu(); + this._previewContextMenu = Gtk.Menu.new_from_model(model); + this._previewContextMenu.attach_to_widget(this._sw, null); The menu is never used here, so you should either wire it up similar to what happens in the other views, or not load it at all. @@ +80,3 @@ + return; + + let docuri = doc.uri.slice(7).replace(/%20/g, ' '); At the very least you should create a GFile for the URI and get the path from it instead. But a better way would be to make Gepub work with a GFile directly. As a side note, the usual pattern for introspectable libraries is not to do anything special in _new() function, which is only provided as a convenience for C clients. Things like the path should be made GObject construct properties, and then you would just use a normal JavaScript constructor to create the object. For instance: let epubdoc = new Gepub.Doc({ file: file }); May be worth fixing that in libgepub now before the API is used more widely. @@ +85,3 @@ + + this._epubResources = []; + for (var i in this._epubdoc.get_resources()) { No need for curly braces on a single line block @@ +86,3 @@ + this._epubResources = []; + for (var i in this._epubdoc.get_resources()) { + this._epubResources.push(i); This loop does not seem very useful, also because this._epubResources seems to be unused. @@ +101,3 @@ + + _replaceResource: function(doc, tag, attr) { + var ret2 = GLib.strdup(doc); strdup() should not be used in JS. You can just copy the string to another variable. @@ +102,3 @@ + _replaceResource: function(doc, tag, attr) { + var ret2 = GLib.strdup(doc); + var rex = new RegExp(attr+'\s*=\s*"([^"]*)"', "ig"); This whole function deserves at least a comment explaining the purpose. Do I understand this correctly that you are loading the data for a resource here and inline it as base64? Why do you need to do that, instead of passing the resource to WebKit when it needs to load it? @@ +117,3 @@ + }, + + replaceResources: function(current) { It does not look like this function needs to be "public" (i.e. it should start with an underscore) @@ +136,3 @@ + + this.set_visible_child_full('view', Gtk.StackTransitionType.NONE); + this._copy.enabled = false; Where is this._copy defined? @@ +162,3 @@ + go_next: function() { + this._epubdoc.go_next(); + if (this.page < this._epubSpine.length) { From what I can see the "spine" is represented as a pointer into a list of all the pages. So if I'm not mistaken its length would be the number pages *after* the current one. I guess this code works though because you save the original spine at the beginning... There are better ways to do this though, e.g. you could make the go_next()/go_prev() methods in Gepub return whether they worked or not. @@ +178,3 @@ + _load_current: function() { + let current = this._epubdoc.get_current(); + current = this.replaceResources(String(current)); Is "current" not already a string? See also my other comments in this function though - maybe there's a better way to do this. @@ +193,3 @@ + + createSearchWidgets: function() { + let sb = this; Any reason not to use "this" instead? @@ +201,3 @@ + this._searchEntry = new Gtk.SearchEntry({ width_request: 500 }); + this._searchEntry.connect('activate', Lang.bind(this, + function() { Move this to the line above. @@ +211,3 @@ + this._prev.set_tooltip_text(_("Find Previous")); + this._prev.connect('clicked', Lang.bind(this, function() { + sb.prev(); You could just pass Lang.bind(this, this.prev) instead @@ +226,3 @@ + let fc = this._previewView.view.get_find_controller(); + fc.connect('found-text', Lang.bind(this, + function(w, match_count, data) { Move this to the line above @@ +227,3 @@ + fc.connect('found-text', Lang.bind(this, + function(w, match_count, data) { + this._onSearchChanged(this._previewView, match_count); You can pass match_count > 0 here @@ +230,3 @@ + })); + + this._onSearchChanged(this._previewView, 0); You can pass false here @@ +237,3 @@ + let findNext = Application.application.lookup_action('find-next'); + findPrev.enabled = Boolean(results); + findNext.enabled = Boolean(results); With my suggestions above, results can be changed to hasResults and would be a boolean already @@ +240,3 @@ + }, + + search: function(str) { This should be private @@ +245,3 @@ + }, + + prev: function() { This should be private @@ +250,3 @@ + }, + + next: function() { This should be private @@ +297,3 @@ + let backButton = this.addBackButton(); + backButton.connect('clicked', Lang.bind(this, + function() { Move this to the line above @@ +300,3 @@ + Application.documentManager.setActiveItem(null); + Application.modeController.goBack(); + this._searchAction.enabled = true; It does not look like the class ever sets this to false. Is this needed here? @@ +318,3 @@ + this.connect('destroy', Lang.bind(this, + function() { + this._searchAction.enabled = true; It does not look like the class ever sets this to false. Is this needed here? @@ +343,3 @@ + section = builder.get_object('rotate-section'); + section.remove(0); + section.remove(0); Is there anything left in the menu at the end? Otherwise you could just omit it for now. @@ +346,3 @@ + + return menu; + Extra whitespace @@ +462,3 @@ + }, + + _updateVisibility: function() { There's nothing that calls this when e.g. the page changes, but perhaps it's OK for now...
(In reply to Daniel Garcia from comment #4) > Here you have a patch that adds epub preview support to gnome-documents. I'm > using webkitgtk to show the epub content, and I've also using libgepub [1], > a custom epub lib written in C with glib and gobject instrospection, that I > wrote before to try to add epub support to evince. This is great! Thanks for picking it up. Having a GObject introspected library sounds great for this task. I also wonder if the WebKit "view" couldn't be moved into libgepub as well, maybe when it's a bit more mature. > The gepub lib is currently in my github, but I've wrote all the code and as > far as I know it is not been used by any other project, so we can add it to > the gnome-documents project or as a separated lib in the gnome git / > infraestructure. Moving it to the GNOME infrastructure sounds like a good idea -- let me know if you need any help with that. It should stay as a separate library though.
(In reply to Cosimo Cecchi from comment #6) > (In reply to Daniel Garcia from comment #4) > > Here you have a patch that adds epub preview support to gnome-documents. I'm > > using webkitgtk to show the epub content, and I've also using libgepub [1], > > a custom epub lib written in C with glib and gobject instrospection, that I > > wrote before to try to add epub support to evince. > > This is great! Thanks for picking it up. > Having a GObject introspected library sounds great for this task. I also > wonder if the WebKit "view" couldn't be moved into libgepub as well, maybe > when it's a bit more mature. Yes, I thought it before, and there's an old branch where I returned a webkit widget. https://github.com/danigm/libgepub/tree/webkit I think it's not very hard to add a new gobject in the libgepub lib that will be a gtkwidget that inherits from webkitwebview or something like that. > > > The gepub lib is currently in my github, but I've wrote all the code and as > > far as I know it is not been used by any other project, so we can add it to > > the gnome-documents project or as a separated lib in the gnome git / > > infraestructure. > > Moving it to the GNOME infrastructure sounds like a good idea -- let me know > if you need any help with that. It should stay as a separate library though. I've created a new bug to request the git repository: https://bugzilla.gnome.org/show_bug.cgi?id=766783#c0
libgepub is now in the gnome git repository: https://git.gnome.org/browse/libgepub/
Created attachment 328671 [details] [review] Initial epub support for gnome-books, v2 I've review all changes proposed in the comment https://bugzilla.gnome.org/show_bug.cgi?id=740971#c5 I've applied almost all changes, but I've some problems trying to make the constructor: let epubdoc = new Gepub.Doc({ path: f.get_path() }); I can't make it work from libgepub so I left the: let epubdoc = Gepub.Doc.new(f.get_path()); The other thing that I can't resolve is the resource load in webkit, so I added a comment to the _replaceResource function. I've been looking for a solution but I don't find a way to provide the resources so the only solution that I can imagine is to replace the path with the resource in the format "data:base64".
Review of attachment 328671 [details] [review]: I couldn't apply your patch, it fails line 652: patch: **** malformed patch at line 652: diff --git a/src/org.gnome.Books.src.gresource.xml b/src/org.gnome.Books.src.gresource.xml To attach patches, please commit them in your repo, and launch "git bz attach master". In terms of UI, you mentioned pagination. I would really prefer it if the text present in the window didn't have any scrollbars. The text should be truncated when it's filled enough of the screen. The page numbers in the UI should match the page numbers of the original (or ePub I guess) data. The goal being to switch pages as you do on an ebook reader, or a real book, rather than scrolling as on a web page. Note that you'd need to take special care of images that don't fit in the preview widget. I'll be expecting a patch that applies before commenting further. ::: src/epubview.js @@ +106,3 @@ + // of the file. It's done this way because resources paths in epub + // files are relative to the zip file and I can't find a way to + // provide the resource to webkit when it ask for it. What information are you looking for here? I don't understand what the problem is. @@ +124,3 @@ + + _replaceResources: function(current) { + // resources as base64 to avoid path search I don't understand what you get as input, and what you expect as output. @@ +127,3 @@ + + let ret = current; + // css This is a bit too much, or too little information for that comment. ::: src/windowMode.js @@ +31,3 @@ COLLECTIONS: 5, + SEARCH: 6, + PREVIEW_EPUB: 7 This isn't public API, so no need to add your new one at the end. Add it after PREVIEW_LOK.
Created attachment 328715 [details] [review] epub: Initial epub support This change adds the epub document preview using libgepub and webkit to show the content.
(In reply to Daniel Garcia from comment #11) > Created attachment 328715 [details] [review] [review] > epub: Initial epub support > > This change adds the epub document preview using libgepub and webkit to > show the content. I've updated the patch using git bz. And in this patch I've replaced all the base64 resourece embed with a custom uri for webkit. I've found that with webkit you can define a custom URL scheme and that's what I do. About the pagination that mentions Bastien, I think it's not easy to paginate epub documents using webkit to draw, at least I don't know how to do it... I was thinking that we can hide the WebkitWebView scrollbar and use the scroll as pages, so next button will always scroll down and at the end of the document, changes the chapter, but I don't know if with webview we can control the scroll. The other possibility I was thinking is to inject javascript in the html with webkit-web-view-run-javascript, and maybe a custom css to paginate, but we should communicate this with the gnome-books process to connect buttons and to change the chapter on the end.
Review of attachment 328715 [details] [review]: Missing code in mainWindow.js: (org.gnome.Books:29469): Gjs-WARNING **: JS ERROR: Error: Not handled MainWindow<._onKeyPressEvent@resource:///org/gnome/Books/js/mainWindow.js:206 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 main@resource:///org/gnome/Books/js/main.js:47 run@resource:///org/gnome/gjs/modules/package.js:192 @/home/hadess/Projects/gnome-install/bin/gnome-books:6 There's no zoomIn/zoomOut support, or properties tab (expected?) The current view is better than nothing, but I'd really like to see some work towards how that UI should look and behave. ::: src/epubview.js @@ +90,3 @@ + let f = Gio.File.new_for_uri(doc.uri); + this._doc = doc; + //this._epubdoc = new Gepub.Doc({ path: f.get_path() }); Remove? @@ +120,3 @@ + // file. + var ret = doc; + var rex = new RegExp(attr+'\s*=\s*"([^"]*)"', "ig"); I'd really like to have a separate test for this, as it could end up loading external data, and we really want to avoid regressing. ::: src/windowMode.js @@ +31,3 @@ COLLECTIONS: 5, + SEARCH: 6, + PREVIEW_EPUB: 7 Same as in the previous review.
I've tested "In Real Life - Cory Doctorow, Jen Wang" as an epub (as sold in the Kobo store), and that didn't display anything but a white page. I've also run into bug 767016, not sure whether that's a side effect of this patch, or something else.
(In reply to Bastien Nocera from comment #14) > I've tested "In Real Life - Cory Doctorow, Jen Wang" as an epub (as sold in > the Kobo store), and that didn't display anything but a white page. It's a 200 meg ePub. Let me know how you want me to send it to you.
Created attachment 328841 [details] [review] Fixed some issues with resource paths. I've fixed some resource loading issues. Uploading libgepub to the latest git version the epub "In Real Life - Cory Doctorow, Jen Wang" should work. Paths are relatives to the xhtml and the xhtml can be in the root of the zip file or in a directory, so I'm translating every relative path to a zip absolute path.
You can find here some prior art for 2 epubs (included) with two very different styles of display, from iBooks on iOS and OSX: https://people.gnome.org/~hadess/gnome-books-examples/ This gives you an idea of the kind of display, reflow, and controls that we would like to have for the epub functionality in gnome-books. Let me know if you have any questions about specific behaviour.
Review of attachment 328841 [details] [review]: Thanks Daniel. I need to find some time to try this in action, but here are some more comments. ::: src/embed.js @@ +353,3 @@ if (LOKView.isOpenDocumentFormat(doc.mimeType)) Application.modeController.setWindowMode(WindowMode.WindowMode.PREVIEW_LOK); + else if (doc.mimeType == 'application/epub+zip') Use the utility method from EPUBView ::: src/epubview.js @@ +31,3 @@ +const ErrorBox = imports.errorBox; +const MainToolbar = imports.mainToolbar; +const Documents = imports.documents; Alphabetical order @@ +40,3 @@ + +const Gepub = imports.gi.Gepub; +const Gio = imports.gi.Gio; These should probably be moved at the top with the other imports.gi.* @@ +91,3 @@ + this._doc = doc; + this._epubdoc = new Gepub.Doc({ path: f.get_path() }); + this._epubdoc.init(null); This can fail, so should be in a try/catch. But on second thought, it would be nice if the loading was plugged into the existing loadStarted/loadFinished/loadError machinery (even thought right now it's specific to Evince document models I believe). (Could also do it in a later refactor -- won't block the patchset on it.) @@ +195,3 @@ + }, + + go_next: function() { Should be goNext/goPrev @@ +216,3 @@ + // getting the basepath of the current xhtml loaded + path = path.split('/').slice(0,-1).join('/'); + current = this._replaceResources(String(current), path); I wonder if this should not happen in libgepub itself instead of here. It feels like every other client would need to do something similar, no? @@ +245,3 @@ + icon_size: Gtk.IconSize.MENU })); + this._prev.set_tooltip_text(_("Find Previous")); + this._prev.connect('clicked', Lang.bind(this, this._goprev)); Instead of connecting to the button signal, you should connect to the action activation signal to trigger this. Otherwise the code you have e.g. in the callback of the "activate" signal of the search entry will be ineffective. See also https://git.gnome.org/browse/gnome-documents/tree/src/preview.js#n116 @@ +275,3 @@ + }, + + _goprev: function() { Nitpick: could call this _goNext/_goPrev @@ +296,3 @@ + conceal: function() { + let fc = this._previewView.view.get_find_controller(); + fc.search('', WebKit2.FindOptions.CASE_INSENSITIVE, 0); You could call this._search('')?
Created attachment 329609 [details] [review] Done changes commented by Cosimo The most important different between this patch and the previous is the use of the new function in the libgepub [1], gepub_doc_get_current_with_epub_uris, that replaces local resource paths with a epub:// eschema. I've moved the code from gnome-documents, in javascript, to the libgepub, in C, and I'm using libxml and libsoup instead regular expressions and replaces, so I think it's much better now. [1] https://git.gnome.org/browse/libgepub/commit/?id=d86cbb44bb1a8f5d00a411b591183cae71fd0d6f
Thank you, Daniel. I worked a bit more on this in the weekend, and pushed https://git.gnome.org/browse/libgepub/log/?h=wip/cosimoc/fixes with more fixes. I also have a branch for gnome-documents with your patch and more fixes on top here: https://git.gnome.org/browse/gnome-documents/log/?h=wip/gepub Please review the new libgepub changes as a first step.
Hi Cosimo, I've merged your changes for libgepub. Yesterday I've been working in a widget for the libgepub that basically is a WebKitWebView linked with a GepubDocument. I'm doing in libgepub almost all that I was doing in gnome-documents. So in gnome-documents all will be easier. I've to add some high level methods to the gepub-widget, because currently, to change the page, you need to get the doc, change the current page and then reload the widget, and for the future I'll thinking of paginating and show two pages modes in the widget. [1] https://git.gnome.org/browse/libgepub/commit/?id=b6a93ab3598d7c8713d943e2d02ad565455bb1ad [2] https://git.gnome.org/browse/libgepub/tree/libgepub/gepub-widget.h
(In reply to Cosimo Cecchi from comment #20) > Thank you, Daniel. > > I worked a bit more on this in the weekend, and pushed > https://git.gnome.org/browse/libgepub/log/?h=wip/cosimoc/fixes with more > fixes. > I also have a branch for gnome-documents with your patch and more fixes on > top here: https://git.gnome.org/browse/gnome-documents/log/?h=wip/gepub > > Please review the new libgepub changes as a first step. I managed to get this when switching quickly between CBZ and epub on your branch: (org.gnome.Books:5238): Gjs-WARNING **: JS ERROR: Exception in callback for signal: load-finished: TypeError: this._toolbar.setModel is not a function Embed<._onLoadFinished@resource:///org/gnome/Books/js/embed.js:383 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 _emit@resource:///org/gnome/gjs/modules/signals.js:124 DocumentManager<._onDocumentLoaded@resource:///org/gnome/Books/js/documents.js:1406 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 DocCommon<.loadLocal/<@resource:///org/gnome/Books/js/documents.js:636 main@resource:///org/gnome/Books/js/main.js:47 run@resource:///org/gnome/gjs/modules/package.js:192 @/home/hadess/Projects/gnome-install/bin/gnome-books:6 I was trying to reproduce an assertion: ** (org.gnome.Books:4778): CRITICAL **: g_type_info_get_tag: assertion 'GI_IS_TYPE_INFO (info)' failed Maybe it's the same bug? Other than that, looks fine to me as a first pass. I'll have quite a few RFEs in terms of UI for libgepub though ;)
Daniel, I integrated your widget changes in the gnome-documents branch [1], but I also reworked the API significantly, especially around paging, to make it much nicer to use from gnome-documents [2]. I will look into the backtrace mentioned by Bastien above next, but in the meantime, please review the libgepub branch. [1] https://git.gnome.org/browse/gnome-documents/log/?h=wip/gepub [2] https://git.gnome.org/browse/libgepub/log/?h=wip/cosimoc/api-rework
(In reply to Cosimo Cecchi from comment #23) > Daniel, I integrated your widget changes in the gnome-documents branch [1], > but I also reworked the API significantly, especially around paging, to make > it much nicer to use from gnome-documents [2]. Patches look mostly fine to me. > I will look into the backtrace mentioned by Bastien above next, but in the > meantime, please review the libgepub branch. I think it's might be related to a leaked WebKit object, or a callback being made into a destroyed web view. > [1] https://git.gnome.org/browse/gnome-documents/log/?h=wip/gepub > [2] https://git.gnome.org/browse/libgepub/log/?h=wip/cosimoc/api-rework
I updated the wip/gepub branch [1] to contain a substantially reworked implementation of the preview classes, which should fix the issue mentioned by Bastien, plus a bunch of other things. Daniel, can I push the libgepub patches? I will merge the gnome-documents branch after that. https://git.gnome.org/browse/gnome-documents/log/?h=wip/gepub
(In reply to Cosimo Cecchi from comment #25) > I updated the wip/gepub branch [1] to contain a substantially reworked > implementation of the preview classes, which should fix the issue mentioned > by Bastien, plus a bunch of other things. > > Daniel, can I push the libgepub patches? I will merge the gnome-documents > branch after that. Yes, feel free to push to the libgepub.
(In reply to Cosimo Cecchi from comment #25) > I updated the wip/gepub branch [1] to contain a substantially reworked > implementation of the preview classes, which should fix the issue mentioned > by Bastien, plus a bunch of other things. > > Daniel, can I push the libgepub patches? I will merge the gnome-documents > branch after that. > > https://git.gnome.org/browse/gnome-documents/log/?h=wip/gepub Not sure about: https://git.gnome.org/browse/gnome-documents/commit/?h=wip/gepub&id=cd24b98431b6794a28e49f4ddd62541bd5ab2838 We don't have any way to get out of presentation mode in gnome-documents, on a touchscreen.
(In reply to Bastien Nocera from comment #27) > Not sure about: > https://git.gnome.org/browse/gnome-documents/commit/?h=wip/ > gepub&id=cd24b98431b6794a28e49f4ddd62541bd5ab2838 > > We don't have any way to get out of presentation mode in gnome-documents, on > a touchscreen. True, but this patch did not really change that. Either way, I removed that patch and pushed everything to master. I'm going to close this bug as FIXED - we can track other improvements in separate ones. Thanks once again Daniel for your work! Looking forward to more :-)
Great! for improvements just tell me what to do next and I'll work on that. I'm always in the irc too. Thank you for your help on this issue and in the libgepub.
Filed: https://bugzilla.gnome.org/show_bug.cgi?id=768001 https://bugzilla.gnome.org/show_bug.cgi?id=768002 https://bugzilla.gnome.org/show_bug.cgi?id=768003 https://bugzilla.gnome.org/show_bug.cgi?id=768004