sailfishos / sailfish-office

Sailfish Office
GNU General Public License v2.0
73 stars 26 forks source link

[office] Don't display the PDF view page header on the cover. Fixes JB#46311 #151

Closed adenexter closed 4 years ago

jpetrell commented 4 years ago

Improves the situation, but due the hacky way Documents cover works the result is still very flaky. Maybe we should just drop the preview mode and always show the most recent (or whatever sorting rules apply there) documents.

Some comments:

adenexter commented 4 years ago

Redone to show different views of the documents instead of screen captures. Results are mixed, the scroll position of text and spreadsheet documents appears to be a property of the document object and not views so there's a limit on fine-tuning the positioning on the cover, but that mostly just amounts to a margin at the top of text documents when scrolled to the top.

PDF covers are crashing.

adenexter commented 4 years ago

The crashes have been resolved I think, but it's doing a thing where after peeking the current page is re-rendered at lower resolution. I'm guessing because the application view is picking up a buffer intended for the cover.

jpetrell commented 4 years ago
adenexter commented 4 years ago

I think I've reproduced the app getting stuck by minimizing during the page opening transition and it has to be a silica issue, the page stack is showing the file selector page despite the current page being the document page. Even if there's an issue in the document page the page underneath should be hidden.

adenexter commented 4 years ago

Showing a different view of the text and spreadsheet documents crashed after a few attempts. That appears to be have been because QQuickPaintedItem call it's paint() method from updatePaintNode() which is called from the render thread while the gui thread is locked so it's safe except something in the process of painting creates or starts a QObject timer from that thread and stopping the timer in the object's destructor fails because you can't do that from a different thread so then a timer is triggered for a deleted object.

And different views of a single document while seemingly possible from an API perspective doesn't really translate to reality because a View is just a wrapper around a canvas which is unique object owned by the document and multiple Views just means multiple objects sending conflicting commands to a single canvas.

What does work is getting a thumbnail image of the current page from a ContentsModel and showing that it the cover, which means all formats expect plain text documents which don't have pages show a single current page on the cover.

The sometimes blank cover was a race condition addressing the ContentsModel after the document was loaded but before the model had reacted to the change in state. It's been 'fixed' with a timer.

I tried putting a busy indicator on the cover but it didn't animate so there wasn't much point.

jpetrell commented 4 years ago
jpetrell commented 4 years ago

The cover starts to behave really nicely now. I still wish there was a way to match preview width with cover width, now some strip on the right side of the document gets clipped in cover preview.

Zooming PDF in/out sometimes takes ages, but may not be related to the changes here.

Now if I minimize the app in the middle of the page opening transition the cover a moment later shows the document but the reopened app is stuck in some weird state with page indicator and transition edge indication shown like the document page was open but no where to be seen, below I can interact with the parent main page.

adenexter commented 4 years ago

Now if I minimize the app in the middle of the page opening transition the cover a moment later shows the document but the reopened app is stuck in some weird state with page indicator and transition edge indication shown like the document page was open but no where to be seen, below I can interact with the parent main page.

I can reproduce a few different versions of that, sometimes the page below is visible and is interactive but opening a new page didn't work. I tried putting a Rectangle in a page to see if that would appear but not the document but that was inconclusive. The contents of a page shouldn't affect the visibility of the transition edge indicator or pages below. You've found an issue with page stack transitions or async page creation.

jpetrell commented 4 years ago

SpreadsheetPage, TextDocumentPage and PresentationPage now duplicate code between each other. Create shared CalligraDocumentPage base component?

Otherwise LGTM

jpetrell commented 4 years ago

Works nicely. :+1: