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: Android implementation for Windows.Data.Pdf #9718

Conversation

workgroupengineering
Copy link
Contributor

@workgroupengineering workgroupengineering commented Sep 2, 2022

GitHub Issue (If applicable): Part of #1629 (This PR implements Android only), closes #1796

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

not impemented

What is the new behavior?

implementation for Windows.Data.Pdf on Android continuation of #1796

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@gitpod-io
Copy link

gitpod-io bot commented Sep 2, 2022

@workgroupengineering
Copy link
Contributor Author

workgroupengineering commented Sep 2, 2022

Commit b63abd5 will be dropped when PR #9623 is merged.

@workgroupengineering workgroupengineering marked this pull request as ready for review September 5, 2022 12:33
Copy link
Member

@jeromelaban jeromelaban left a comment

Choose a reason for hiding this comment

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

Very nice !

src/SamplesApp/SamplesApp.Droid/Properties/AssemblyInfo.cs Outdated Show resolved Hide resolved
src/Uno.Foundation/Rect.Android.cs Outdated Show resolved Hide resolved
src/Uno.UWP/Data/Pdf/PdfPage.Android.cs Outdated Show resolved Hide resolved
src/Uno.UWP/Data/Pdf/PdfPage.Android.cs Outdated Show resolved Hide resolved
src/Uno.UWP/Data/Pdf/PdfDocument.Android.cs Outdated Show resolved Hide resolved
src/Uno.UWP/Data/Pdf/PdfDocument.Android.cs Outdated Show resolved Hide resolved
@workgroupengineering workgroupengineering marked this pull request as draft September 7, 2022 13:06
@workgroupengineering workgroupengineering marked this pull request as ready for review September 9, 2022 08:20
@jeromelaban jeromelaban marked this pull request as draft September 9, 2022 12:24
@workgroupengineering workgroupengineering marked this pull request as ready for review September 9, 2022 14:35
@workgroupengineering workgroupengineering marked this pull request as draft September 12, 2022 08:11
@workgroupengineering workgroupengineering marked this pull request as ready for review September 12, 2022 12:44
@jeromelaban jeromelaban marked this pull request as draft September 12, 2022 14:17
@workgroupengineering
Copy link
Contributor Author

What did I forget to answer? I seem to have applied all the review comments.

@jeromelaban
Copy link
Member

The build was not passing at that time, we'll need to review the Sample from the APK now :)

@jeromelaban
Copy link
Member

I've checked quickly the samples app on a device, and it works! Very nice! It should be interesting to make sure that the ScrollViewer that show the document is able to scroll horizontally, otherwise it's cropped. I'm not too sure about the API, but is there a way to show the pdf originally scaled to the available view size?

@workgroupengineering
Copy link
Contributor Author

I've checked quickly the samples app on a device, and it works! Very nice! It should be interesting to make sure that the ScrollViewer that show the document is able to scroll horizontally, otherwise it's cropped. I'm not too sure about the API, but is there a way to show the pdf originally scaled to the available view size?

I have applied your suggestions. What are the doubts about implementing the API?

@workgroupengineering workgroupengineering force-pushed the features/android/PdfDocument branch 2 times, most recently from dc37b32 to 8929d15 Compare October 18, 2022 07:56
@github-actions github-actions bot added the area/automation Categorizes an issue or PR as relevant to project automation label Oct 26, 2022
@workgroupengineering
Copy link
Contributor Author

can someone restart the CI?

@workgroupengineering workgroupengineering marked this pull request as ready for review February 24, 2023 12:08
@workgroupengineering workgroupengineering force-pushed the features/android/PdfDocument branch 3 times, most recently from a1e526d to 9a1fbf7 Compare March 14, 2023 08:43
@jeromelaban
Copy link
Member

Very nice! It's getting there!

Two observations:

  • Resizing of the image does not work, though I don't know if it's a ScrollViewer issue on Uno's end, or if it's some missing configuration
  • The "Arrange to Width" does not seem to be working properly, where the width of the PDF is larger than the available size on screen. Is this expected?

@workgroupengineering
Copy link
Contributor Author

Very nice! It's getting there!

Two observations:

* Resizing of the image does not work, though I don't know if it's a ScrollViewer issue on Uno's end, or if it's some missing configuration

* The "Arrange to Width" does not seem to be working properly, where the width of the PDF is larger than the available size on screen. Is this expected?

Thanks for the review,
I double check with UWP

@workgroupengineering
Copy link
Contributor Author

I verified.

  • It seems to be a scrollviewer problem.
  • I added a test to verify that scaling works correctly.

@workgroupengineering workgroupengineering force-pushed the features/android/PdfDocument branch 2 times, most recently from a9e19ac to e37c3e8 Compare April 17, 2023 07:18
@workgroupengineering workgroupengineering force-pushed the features/android/PdfDocument branch 3 times, most recently from 3548f70 to e5e245e Compare May 8, 2023 07:40
@workgroupengineering workgroupengineering force-pushed the features/android/PdfDocument branch 2 times, most recently from a513dd9 to ad6ec72 Compare June 30, 2023 15:19
@jeromelaban
Copy link
Member

@workgroupengineering thanks for taking the time to rebase on master! We'll take a look again and see if there's anything else to adjust.

Copy link
Member

@jeromelaban jeromelaban left a comment

Choose a reason for hiding this comment

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

This is looking very good! Just one small adjustment and we can merge this!

src/Uno.UWP/Data/Pdf/PdfDocument.Android.cs Outdated Show resolved Hide resolved
@workgroupengineering workgroupengineering force-pushed the features/android/PdfDocument branch 2 times, most recently from bf11cf6 to 1c9931b Compare July 28, 2023 08:55
@jeromelaban jeromelaban merged commit 5597644 into unoplatform:master Jul 28, 2023
80 checks passed
@workgroupengineering workgroupengineering deleted the features/android/PdfDocument branch July 28, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/automation Categorizes an issue or PR as relevant to project automation area/build Categorizes an issue or PR as relevant to build infrastructure platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants