Merge lp:~mzanetti/ubuntu-docviewer-app/fix-layouts-with-icons into lp:ubuntu-docviewer-app/trunk

[Michael Zanetti]

properly set icon sizes inside layouts

QtQuick.Layouts ignore width/height and only act on implicitWidth/implicitHeight. Given that the implicit size of an Icon does not necessarily match the requested width/height (for example if the icon loads a png which simply isn't available in the requested size) this can badly mess up the layout and even end up in a loop reloading the image.

sizes in Layouts should always be set using Layout.preferredWidth/preferredHeight.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:99
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/184/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2424/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-docviewer-app-utopic-amd64-ci/81/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-docviewer-app-vivid-amd64-ci/87/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/184/rebuild

review: Needs Fixing (continuous-integration)

[Stefano Verzegnassi (verzegnassi-stefano)]

Hi Michael,
Thank you for the patch!

I wonder if you intentionally chose lp:~verzegnassi-stefano/ubuntu-docviewer-app/attempt-fix-zooming-1 as prerequisite branch.
Also, it seems there's a conflict with the translation template.

review: Needs Information

[Michael Zanetti (mzanetti)]

Hi Stefano.

No, I was asked by Alan to have a look at your branch and had to do those changes to make it work with Qt 5.5 and my High-resolution screen, otherwise it would end up in a loop requesting those images over and over again at startup.

While the root cause for that loop is a bug in Qt, fixing the layout usage is a good idea anyways. As I did those changes already I just pushed the branch so the fix isn't lost.

[Stefano Verzegnassi (verzegnassi-stefano)]

I had that doubt because the patch didn't seem to be related to the prerequisite branch. In this case it's OK for me.

Thank you again for giving a look at the branch. :D

review: Approve

[David Planella (dpm)]

Did this branch ever land?

It's marked as Approved instead of Merged, so it appears in the list of branches to review. If it landed, could it be marked as Merged, or alternatively as Rejected?

Thanks!

[Stefano Verzegnassi (verzegnassi-stefano)]

This branch has been proposed against a development branch that still requires some work.
Probably there are other QtQuick.Layouts in the app that may need the fix proposed here.

During the weekend I'll open a branch that refers to lp:ubuntu-docviewer-app and will include this MP.

[Stefano Verzegnassi (verzegnassi-stefano)]

Proposed for merging a new branch, depending on trunk and including the changes made by Michael Zanetti.
For this reason, I mark this MP as rejected.

Ref.
https://code.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/fix-layout-with-icons-2/+merge/263369

Unmerged revisions

99. By Michael Zanetti on 2015-03-25

fix layouts

98. By Stefano Verzegnassi on 2015-03-24

Attempt to fix bad zooming behaviour

=== modified file 'src/app/qml/documentPage/DocumentGridDelegate.qml'
--- src/app/qml/documentPage/DocumentGridDelegate.qml	2015-03-04 19:38:48 +0000
+++ src/app/qml/documentPage/DocumentGridDelegate.qml	2015-03-25 11:45:29 +0000
@@ -79,7 +79,8 @@
             anchors.centerIn: parent
             anchors.verticalCenterOffset: - infoColumn.height * 0.3
 
-            width: units.gu(8); height: width
+            Layout.preferredWidth: units.gu(8);
+            Layout.preferredHeight: width
 
             // At the moment the suru icon theme doesn't have much icons.
             // Just some note for the future:

=== modified file 'src/app/qml/documentPage/DocumentListDelegate.qml'
--- src/app/qml/documentPage/DocumentListDelegate.qml	2015-03-04 17:44:36 +0000
+++ src/app/qml/documentPage/DocumentListDelegate.qml	2015-03-25 11:45:29 +0000
@@ -59,8 +59,8 @@
         spacing: units.gu(2)
 
         Icon {
-            width: height
-            height: units.gu(5)
+            Layout.preferredWidth: height
+            Layout.preferredHeight: units.gu(5)
 
             // At the moment the suru icon theme doesn't have much icons.
             name: {