Use epub navigator fragment #148

johanpoirier · 2020-07-13T10:00:59Z

This is the implementation of the EpubNavigatorFragment following the slack discussion with @mickael-menu

I have still a lot of questions on this issue, some of them will be posted as comments.

r2-navigator/src/main/java/org/readium/r2/navigator/IR2Activity.kt

johanpoirier · 2020-07-13T10:02:46Z

r2-navigator/src/main/java/org/readium/r2/navigator/R2BasicWebView.kt


-    lateinit var activity: AppCompatActivity
-    lateinit var listener: IR2Activity
+    lateinit var activity: IR2Activity


Does the WebView should access the Activity ?

I don't think so. To properly decouple the web view from the Activity and the EpubNavigatorFragment (it could potentially be used with other fragments), you can add a R2BasicWebView.Listener interface with all the APIs you need to remove activity, fragment and listener.

Something like:

interface R2BasicWebView.Listener { fun onPageLoaded() fun onScroll() fun onTap() // To replace the `listener.toggleActionBar()` fun onProgressionChanged(Double) fun onHighlightActivated(id) fun onHighlightAnnotationMarkActivated(id) fun goForward(animated: Boolean) fun goBackward(animated: Boolean) }

Additionally, you can also add a readingProgression: ReadingProgression property, because we need it to do the scrollToPosition() JS call. You can also save the scroll mode set in setScrollMode() in a scrollMode variable, to not need access to the preferences.

With all this, the scrollLeft/Right() functions can be simplified with something like:

@android.webkit.JavascriptInterface open fun scrollRight(animated: Boolean = false) { uiScope.launch { // The whole supportActionBar piece of code should be done in the Fragment. listener.onScroll() if (scrollMode) { if (readingProgression == ReadingProgression.RTL) { this@R2BasicWebView.evaluateJavascript("scrollRightRTL();") { result -> if (result.contains("edge")) { listener.goBackward(animated = animated) } } } else { listener.goForward(animated = animated) } } else { if (!this@R2BasicWebView.canScrollHorizontally(1)) { listener.goForward(animated = animated) } this@R2BasicWebView.evaluateJavascript("scrollRight();", null) } } }

By the way you can check out the comments on this PR which is related to this: #112

johanpoirier · 2020-07-13T10:03:51Z

r2-navigator/src/main/java/org/readium/r2/navigator/R2BasicWebView.kt

    @android.webkit.JavascriptInterface
    fun centerTapped() {
-        listener.toggleActionBar()
+        listener.onTap(PointF(0F, 0F))


Same here : why is the WebView behaving in the name of the Activity ?

Yeah it shouldn't, you can use listener.onTap() described in the other comment, and then in the Fragment you can call the actual VisualListener.onTap() with the point. Until we have a real Point value, you can use the center of the Fragment as a reference, because using (0, 0) might trigger some issues if an app decides to do some action when tapping on the left border (e.g. going backward).

mickael-menu · 2020-07-13T11:50:29Z

r2-navigator/src/main/java/org/readium/r2/navigator/R2BasicWebView.kt


                    // Initialize a new instance of LayoutInflater service
-                    val inflater = activity.getSystemService(Context.LAYOUT_INFLATER_SERVICE) as LayoutInflater
+                    val inflater = fragment.requireActivity().getSystemService(Context.LAYOUT_INFLATER_SERVICE) as LayoutInflater


@johanpoirier For this we should be able to use context.getSystemService without having the fragment or activity as dependency.

johanpoirier

I have a larger issue with the locators. The table of content is not working anymore and the last reading position is not restored.

johanpoirier · 2020-07-13T14:34:17Z

r2-navigator/src/main/java/org/readium/r2/navigator/divina/R2DiViNaActivity.kt

        divinaWebView = findViewById(R.id.divinaWebView)
-        divinaWebView.activity = this
-        divinaWebView.listener = this
+        //divinaWebView.listener = this


I'm not doing anything for DiViNa right now.

Okay, that's fine as we'll tackle DiViNa soon with an updated version of the JS player.

johanpoirier · 2020-07-13T14:36:42Z

r2-navigator/src/main/java/org/readium/r2/navigator/pager/R2EpubPageFragment.kt

+//                        }
+//                    }
+//
+//                }


I don't know what to do with this block of code.

We need to keep it as this is what handles the restoration inside a resource.

That's probably why you see this issue you described in another comment:

The table of content is not working anymore and the last reading position is not restored.

What issue do you have with this piece of code?

If it's val epubNavigator = (webView.navigator as? R2EpubActivity), I think we can adapt it without changing too much stuff by using the EpubNavigatorFragment instead:

val epubNavigator = (webView.navigator as? EpubNavigator)

Alternatively, we can create a R2EpubPageFragment.Listener to expose the pendingLocator from the EpubNavigatorFragment, or reuse the one from R2BasicWebView.Listener and access it through webView.listener.pendingLocator.

R2EpubPageFragment.Listener is probably cleaner but I'll do more clean up later on in the EPUB navigator, so as long as the code is private, you can implement what's the most convenient.

So I changed the first line to that:

val epubNavigator = (webView.navigator as? EpubNavigatorFragment) val currentFragment: R2EpubPageFragment = (epubNavigator?.resourcePager?.adapter as R2PagerAdapter).getCurrentFragment() as R2EpubPageFragment

But this line is never true when a chapter reaches the end:

if (this@R2EpubPageFragment.tag == currentFragment.tag) {

I may need an interactive review if you have some time before the end of the week.

I may need an interactive review if you have some time before the end of the week.

Sure, I'll try to take a look tomorrow.

johanpoirier · 2020-07-13T14:37:30Z

r2-navigator/src/main/java/org/readium/r2/navigator/R2BasicWebView.kt

+    interface Listener {
+        fun onPageLoaded() {}
+        fun onScroll() {}
+        fun onTap(point: PointF): Boolean = false
+        fun onProgressionChanged(progression: Double)
+        fun onHighlightActivated(id: String) {}
+        fun onHighlightAnnotationMarkActivated(id: String) {}
+        fun goForward(animated: Boolean = false, completion: () -> Unit = {}): Boolean
+        fun goBackward(animated: Boolean = false, completion: () -> Unit = {}): Boolean
+    }


@mickael-menu did you have that in mind?

Yeah exactly 👍

johanpoirier · 2020-07-13T14:40:35Z

r2-navigator/src/main/java/org/readium/r2/navigator/epub/EpubNavigatorFragment.kt

+    }
+
+    override val readingProgression: ReadingProgression
+        get() = TODO("Not yet implemented")


I don't know how and where to set this variable.

You just need to report the one from the Publication, this is an alias to expose it to reading apps.

Suggested change

get() = TODO("Not yet implemented")

get() = publication.contentLayout.readingProgression

mickael-menu · 2020-07-14T06:52:38Z

r2-navigator/src/main/java/org/readium/r2/navigator/pager/R2PagerAdapter.kt

                        }
                    }
                Publication.TYPE.DiViNa -> TODO()
+                Publication.TYPE.PDF -> TODO()


@johanpoirier Publication.Type.PDF is not part of r2-shared at the moment, could you make sure that the PR builds with the alpha branches of the other repos?

I also have some issues in the test-app with some APIs that doesn't exist in the Web View:

r2-testapp/src/main/java/org/readium/r2/testapp/epub/UserSettings.kt: (375, 73): Unresolved reference: setScrollMode

johanpoirier · 2020-07-16T13:47:04Z

@mickael-menu does this PR needs more work?

mickael-menu · 2020-07-16T14:40:19Z

@johanpoirier

@mickael-menu does this PR needs more work?

I won't have the time to do a complete review before 2 weeks (holidays), but if it works in your app, it should be fine.

We're still missing the navigator for CBZ (ImageNavigatorFragment), if you want to complete the PR. Audiobooks will be handled differently, as we want to provide a chromeless navigator to leave full control over the UX for reading apps. So it doesn't make sense as a Fragment.

stevenzeck · 2020-07-25T16:57:49Z

Is this a small part of readium/kotlin-toolkit#176?

johanpoirier · 2020-07-26T17:32:17Z

Is this a small part of readium/kotlin-toolkit#176?

Yes it is.

paulocoutinhox · 2020-08-04T17:39:27Z

+1

Fix NavigatorDelegate Make the glue Listeners internals

…, otherwise it will not be used during configuration changes

Rename Navigator.VisualListener to VisualNavigator.Listener Fix shadowed members goLeft() and goRight() Implement goForward/goBackward for CBZ

mickael-menu

@johanpoirier Thanks again for this most-wanted contribution!

johanpoirier added 7 commits July 9, 2020 15:11

Add epub navigator fragment

d65a197

Use epub navigator fragment

805a214

Use epub fragment as navigator and IR2

f55b661

Add onTap trigger

c32fa55

Add onTap listener

0bb8ee5

Merge remote-tracking branch 'readium/alpha' into use-fragment

b3b37dc

Implements designed interfaces

c9c27ce

johanpoirier commented Jul 13, 2020

View reviewed changes

mickael-menu reviewed Jul 13, 2020

View reviewed changes

johanpoirier force-pushed the use-fragment branch from 4c92cfb to f2bee1d Compare July 13, 2020 14:28

Use R2BasicWebView.Listener

4407424

johanpoirier force-pushed the use-fragment branch from f2bee1d to 4407424 Compare July 13, 2020 14:30

johanpoirier commented Jul 13, 2020

View reviewed changes

Set reading progression from publication

1e1f566

mickael-menu reviewed Jul 14, 2020

View reviewed changes

johanpoirier added 3 commits July 15, 2020 08:53

Fix compilation errors

8ab9b12

Find epub nav fragment by tag instead of id

40febfa

Restore ToC activity result handler

897de8a

mickael-menu mentioned this pull request Jul 15, 2020

Fix launching the navigator readium/r2-testapp-kotlin#339

Merged

mickael-menu and others added 2 commits July 15, 2020 15:57

Fix currentLocator and readingProgression in R2EpubActivity

1d9312f

Remove useless pendingLocator

352b9fa

johanpoirier added 4 commits July 17, 2020 08:07

Expose resourcePager from EpubNavigatorFragment

34cd981

Unbind R2FXL fragment from IR2Activity

0cd1099

Use Cbz fragment

4e8bb97

Add Image fragment nav to nav fragment factory

5fdeb0c

johanpoirier marked this pull request as ready for review August 4, 2020 08:27

johanpoirier requested a review from aferditamuriqi as a code owner August 4, 2020 08:27

stevenzeck mentioned this pull request Jul 29, 2022

Evaluate replacing R2RTLViewPager and R2ViewPager with ViewPager2 readium/kotlin-toolkit#148

Closed

mickael-menu added 2 commits August 6, 2020 17:53

Add some documentation

f38eff6

Fix NavigatorDelegate Make the glue Listeners internals

FragmentFactory needs to be initialized before calling super.onCreate…

0e91cae

…, otherwise it will not be used during configuration changes

mickael-menu mentioned this pull request Aug 7, 2020

Exposing Fragment instead of Activity readium/kotlin-toolkit#176

Closed

mickael-menu added 4 commits August 7, 2020 12:45

Forward legacy IR2Activity listeners

56cad0a

Fix reading progression

517919b

Members visibility fixes

3e2d3a4

Rename Navigator.VisualListener to VisualNavigator.Listener Fix shadowed members goLeft() and goRight() Implement goForward/goBackward for CBZ

Add mention of the new Fragments in the Changelog

bb19ea7

mickael-menu approved these changes Aug 7, 2020

View reviewed changes

mickael-menu merged commit f374c0e into readium:alpha Aug 7, 2020

	get() = TODO("Not yet implemented")
	get() = publication.contentLayout.readingProgression

Use epub navigator fragment #148

Use epub navigator fragment #148

Uh oh!

Conversation

johanpoirier commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

johanpoirier Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanpoirier Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanpoirier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanpoirier commented Jul 16, 2020

Uh oh!

mickael-menu commented Jul 16, 2020

Uh oh!

stevenzeck commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johanpoirier commented Jul 26, 2020

Uh oh!

paulocoutinhox commented Aug 4, 2020

Uh oh!

mickael-menu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johanpoirier commented Jul 13, 2020 •

edited

Loading

johanpoirier Jul 13, 2020 •

edited

Loading

johanpoirier Jul 13, 2020 •

edited

Loading

stevenzeck commented Jul 25, 2020 •

edited

Loading