[UI / PDF] Full screen view for PDFs (iOS >= 13)

[mneuwert]

Description

• Tap to trigger full screen view
• Thumbnails positioned based on vertical size class after rotating the device to give the displayed document more screen real estate.
• Some other small UI tweaks, but all the goodness only available for iOS 13.x and higher though

Related Issue

#428

Motivation and Context

Better UX when viewing PDFs

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

QA

Test plan: https://github.com/owncloud/QA/blob/master/Mobile/iOS-app/Version%2011.5/PDF%20View%20Improvements.md

• (1) Some glitches in thumbnails [UI / PDF] Full screen view for PDFs (iOS >= 13) #813 (comment) [FIXED]
• (2) Thumbnails are off in iPadOS13 [UI / PDF] Full screen view for PDFs (iOS >= 13) #813 (comment) [FIXED]
• (3) Progress bar does not run smoothly [UI / PDF] Full screen view for PDFs (iOS >= 13) #813 (comment) [WONT FIX]

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Michael Neuwert seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[hosy]

@mneuwert thank you for this nice feature. Before I will start with code review I have some findings, which may be fixed before:

• Status bar color should be changed to .darkContent in fullscreen view
• Thumbnail icons on iPad should always shown on the right side not on bottom (in portrait and landscape mode)
• on iOS 12 fullscreen is not working

[mneuwert]

@mneuwert thank you for this nice feature. Before I will start with code review I have some findings, which may be fixed before:

• Status bar color should be changed to .darkContent in fullscreen view
• Thumbnail icons on iPad should always shown on the right side not on bottom (in portrait and landscape mode)
• on iOS 12 fullscreen is not working

• Valid point about status bar I guess..
• Thumbnail icons positioning is rather bound to size class rather than to particular HW. So, please be aware that in multitasking mode, the app could just look like an iPhone app.
• If I enable this stuff on iOS 12, it is badly crashing all the time.. unfortunately. Thumbnails view seems to be very sensitive to layout changes there. It ends up in some "unrecognised selector sent to blah" exception quite often.

[hosy]

@mneuwert thank you for this nice feature. Before I will start with code review I have some findings, which may be fixed before:

• Status bar color should be changed to .darkContent in fullscreen view
• Thumbnail icons on iPad should always shown on the right side not on bottom (in portrait and landscape mode)
• on iOS 12 fullscreen is not working

• Valid point about status bar I guess..
  Cool!

• Thumbnail icons positioning is rather bound to size class rather than to particular HW. So, please be aware that in multitasking mode, the app could just look like an iPhone app.
  Yes, but currently on the iPad the thumbnail view is always on bottom. But for me it makes more sense to put in on the right side, when a larger size class is active.

• If I enable this stuff on iOS 12, it is badly crashing all the time.. unfortunately. Thumbnails view seems to be very sensitive to layout changes there. It ends up in some "unrecognised selector sent to blah" exception quite often.

At the moment we shipping the app with iOS 12 support, so we need a running workaround for this version.

[hosy]

I had a short call with @mneuwert about the following findings:

• Fullscreen support only for iOS >= 13
• show thumbnail view (bottom or right) in dependency on current size class

[mneuwert]

@hosy regarding status bar style... I tried pretty much everything.. And I didn't manage to get these status bar appearance callbacks being called... childForStatusBarStyle, preferredBarStatusStyle.. Not sure what's wrong or may be something is broken. Tried to build up the correct chain between parent and child (DisplayHostViewController -> PDFViewController) but somehow it is not working at all. Also explicitly set a property in Info.plist to allow view controller based status bar management... Any ideas?

[hosy]

@mneuwert sometimes the PDF view does not render the PDF file because of this crash:

 [Unknown process name] Error: this application, or a library it uses, has passed an invalid numeric value (NaN, or not-a-number) to CoreGraphics API and this value is being ignored. Please fix this problem.
2020-10-21 20:46:08.275657+0200 ownCloud[67923:14333676] [Unknown process name] Backtrace:
  <_ZN12_GLOBAL__N_118transform_is_validEPK17CGAffineTransform+23>
   <CGPathCreateWithRect+23>
    <CGContextAddRect+218>
     <op_re+201>
      <pdf_scanner_handle_xname+130>
       <CGPDFScannerScan+486>
        <CGContextDrawPDFPageWithDrawingCallbacks+3654>
         <CGContextDrawPDFPageWithProgressCallback+47>
          <-[PDFPage _drawWithBox:inContext:withRotation:isThumbnail:withAnnotations:withBookmark:withDelegate:]+669>
           <-[PDFPage drawWithBox:inContext:isThumbnail:]+88>
            <-[PDFView drawPage:toContext:]+74>
             <-[PDFTilePool _renderTileForRequest:]+732>
              <_dispatch_call_block_and_release+12>
               <_dispatch_client_callout+8>
                <_dispatch_lane_serial_drain+796>
                 <_dispatch_lane_invoke+439>
                  <_dispatch_workloop_worker_thread+882>
                   <_pthread_wqthread+290>                    <start_wqthread+15>

[hosy]

@mneuwert maybe one reason, why the statusbar color cannot be change, is the overlaying ThemeNavigationController which handles the global preferredStatusBarStyle.

Another note: Currently on iOS 12 no thumbnail view is visible.

[mneuwert]

@hosy Good point about the status bar style.. Do you have more details on the crash eventually? Remember you said it happens with the user manual for OC. May be reproduction steps?

[mneuwert]

@hosy can you please check if the crash is reproducible with latest changes?

As for status bar, I am not sure.. probably we need to think a bit more how it is handled globally considering also current UI mode: dark / light. Tried many things, but somewhere the chain is broken: navigation controller -> display host parent -> viewer child..

[hosy]

@jesmrec probably the PDF crash depends on the an Apple bug in PDFKit. If you run into this problem, please check if you can reproduce it.

[hosy]

(1) [FIXED]

@mneuwert the thumbnail view does not appear immediately on all devices. After a rotation it appears.
Furthermore in landscape view on the iPad it should be shown on the right, not on the bottom.

Edit by @jesmrec: reproducible in iOS14, iPadOS14 and also iOS13 (iPhone). With iPadOS13, check report (2)

[jesmrec]

(2) [FIXED]

Probably, related to (1)

Reproducible with an iOS13 iPadAir

1. Download a PDF file, not opened before

Current: After downloading, file is displayed and thumbnails do not appear. Changing orientation, they do not appear either. Closing the file and reopening, thumbnails appear.

Expected: After first downloading, thumbnails appear.

iPadOS13, iPadAir.

NOTE: same behaviour reproducible in iOS12

[jesmrec]

(3) [WONT FIX]

This one is a regression, not a matter of this PR. Just for getting some information.

The progress bar just above the page indicator:

behaves in a strange way. It seems to have only three positions (on the left, on the right and in center), instead of moving smoothly through the screen width. Is there something to do?

As i clarified above, this is not a matter of this PR. In case it could be fixed, it's not a problem to do it in a separate ticket.

[mneuwert]

Re (3)

• The scrollview which displays the scroll bar is entirely managed by PDFView which comes from Apple's PDFKit. So we can't really fix it.
• I found another issue.. The white horizontal stripe below the scroll bar. It shouldn't have been there. Now removed.

[mneuwert]

Re (1) & (2)

Should be fixed with latest commit

[jesmrec]

(1) and (2) fixed

[jesmrec]

Approved on my side. There are some conflicts before merging, could you take a look?

[mneuwert]

@jesmrec Fixed merge conflicts