[Feature Request] PDF support

[jspizziri]

Add the ability to view PDF files in addition to the other currently supported formats.

[jspizziri]

@adesege, I'd recommend starting here.

Most of the library's implementation has been taken straight from the TestApps in the swift-tooklit and kotlin-tookit.

For instance, if you compare the ios/Reader structure in this library to the TestApp/Sources/Reader structure in the swift-tooklit, you'll see that they are nearly identical. The key difference is that I've stripped out everything that the the library doesn't currently support.

So the task here would be to port over the code in the Reader/PDF folder (but obviously for both Android and iOS).

Does that make sense?

[adesege]

@jspizziri I will compare the repos and add PDF functionality. I will raise a PR as soon as I have something tangible to show.

Thanks.

[adesege]

Hi @jspizziri,

I've been able to add PDF support for Android. However, there seems to be a performance issue with my implementation. You can check the changes here https://github.com/5-stones/react-native-readium/compare/main...adesege:react-native-readium:add-pdf?expand=1

Let me know what you think.

Thanks.

[jspizziri]

@adesege can you create an MR for it? We'll obviously want to make sure the performance issue gets resolved (you're not using an android emulator are you?). Additionally, we'll need an iOS implementation as well.

[jspizziri]

@adesege any updates on your PR?

[adesege]

Hi @jspizziri, apologies for the delayed response. I've been quite busy with work.

I just cloned the repo but I'm unable to get the example app working. The app gets installed on the device but it doesn't open the book.

I've tried creating a new example app and just copy the javascript code to it but it didn't make any difference.

[jspizziri]

@adesege I'm not able to reproduce your issue with the example app. Can you add some more info on the steps you've taken?

[adesege]

@jspizziri It started working again some hours ago. I didn't change anything.

The issue I was having with the example app was that the metro bundler was not able to reload the app on a device.

Thanks

[adesege]

Going through the codebase, I can see there has been some skeleta work to support PDF. I don't know if that has always been there, though.

• android/src/main/java/com/reactnativereadium/utils/FragmentFactory.kt
• ios/Reader/ReaderModule.swift

Are the changes in those files still relevant? If yes, where do you suggest I place this block of code

supportFragmentManager.fragmentFactory = CompositeFragmentFactory(
      EpubNavigatorFragment.createFactory(publication, baseUrl, initialLocator, this),
      PdfNavigatorFragment.createFactory(publication, initialLocator, this)
  )

I'm familiar with Java so I will be starting with Android first.

[jspizziri]

@adesege most of those core files were lifted from the readium example apps that I linked to above. I simply reworked it to work within the RN context and what was needed for this project. The linked projects do support PDF so most of the underlying code should be ready to adopt it. For instance, on Android the bulk of the work will probably be porting over the PdfReaderFragment.kt.

Does that make sense at all?

[adesege]

@jspizziri That makes sense. While looking at the test-app, I will need to update the android library to 2.3.0 because the test-app uses a PDF adapter as per https://github.com/readium/kotlin-toolkit/blob/2.3.0/docs/migration-guide.md#pdf-support.

[adesege]

@jspizziri Here's the upgrade PR #38. I'm currently stuck at migrating to the new Preferences API as per https://github.com/readium/kotlin-toolkit/blob/2.3.0/docs/migration-guide.md#upgrading-to-the-new-preferences-api.

Here's the commit of the work done so far on the Preferences API 977d6ec.

Since settings are set using UserPreferences data class, I'm finding it difficult to convert the hash from the user to the data class.

android/src/main/java/com/reactnativereadium/preferences/UserPreferences.kt is copied directly from the Test App and I'm supposed to make it work with the library.

I'll appreciate any insights or help.

[adesege]

To test, you can either checkout to the second to the last commit in the branch or delete android/src/main/java/com/reactnativereadium/preferences/UserPreferences.kt. The application should build and behave as normal.

The last step of the migration is to migrate UserSettings to UserPreferences.

[jspizziri]

@adesege

I will need to update the android library to 2.3.0

It would actually be good to upgrade to 2.5 as that's the version I just upgraded to on iOS (technically pointing at the develop branch there as there's a commit that's needed in it to properly function on iOS).

I'm currently stuck at migrating to the new Preferences API as per

I had a conversation with the readium maintainer on this here, and based on what he said I'm not sure if we need to upgrade. With that said, it would be good to update as we need to eventually. I'll take a look as soon as I can and see if I have any insights.

Great work so far! Really appreciate your effort on this!

[jspizziri]

@adesege , can't we do something similar to the old updateSetingsFromMap ?

It follows the following procedure:

1. Iterates over the keys of a map.
2. Checks for the existence of that key in the preferences.
3. Sets that property appropriately.

[adesege]

Thanks for the feedback, @jspizziri. Version 2.3.0 is the latest version in the migration guide. I'm not sure the Android version has a 2.5.

The application works without the UserPreferences migration. Although, I observed that dark mode is not automatically set on initial load like before. However, performing the migration will get the library to be close to the Test App which will help when migrating features from the Test App to the library.

The way I thought of doing it is to create a DTO that maps settings from the user to member variables which will then be converted to EpubPreferences and passed to the navigator.submitPreferences method. However, one of the challenges I'm facing is that fontFamily property expects FontFamily class which doesn't have a method to covert the value from the user to that class.

[jspizziri]

Version 2.3.0 is the latest version in the migration guide. I'm not sure the Android version has a 2.5.

👍

However, performing the migration will get the library to be close to the Test App ...

Yeah, I'm all for migrating if you're up for it! It will be a long-term benefit.

one of the challenges I'm facing is that fontFamily property expect

We'll probably need to write a small switch function that maps string values to the standard FontFamily types. The default of the switch (if an invalid string value is passed should just be something sane like serif.

To push it a bit further we could also expose those string values as constants to the JS side via a technique like this:

1. Expose the string constant values in kotlin
2. Expose the constants in JS

In the event we did expose those strings however we'd also want to mirror that on the iOS side.

Any of that helpful?

[adesege]

That was helpful, thanks. I will work on the Android first. I'm not familiar with Swift or Objective-C 😄