Implement page rotation for skrewed PDFs

[dcaliste]

This PR implement page rotation as discussed in #110. This is intended for documents that have a faulty page orientation, like scans. One can rotate the device (keeping the orientation with a finger on screen during rotation) but then the scroll direction becomes horizontal, or reversed.

The PR is divided into three commits:

• the first one introduce the rotation logic into the rendering part. It is nice to have it, because it is abstracting the page coordinates and the canvas coordinates. Later we may base a side by side rendering on this for instance.
• the second one, I don't like. It is adding a button in the toolbar to rotate. I didn't find any better solution.
• the third one save the orientation in the setting database to avoid rotating again the same document.

If you have any comment on a better way to introduce the rotation action in the UI, I would be happy to change the second commit. I've tried a pinch to rotate gesture, but it's very erratic and easily triggered unintended because mixing with the pinch to zoom feature.

[dcaliste]

As discussed on TJC, I've added a commit to implement the rotate action as a push up menu entry attached to the toolbar.

This may also become a nice place to add new actions related to the view of the document, like later side by side…

@rainemak or @pvuorela what's your opinion?

[pvuorela]

Nice! Tested and had some discussion with Martin about the design. I'm still a bit pondering how much needed feature this is, but Martin seemed ok having it per se. Myself thinking that user can also lock the orientation and turn the screen for better viewing? For comparison Android Acrobat doesn't provide this, but desktop applications do have the action.

Code-wise it doesn't seem too intrusive which is good. But testing rotation a couple times I'm getting Documents crashing quite easily now.

As discussed on TJC, I've added a commit to implement the rotate action as a push up menu entry attached to the toolbar.
This may also become a nice place to add new actions related to the view of the document, like later side by side…

We were thinking if the toolbar should be like one in Browser and have a button for expanding it. Could then have the actions as buttons and rotation to both directions.

For another thing we should get rid of the split view UI (i.e. tap to expose upper pulldown part) and it could make sense to get design for that before doing more action containers.

[pvuorela]

We don't commonly have all-caps enums, but rather CamelCased ones. Also something like counterclockwise or inverted for naming?

[dcaliste]

That's right, I'm going to change and CounterClockWise seems more Englisk indeed.

[dcaliste]

Case changed.

[dcaliste]

Myself thinking that user can also lock the orientation and turn the screen for better viewing? For comparison Android Acrobat doesn't provide this, but desktop applications do have the action.

I fully agree with this reasoning. You need the rotation for desktop because you cannot rotate the screen easily, for handhold devices, you can lock and rotate. But as mentioned in the TJC post, this breaks the flick direction. If you have wrongly oriented scanned pages, like portrait oriented for landscape for instance, you can lock rotate the device, but flick direction is then from thr right, instead of from the top, which would be the natural direction.

In addition, even if the use case is discarded, I would like to make the first commit go in, after review and corrections of course, because it disentangles the canvas geometry from the pdf page geometry, in the sense, that canvas width is not anymore considered as the page width, which will allow to implement side-by-side view for PDFs that suggest it.

We were thinking if the toolbar should be like one in Browser and have a button for expanding it.

I thought about this, but discarded it because I didn't find any action button to remove to put a new expand one. Because doing this means that one of the current action will become a two tap action. Of course design may have further ideas.

For another thing we should get rid of the split view UI (i.e. tap to expose upper pulldown part) and it could make sense to get design for that before doing more action containers.

I had some years ago now, when design suggested it first, tested a patch to implement this, see my title branch for it. It was looking nice, but the action to go back on document list was totally broken with zoomed document and I gave up at that time. Maybe a good moment to revive all these commits (it was quite a big amount of modifications indeed).

Code-wise it doesn't seem too intrusive which is good. But testing rotation a couple times I'm getting Documents crashing quite easily now.

That's bad, I'm going to test further and follow your remarks.

[dcaliste]

@pvuorela may I ask you in which conditions it is crashing? I've tried this morning the following without any issues:

• rotate ten times in a row quickly on a zoomed out document ;
• do the same on a zoomed in document ;
• rotate document and then flick around ;
• rotate document and then rotate the device.

[pvuorela]

I'm just opening a file and then triggering rotate a few times.
Now attached gdb and tried again:

Thread 4 (Thread 0xaeff4390 (LWP 17230)):
#0 0x00000000 in ?? ()
#1 0xb3af3ec4 in QSGOpaqueTextureMaterialShader::updateState (this=0xb0666780, state=..., newEffect=, oldEffect=0x0) at scenegraph/util/qsgtexturematerial.cpp:99
#2 0xb3ae37c0 in QSGBatchRenderer::Renderer::renderMergedBatch (this=0xb0603560, batch=0xb0672550) at scenegraph/coreapi/qsgbatchrenderer.cpp:2271
#3 0xb3ae4688 in QSGBatchRenderer::Renderer::renderBatches (this=this@entry=0xb0603560) at scenegraph/coreapi/qsgbatchrenderer.cpp:2514
#4 0xb3ae7f18 in QSGBatchRenderer::Renderer::render (this=0xb0603560) at scenegraph/coreapi/qsgbatchrenderer.cpp:2708
#5 0xb3aef7b4 in QSGRenderer::renderScene (this=, bindable=...) at scenegraph/coreapi/qsgrenderer.cpp:222
#6 0xb3aefd0a in QSGRenderer::renderScene (this=, fboId=) at scenegraph/coreapi/qsgrenderer.cpp:178
#7 0xb3af9f84 in QSGRenderContext::renderNextFrame (this=0x2e3460, renderer=0xb0603560, fboId=) at scenegraph/qsgcontext.cpp:555
#8 0xb3b2bc5c in QQuickWindowPrivate::renderSceneGraph (this=this@entry=0x2f27d0, size=...) at items/qquickwindow.cpp:425
#9 0xb3b0d38c in QSGRenderThread::syncAndRender (this=this@entry=0x50a0c0) at scenegraph/qsgthreadedrenderloop.cpp:629
#10 0xb3b0dc96 in QSGRenderThread::run (this=0x50a0c0) at scenegraph/qsgthreadedrenderloop.cpp:715
#11 0xb628c490 in QThreadPrivate::start (arg=0x50a0c0) at thread/qthread_unix.cpp:365
#12 0xb61cb032 in start_thread (arg=0xaeff4390) at pthread_create.c:314

No ideas from that on what is happening.

[dcaliste]

Thank you for the trace, confirming, I think, that texture is used after free or something like that. I guess I'm not properly invalidating textures when re-layouting after rotation. I'm going to fix this and update the MR.

[dcaliste]

I've updated the MR by changing a call from deleteAllTextures() to a loop on pages calling cleanPageTexturesLater() when rotation is done and layout invalidated. If I was right, this should cure the crash of using old textures that have been deleted already.

In the case of rotation, we need to invalidate textures otherwise old not-rotated ones are displayed while new rotated ones are computed. This invalidation is not necessary when zooming, since we sketch the old textures waiting for new ones to be calculated. Ideally, we should be able to reuse the badly oriented textures while waiting for proper ones to be calculated by storing the texture orientation and rotate at rendering time, but IMHO it's quite additional work for a seldom use case.

I've also changed for Camel like case in the PageRotation enumeration.

[dcaliste]

I've rebased on master.