GNOME Bugzilla – Bug 782685
Show more info in epub preview, like number of chapters, estimated reading time, etc
Last modified: 2017-06-02 14:42:10 UTC
Currently there's no way to know the size of an epub doc in the preview, we've the chapter length with the vertical scroll, but we don't show anywhere the number of chapters. It could be great to have more information about the book in a bottom bar. Some readers adds the book percentage and a estimated reading time to the end of the book or to the end of the chapter. That could be added there too.
Created attachment 351961 [details] [review] Patch to show the current chapter and the number of chapters in epub preview
Review of attachment 351961 [details] [review]: Thanks, a few comments below. ::: src/epubview.js @@ +22,3 @@ const GdPrivate = imports.gi.GdPrivate; const Gepub = imports.gi.Gepub; +const Gtk = imports.gi.Gtk; Please keep the imports sorted alphabetically @@ +69,3 @@ + return new EPUBViewNavControls(this, this.overlay); + }, + Remove extra empty line @@ +138,3 @@ + }, + + return ""; I feel that it would be much easier if you just added a getter for the epubdoc from the preview object. That way, the nav controls subclass would have direct access to this information without the need to go through methods of the preview class, and you could save the "chapter-change" signal. @@ +228,3 @@ + margin: Preview.PREVIEW_NAVBAR_MARGIN, + valign: Gtk.Align.END, + Can you not just create a GtkBox here, instead of having an extra type? @@ +230,3 @@ + opacity: 0 }); + + createBarWidget: function() { Does not look like the variable needs to be saved in the "this" object, since it's only used in the scope of this function @@ +231,3 @@ + + this.label = new Gtk.Label({ label: this.preview.info_text() }); + let barWidget = new EPUBBarWidget({ orientation: Gtk.Orientation.HORIZONTAL, Ditto Also, should the range not go from 1 to the number of pages in the document? Why do you set it later instead?
Thank you for the review. I'm applying this changes to the patch, but there's some changes that I don't know the best way to solve. Comments below: (In reply to Cosimo Cecchi from comment #2) > Review of attachment 351961 [details] [review] [review]: > > Thanks, a few comments below. > > ::: src/epubview.js > [...] > > @@ +138,3 @@ > + }, > + > + return ""; > > I feel that it would be much easier if you just added a getter for the > epubdoc from the preview object. That way, the nav controls subclass would > have direct access to this information without the need to go through > methods of the preview class, and you could save the "chapter-change" signal. > I can move the method to the EPUBViewNavControls class, but there's a problem here, the _epubdoc doesn't exists when the createBarWidget is called, for this reason I've added the info_text method and because of this exists the return "". Maybe I can define a signal in the preview to notify the barWidget that the epubdoc is ready, and only then create the label and scale with the real epubdoc information. What do you think? > @@ +228,3 @@ > + margin: > Preview.PREVIEW_NAVBAR_MARGIN, > + valign: Gtk.Align.END, > + > > Can you not just create a GtkBox here, instead of having an extra type? > I've created the EPUBBarWidget class to set the css name to 'toolbar'. This apply some styles that in other case, I don't know how to add. I've done this based on the GdNavBar type, that inherites from GtkBox and sets the class css name to toolbar. I've tried to call "Gtk.Widget.set_css_name.call(barWidget, 'toolbar');" but that doesn't work, with the Gtk.Box widget directly we don't have round borders and padding, so the only method that I can found to do this is to create a subclass and change the css_name in the init method. > > Also, should the range not go from 1 to the number of pages in the document? > Why do you set it later instead? Yes, that's a weird thing that I'm doing, setting the range on each change... This is related with the previous problem, in the call to createBarWidget the epubdoc isn't initializated yet, so I created all widgets with default values, and then in the signal callback I'm changing the range with the real epubdoc information. Maybe a signal "epub-ready" in the preview widget would be a good idea to initialize this widgets only once, and I'll be able to create the label and level widgets with real values.
Created attachment 352074 [details] [review] Patch to show the current chapter and the number of chapters in epub preview This second patch fixes some issues commented by Cosimo and implements the barWidget label and scale widgets initialization with a epub-ready signal, all done in the EPUBViewNavControls widget. Maybe this is a better implementation that the previous patch.
Is there any way to do this after adding pagination?
(In reply to Bastien Nocera from comment #5) > Is there any way to do this after adding pagination? This is an "easy" change, pagination is a little hard. I think about this change as a previous step to be able to store the "chapter" percentage, to keep the real reading progress, not only the chapter as it is now. But if we'll have real pagination this should change a little. I any case, we've a simple patch here, I'll continue working in pagination to get it done as soon as possible. If you think this isn't needed yet, this patch will wait until we've the pagination and then we can review this again.
Thanks Daniel, I pushed a slightly amended version of your last patch to git master.
I was playing with this while shopping for a similar overlaid bar widget to offer zoom controls in gnome-photos. I noticed two things: (a) The bar doesn't have the GtkLabel and GtkScale immediately after an e-pub has finished loading. Those show up when I move the mouse. (b) The PreviewNavControls base class expects the bar to have a "hover" property. It was added to GdNavBar in commit 74dd32170d3aca20bbce1282bd24193807306603 to properly detect when the mouse pointer enters and leaves the bar. EPUBBarWidget doesn't have it, so leaving the mouse pointer hovering over the bar doesn't disable the auto-hide timer. Don't we need an I/O GdkWindow like GdNavBar to properly handle the events? I thought I'd mention this here since Daniel is already here. We can open further follow-up bugs, if that's preferred.
Created attachment 352682 [details] [review] epub: Showing the barWidget widgets on init
(In reply to Debarshi Ray from comment #8) > I was playing with this while shopping for a similar overlaid bar widget to > offer zoom controls in gnome-photos. I noticed two things: > > (a) The bar doesn't have the GtkLabel and GtkScale immediately after an > e-pub has finished loading. Those show up when I move the mouse. I've attached a patch to fix this. It's a simple line: barWidget.show_all().
Created attachment 353002 [details] [review] epub: Added an info bar in the epub preview Let's update the patch and its status to reflect reality.
Review of attachment 352682 [details] [review]: ::: src/epubview.js @@ +221,3 @@ epubdoc.connect('notify::page', Lang.bind(this, this._updatePage)); this._updatePage(); + this.barWidget.show_all(); While this is one way to fix this bug, the actual problem is that EPUBView:hasPages violates the semantics of that virtual function. It should return true only after the document has been loaded and has a non-zero number of pages.
Created attachment 353003 [details] [review] epubview: Don't blindly advertise the presence of pages
Created attachment 353004 [details] [review] epubview: Create the entire bar in one place
Created attachment 353005 [details] [review] epubview: Update the visibility of the controls when the page changes
Review of attachment 353003 [details] [review]: Looks good
Review of attachment 353004 [details] [review]: Looks good with one small style change ::: src/epubview.js @@ +245,3 @@ + barWidget.pack_start(this._level, true, true, 5); + this._level.connect('value-changed', Lang.bind(this, + barWidget.add(this._label); Move this one line up
Review of attachment 353005 [details] [review]: Looks good with a small style nit ::: src/epubview.js @@ +216,3 @@ this._level.set_range(1.0, this.preview.numPages); + this._epubdoc.connect('notify::page', Lang.bind(this, + function() { Also move this one line up
Comment on attachment 353003 [details] [review] epubview: Don't blindly advertise the presence of pages Pushed to master.
Created attachment 353086 [details] [review] epubview: Create the entire bar in one place Pushed after fixing the Lang.bind() formatting.
Created attachment 353087 [details] [review] epubview: Update the visibility of the controls when the page changes Pushed after fixing the Lang.bind() formatting.
Thanks for pushing the EPUB support forward, Daniel!