Skip to content
This repository has been archived by the owner on Jul 29, 2022. It is now read-only.

Added support for page events from fragment #209

Conversation

chrfalch
Copy link
Contributor

When using fragments the only way of being notified of a page change is to observe the currentLocator property on the fragment.

There is a small problem with this for epub fragments. The current locator changes multiple times when navigating to a Locator. This is caused by the fact that the viewPager must load the requested chapter (which causes a currentLocator update telling us that we are on the first page in the chapter) before it can navigate to the correct position in the chapter (which again causes an update to the currentLocator).

In the fragment there is a mechanism for notifying the parent activity when the page has changed. The problem with the current implementation is that it assumes that this is an R2EpubActivity - which is not true when using a fragment to build a custom UX around the reader.

This PR fixes this by removing the hard dependency on R2EpubActivity in the fragment. A new interface, IR2Listener, has been split out from the IR2Activity interface and replaces the activity member in the R2EpubNavigatorFragment. The listener member now supports this new interface and will call its corresponding methods when the active page changes.

chrfalch added 2 commits June 10, 2021 23:13
Instead of observing the currentLocator and receive multiple location changes
when a new chapter is first loaded and then navigates to the requested location,
the EpubNavigatorFragment now calls its listener when the inner webview
notifies about a page change.

The listener has been updated with members for notifying page change/load/end
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

It seems to have broken highlights in the test app:

  1. Create an highlight
  2. Close then reopen the book at the same location
  3. Notice that the highlight is not redrawn

I think it's because in the Test App we set explicitly the EpubNavigatorFragment's listener to a different object (EpubReaderFragment) while still subclassing the R2EpubActivity and relying on it being the recipient for onPageLoaded(), etc. The test app is in an uncomfortable state because it is taking advantage of some new stuff from EpubReaderFragment while still using R2EpubActivity for the highlights. It's a mess.

So to unlock the situation, I suggest going back to one of your earlier suggestion:

class EpubNavigatorFragment(
    ...
    internal val listener: Listener? = null,
    internal val paginationListener: PaginationListener? = null,
) {
    interface PaginationListener {
        fun onPageChanged(pageIndex: Int, totalPages: Int, url: String) {}
        fun onPageEnded(end: Boolean) {}
        fun onPageLoaded() {}
    }

    override fun onPageLoaded() {
        if (paginationListener != null) {
            paginationListener?.onPageLoaded()
        } else {
            r2Activity?.onPageLoaded()
        }
    }
    
}

Also since this is a new API, we can maybe clean up a bit the methods. For example adding the locator, and I don't think the url in onPageChanged is useful.

I'm not even sure what onPageEnded(end: Boolean) means.

@chrfalch
Copy link
Contributor Author

chrfalch commented Jun 14, 2021

Seems like a good solution - I'll see if I can update this later tonight. Maybe we should just create a new callback for locator changes and drop the pageEnded method? Something like:

interface PaginationListener {
    fun onPageChanged(locator: Locator) {}
    fun onPageLoaded() {}
}

Maybe the name of both the interface and the callback is a bit misleading with the locator parameter, what do you think?

@mickael-menu
Copy link
Member

Okay for removing onPageEnded.

I think we should keep the pageIndex (maybe 0-indexed?) and totalPages because this is some info that is sometimes requested by integrators and was available with R2EpubActivity.

Also if we have only Locator, then I think this API is confusing when compared with currentLocator.observe().

chrfalch and others added 3 commits June 15, 2021 09:43
- Removed the IR2Activity
- Kept the r2Activity notifications to avoid any compatibility issues
- Added local interface PaginationListener in EpubNavigatorFragment
- Added callbacks on paginationListener
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Looking good, thanks. 👍

I didn't add this to the changelog as I don't necessarily want to advertise this legacy API. We'll need to come up with a more integrated API for Navigator2.

@mickael-menu mickael-menu merged commit cf04f97 into readium:develop Jun 18, 2021
@chrfalch
Copy link
Contributor Author

Thanks! :) :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants