Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement minimalistic EPUB reader #1263

Merged
merged 15 commits into from
May 30, 2020
Merged

Implement minimalistic EPUB reader #1263

merged 15 commits into from
May 30, 2020

Conversation

itegulov
Copy link
Contributor

Changes
Introduces an epubjs powered EPUB reader which supports navigation through pressing left/right arrow keys and closes on ESC. Similarly to photo player, EPUB reader is implemented through a fullscreen dialog component.

@itegulov
Copy link
Contributor Author

@dkanada I have addressed your comments from jellyfin/jellyfin-plugin-bookshelf#10 (comment) and made book player to use dialogue instead of a separate page.

@sonarcloud
Copy link

sonarcloud bot commented May 18, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
4.9% 4.9% Duplication

@dkanada
Copy link
Member

dkanada commented May 18, 2020

@MrTimscampi has anyone figured out these plugin classes for the ES6 migration? I spent a bit of time looking into it while I was messing with epub.js but got hung up on plugin quirks.

@dkanada
Copy link
Member

dkanada commented May 18, 2020

@itegulov I managed to figure out the ES6 conversion, do you mind if I push a commit here for the book player? The two things I think we need for an MVP are progress reporting and mobile support in the form of swipe controls. The easiest method would be selectively applying continuous mode for mobile devices, how does that sound? We have a utility to set the layout that can be used for that.

@itegulov
Copy link
Contributor Author

itegulov commented May 19, 2020

@dkanada Yeah, go ahead. Continuous mode for mobile devices sounds good although I am worried if we will have problems with determening consistent progress across different devices. I don't know if there is a notion of "absolule" position in epubjs as opposed to page number which is relative to your screen's resolution/your reading mode.

@itegulov
Copy link
Contributor Author

I went ahead and added table of contents implemented via a pop-up dialogue window. It would look much better if it was a sidebar, but better than nothing. I also tested it on my phone and it works fine there as well. If you need anything else from my part to reach an MVP please feel free to say so.
latest-screenshot

@dkanada
Copy link
Member

dkanada commented May 19, 2020

Nope this looks like more than enough to me! I'll spend a bit looking into the progress reporting when I get my laptop working again and also convert the player to ES6 but the rest of the code looks fine.

@Artiume
Copy link
Contributor

Artiume commented May 20, 2020

Tested this on Chrome Mobile. I was able to select a chapter, but I was unable to change pages due to swiping being unresponsive.

@heyhippari heyhippari mentioned this pull request May 20, 2020
@itegulov
Copy link
Contributor Author

@Artiume yeah swiping does not work on mobile devices right now. I think @dkanada is working on applying continuous flow for mobile devices so you wouldn't need swipes once this is done.

@dkanada
Copy link
Member

dkanada commented May 22, 2020

@ferferga the scroller style changes have caused an issue for the book player, but I'm not sure how you want to solve it. Maybe updating the style in the contstructor and then again once the dialog is closed to revert to the old one?

@dkanada dkanada added this to In progress in Release 10.6.0 via automation May 22, 2020
@ferferga
Copy link
Member

@dkanada As specified in OP, this is a dialog, meaning that content that it's behind it is still visible (but behind the book reader). Same situation happens with the video player while loading.

Take a look at how this was solved in the photo player. As there is no way to create an event listener that "rules them all", even future dialog components, you must manually do a class switch when opening and closing the dialog.

That's why I thought originally about creating a function in a scrollbarhelper file called switchScrollbar, so future developers of dialogs will find how this is accomplished more easily. Otherwise, we should remind them of this if they forgot.

There is also a really cool thing: if the dialog is closed unexpectedly due to a crash of the module or because the scrollbar switching hasn't been properly handled by the dev in all situations, the scrollbar will reappear once the user changes between pages in the UI, so in any case the user will be stuck without the possibility to scroll.

@heyhippari
Copy link
Contributor

