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

feat: add Fabric support #700

Merged
merged 19 commits into from
Feb 12, 2023
Merged

Conversation

WoLewicki
Copy link
Contributor

@WoLewicki WoLewicki commented Nov 21, 2022

PR adding New Architecture support to the library 🎉

We at Software Mansion have been working on improving support for the new architecture for quite a while now. If you need help with anything related to New Architecture, like:

or you just want to ask any questions, hit us up on projects@swmansion.com


j-piasecki and others added 3 commits December 9, 2022 12:03
It seems like in some cases, Yoga (I think) will measure the view only along one axis first, resulting in `onSizeChanged` being called with either w or h set to zero. This in turn starts the rendering of the pdf under the hood with one dimension being set to zero and the follow-up call to `onSizeChanged` with the correct dimensions doesn't have any effect on the already started process.
The offending class is [DecodingAsyncTask](https://github.com/barteksc/AndroidPdfViewer/blob/d243b39377f19c3eae41e227067da254ebbf731b/android-pdf-viewer/src/main/java/com/github/barteksc/pdfviewer/DecodingAsyncTask.java#L68), which tries to get width and height of the `PdfView` in the constructor, and is [created](https://github.com/barteksc/AndroidPdfViewer/blob/d243b39377f19c3eae41e227067da254ebbf731b/android-pdf-viewer/src/main/java/com/github/barteksc/pdfviewer/PDFView.java#L279) as soon as the measurement is complete (relevant lines [here](https://github.com/barteksc/AndroidPdfViewer/blob/d243b39377f19c3eae41e227067da254ebbf731b/android-pdf-viewer/src/main/java/com/github/barteksc/pdfviewer/PDFView.java#L1525-L1528) and [here](https://github.com/barteksc/AndroidPdfViewer/blob/d243b39377f19c3eae41e227067da254ebbf731b/android-pdf-viewer/src/main/java/com/github/barteksc/pdfviewer/PDFView.java#L482-L484)), which in some cases may be incomplete as described above.
By delaying calling `super.onSizeChanged` until the size in both dimensions is correct we are able to prevent this from happening.
@wonday wonday merged commit b9cc6e3 into wonday:master Feb 12, 2023
@samjayhk
Copy link

Hello @WoLewicki , just want to thanks for your development work. I have tried to using master branch under Fabric mode, and it's all going well. But may I know if you have planned release current master branch as alpha version? Thanks!

@WoLewicki
Copy link
Contributor Author

@samjayhk thanks! I am not a maintainer of this library, so this question is rather to @wonday.

@WoLewicki WoLewicki deleted the @wolewicki/add-fabric branch February 27, 2023 09:57
@Bort-777
Copy link

Bort-777 commented Apr 12, 2023

Hello, @wonday .

Do you have any plans to release a new version of the library? I was thinking it might be a good idea to prepare a pull request for the minor version bump, let me know if there's anything we can do to help.

Thanks for your time!

@WoLewicki WoLewicki restored the @wolewicki/add-fabric branch April 14, 2023 12:27
@Sowed
Copy link

Sowed commented Apr 28, 2023

Hello @WoLewicki , just want to thanks for your development work. I have tried to using master branch under Fabric mode, and it's all going well. But may I know if you have planned release current master branch as alpha version? Thanks!


@WoLewicki, same sentiment here. Thank you for this PR. Had an app breaking on me with the PDF when I migrated to RN 0.71.7, installing master directly following this PR fixes it for me. So far so good.

Installed via

# terminal
yarn add https://github.com/wonday/react-native-pdf.git#master
# package.json
"react-native-pdf": "https://github.com/wonday/react-native-pdf.git#master",

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

Successfully merging this pull request may close these issues.

6 participants