Merge lp:~verzegnassi-stefano/ubuntu-docviewer-app/lok-qml-async-imageprovider into lp:ubuntu-docviewer-app
| Status: | Merged |
|---|---|
| Approved by: | Stefano Verzegnassi on 2016-02-05 |
| Approved revision: | 256 |
| Merged at revision: | 294 |
| Proposed branch: | lp:~verzegnassi-stefano/ubuntu-docviewer-app/lok-qml-async-imageprovider |
| Merge into: | lp:ubuntu-docviewer-app |
| Prerequisite: | lp:~ubuntu-docviewer-dev/ubuntu-docviewer-app/ubuntu-docviewer-app-re-fix |
| Diff against target: |
602 lines (+162/-120) 17 files modified
po/com.ubuntu.docviewer.pot (+9/-9) src/app/qml/loView/PartsView.qml (+4/-4) src/app/qml/pdfView/PdfPresentation.qml (+1/-1) src/app/qml/ubuntu-docviewer-app.qml (+2/-2) src/plugin/libreofficetoolkit-qml-plugin/CMakeLists.txt (+1/-0) src/plugin/libreofficetoolkit-qml-plugin/lodocument.cpp (+5/-5) src/plugin/libreofficetoolkit-qml-plugin/lodocument.h (+1/-1) src/plugin/libreofficetoolkit-qml-plugin/lopartsimageprovider.cpp (+23/-31) src/plugin/libreofficetoolkit-qml-plugin/lopartsimageprovider.h (+8/-12) src/plugin/libreofficetoolkit-qml-plugin/lopartsimageresponse.cpp (+54/-0) src/plugin/libreofficetoolkit-qml-plugin/lopartsimageresponse.h (+41/-0) src/plugin/libreofficetoolkit-qml-plugin/lopartsmodel.cpp (+0/-20) src/plugin/libreofficetoolkit-qml-plugin/lopartsmodel.h (+0/-7) src/plugin/libreofficetoolkit-qml-plugin/lorendertask.cpp (+1/-1) src/plugin/libreofficetoolkit-qml-plugin/lorendertask.h (+3/-3) src/plugin/libreofficetoolkit-qml-plugin/loview.cpp (+9/-22) src/plugin/libreofficetoolkit-qml-plugin/loview.h (+0/-2) |
| To merge this branch: | bzr merge lp:~verzegnassi-stefano/ubuntu-docviewer-app/lok-qml-async-imageprovider |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jenkins Bot | continuous-integration | Approve on 2016-02-05 | |
| Roman Shchekin | 2016-01-22 | Approve on 2016-02-05 | |
| Nicholas Skaggs | 2016-01-17 | Needs Fixing on 2016-02-02 | |
|
Review via email:
|
|||
Commit Message
Use QQuickAsyncImag
This is backported on Ubuntu/Ubuntu Touch since Qt 5.4.1-1ubuntu7.
On any other distro/OS it works only with Qt 5.6 (or later).
Description of the Change
Use QQuickAsyncImag
This is backported on Ubuntu/Ubuntu Touch since Qt 5.4.1-1ubuntu7.
On any other distro/OS it works only with Qt 5.6 (or later).
FAILED: Continuous integration, rev:248
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Roman Shchekin (mrqtros) wrote : | # |
Just remove it and manage memory yourself (in "slotTaskRender
By doing so, the app crashes any time a document is loaded.
It does not crash always at the same point, but it may occur at two different points:
1) In the internalRenderC
if (m_activeTaskCount && !task->
return;
2) In lopartsimageres
void LOPartsImageRes
{
disconnect(
if (m_task) {
delete m_task;
}
}
With the current code, I really think we should give the ownership of the task to the RenderEngine.
As we discussed earlier, the code I wrote is not so good because the ImageResponse gets the ownership and then it gives that ownership away in a asymmetrical/
Supposed that we should try to make RenderEngine work properly with different threads, could we revert to the code I wrote earlier (RenderEngine takes ownership of the tasks, and we connect to it using a BlockingQueueCo
It used to work with no issue when we tried it.
I had a further look at the issue.
It seems that QQuickPixmapCache calls QQuickImageResp
We properly synchronize the threads when we queue/dequeue the task from the engine but, if the task is currently being processed, it gets deleted during its execution.
We need to ensure that the GUI thread (where RenderEngine lives) and the LOImageResponse are in sync when we delete the task.
I guess we'd be better not to give ownership of the task to a class that lives in another thread, and let RenderEngine handles its tasks at its best.
We only need to subscribe to the RenderEngine's signals, using a Qt::BlockingQue
Roman, I remember that the usage of a blocking connection was okay for you before you updated the RenderEngine.
What do you think?
- 249. By Stefano Verzegnassi on 2016-01-24
-
Moved all the RenderEngine code in LOPartsImageRes
ponse to LOPartsImagePro vider
FAILED: Continuous integration, rev:249
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 250. By Stefano Verzegnassi on 2016-01-24
-
Merged trunk. Updated translation template.
- 251. By Stefano Verzegnassi on 2016-01-24
-
Restored 'cache:false' so that we don't display a thumbnail from a different document
FAILED: Continuous integration, rev:251
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:251
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 252. By Stefano Verzegnassi on 2016-01-26
-
Debugging Jenkins issue with QQuickAsyncImag
eProvider
FAILED: Continuous integration, rev:252
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:252
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 253. By Stefano Verzegnassi on 2016-02-02
-
Merged trunk, updated .pot
FAILED: Continuous integration, rev:253
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
Still building the project against OTA-4 (May, 2014)...
FAILED: Continuous integration, rev:253
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:253
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Nicholas Skaggs (nskaggs) wrote : | # |
Text conflict in po/com.
1 conflicts encountered.
Everytime something lands, you get this lovely conflict. Not sure if there's a better way for you to avoid this :-)
| Nicholas Skaggs (nskaggs) wrote : | # |
In other news, jenkins should build and run this cleanly now.
| Roman Shchekin (mrqtros) wrote : | # |
So, blockig connection solved all issues?
Just for inromation - you decided do not use render tasks separately from RenderEngine?
- So it seems. We've discussed about it some week ago, and it seems to keep everything in sync since it waits for the ImageResponse to complete its work on the returned image.
- I moved the code that calls the RenderEngine to the image provider. This way it is used from the GUI thread and there is no problem with ownership, etc...
- 254. By Stefano Verzegnassi on 2016-02-05
-
Merged trunk + fixed .pot conflict
- 255. By Stefano Verzegnassi on 2016-02-05
-
Fixed LOK viewer going full-screen with a .odp/.ppt(x) document.
Both PDF full-screen presentation mode and LOK viewer were using a 'isPresentation' property with two different meaning. My fault, sorry! :/
PASSED: Continuous integration, rev:255
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
- 256. By Stefano Verzegnassi on 2016-02-05
-
Removed Jenkins debug
PASSED: Continuous integration, rev:256
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/


@Roman, we still have to discuss about the line 398 of the diff... :)