That's why I thought originally about creating a function in a scrollbarhelper file called switchScrollbar, so future developers of dialogs will find how this is accomplished more easily. Otherwise, we should remind them of this if they forgot.

Hm, why not put it directly in the dialog opening code? If the size wanted is fullscreen, apply the fix and continue.

@heyhippari heyhippari added the playback This PR or issue mainly concerns playback label May 23, 2020
@heyhippari heyhippari self-requested a review May 23, 2020 09:21
src/components/bookPlayer/plugin.js Outdated Show resolved Hide resolved
@heyhippari
Copy link
Contributor

This actually needs format checking in addition to item type in canPlayMediaType, as it will then show up as a player for CBZ, CBR, AZW and other formats of the book type.

dkanada and others added 2 commits May 27, 2020 21:12
Co-authored-by: Julien Machiels <julien.machiels@protonmail.com>
@itegulov
Copy link
Contributor Author

@MrTimscampi

This actually needs format checking in addition to item type in canPlayMediaType, as it will then show up as a player for CBZ, CBR, AZW and other formats of the book type.

Good catch! I could not figure out a way to stop play button to show up for other book types so I added an error dialog saying "This book type is not supported yet" in case someone opens a non-epub book with book player.

I have also fixed a few bugs related to book player not removing event listeners correctly on closure.

@itegulov
Copy link
Contributor Author

@dkanada just wanted to check in if you are still working on mobile support and progress report? If not, I'll be free on weekend and can look into it.

@dkanada
Copy link
Member

dkanada commented May 28, 2020

I haven't had the time yet, you can definitely tackle it if you want. I looked into it a little, and you'll probably want to generate the CFIs for each book (and maybe cache them) and then save progress on the server as a percentage maybe. Two other features I was looking into were a continuous mode toggle next to the ToC and optional text or background color options. The latter was requested by someone else, but a "realistic" papery beige or a dark mode would be quite nice.

@heyhippari
Copy link
Contributor

I could not figure out a way to stop play button to show up for other book types so I added an error dialog saying "This book type is not supported yet" in case someone opens a non-epub book with book player.

That's pretty much what I had in mind.

The idea is to use this as the player for all book types, calling a different library depending on the book format.

So ideally, setCurrentSrc would work as it does in htmlVideoPlayer (By checking capabilities and selecting a player, so, in this case, checking book format and initializing the player).

Something like setSrcWithEpubjs that would create the mediaElement for EpubJS and initialize it.

The idea being to make it rather generic, so that we can plug in more setSrcWithXYZ into it so support more book formats (I'm thinking pdf.js and comic-reader for now, which I plan to handle once this base PR is merged :) )

Imo, automatic continuous mode on mobile, and progress report are all that is needed for a merge. Further improvements can be done in subsequent PRs.

@sonarcloud
Copy link

sonarcloud bot commented May 29, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
4.8% 4.8% Duplication

@heyhippari
Copy link
Contributor

heyhippari commented May 29, 2020

@itegulov For format checking, I just figured out that we have a this:

YoutubePlayer.prototype.canPlayItem = function (item) {
    // Does not play server items
    return false;
};

It's used in PlaybackManager to check if a player can handle some items.

We can use this to check if the item is an EPUB directly, without any changes to the player (And remove the checks we have in this currently).

Edit: Here's the one used in the upcoming CBZ/CBR reader, for reference:

    canPlayItem(item) {
        if (item.Path && (item.Path.endsWith('cbz') || item.Path.endsWith('cbr'))) {
            return true;
        }
        return false;
    }

Release 10.6.0 automation moved this from In progress to Reviewer approved May 30, 2020
Copy link
Member

@dkanada dkanada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can address any future improvements in other pull requests :) the code looks very clean and this is a great feature!

@dkanada dkanada merged commit 9cae072 into jellyfin:master May 30, 2020
Release 10.6.0 automation moved this from Reviewer approved to Done May 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
playback This PR or issue mainly concerns playback
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants