Ubuntu Document Viewer App

Merge lp:~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-zoom-final into lp:ubuntu-docviewer-app

  1. ubuntu-docviewer-app-zoom-final
  2. Merge into lo-viewer
Proposed by Roman Shchekin
Status: Merged
Approved by: Stefano Verzegnassi
Approved revision: 200
Merged at revision: 204
Proposed branch: lp:~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-zoom-final
Merge into: lp:ubuntu-docviewer-app
Diff against target: 248 lines (+126/-54)
3 files modified
po/com.ubuntu.docviewer.pot (+6/-6)
src/app/qml/common/ScalingPinchArea.qml (+55/-0)
src/app/qml/loView/LOViewPage.qml (+65/-48)
To merge this branch: bzr merge lp:~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-zoom-final
Reviewer Review Type Date Requested Status
Alan Pope 🍺🐧🐱 🦄 (community) Approve
Stefano Verzegnassi Approve
Jenkins Bot continuous-integration Approve
Review via email: mp+278248@code.launchpad.net

Commit message

Zoom in LO.

Description of the change

Zoom in LO.

To post a comment you must log in.
Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Continuous integration, rev:199
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/24/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/167/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/24/rebuild

review: Needs Fixing (continuous-integration)
lp:~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-zoom-final updated
200. By Roman Shchekin

Resolved conflict.

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

PASSED: Continuous integration, rev:200
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/25/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/168/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/25/rebuild

review: Approve (continuous-integration)
Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

I left 4 diff comments.
They are just a few small notes, your changes look good to me.

Tested on desktop, with both Ubuntu.Layouts modes. I didn't see any regression (cannot test touch events on my PC though).

Tested on the BQ, and works greatly as expected!

Nice work, Roman!

review: Approve
Revision history for this message
Alan Pope 🍺🐧🐱 🦄 (popey) wrote :

This is looking great.

One thing I noticed though is the zoom dropdown is always fixed on "Automatic (Fit width)" even if you choose another zoom level, or zoom in or out with the buttons. This means if you change the zoom level, you can never re-choose "Fit Width) because it thinks it's already ticked.

review: Needs Fixing
Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

> One thing I noticed though is the zoom dropdown is always fixed on "Automatic
> (Fit width)" even if you choose another zoom level, or zoom in or out with the
> buttons. This means if you change the zoom level, you can never re-choose "Fit
> Width) because it thinks it's already ticked.

Honestly, I've missed that. Thanks!

Just had a look at it, it's not a bug introduced by this MP.

I've checked if the Viewer properly set the zoom mode to Manual when a "custom" (i.e. user-specified) value is set, and it does.

It's rather a bug in the ZoomSelector, from line 126:

selectedIndex: {
    if (loPageContentLoader.item.loView.zoomMode == LibreOffice.View.FitToWidth)
        return 0

    for (var i=0; i<textField.values.length; i++) {
        if (values[i] == loView.zoomFactor) {
            return i
        }
    }

    return -1
}

It returns:
    ReferenceError: loView is not defined

The 'for' loop in the code does not return any index, since the comparison between values[i] and loView.zoomFactor is never executed.
For that reason, the first entry in the ZoomSelector ("Automatic (Fit width)") is always selected.

I didn't take care of the current code of the ZoomSelector, since it was already planned to be replaced with some properly designed code.

This bug is already fixed in the UITK 1.3 branch, since it introduces the new ZoomSelector.

Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

Anyway, and that's for sure, the work on zooming is not completed by this MP.

We still have three relevant things to do:

1) Add the double-tap-to-(un)zoom gesture.

2) Properly define the zoom behaviour (minimum zoom, maximum zoom, default value) for the following three form factors:
        - Touch device with screen.width < units.gu(80) - a.k.a. phones
        - Touch device with screen.width > units.gu(80) - a.k.a. tablet
        - Desktop
   We should also take care that the three types of documents we support through LibreOffice (Text, Spreadsheet and Presentation) have some different requirement.
   I started to have a look at this, but it needs to be planned accurately.

   In particular on phones we should limit the minimum zoom to the value set by the "Fit to width" zoom (e.g. Text document).

3) Introduce two new zoom modes: "Fit to Height" and "Automatic".
   "Automatic" mode should be a mix of "Fit to Height" and "Fit to Width". We need it in order to ensure that slides of presentation documents will be always fully visible, no matter which is the size/ratio of the application window.

Revision history for this message
Alan Pope 🍺🐧🐱 🦄 (popey) wrote :

Great! Looks good so far!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'po/com.ubuntu.docviewer.pot'
--- po/com.ubuntu.docviewer.pot 2015-10-31 20:37:19 +0000
+++ po/com.ubuntu.docviewer.pot 2015-11-21 19:45:28 +0000
@@ -8,7 +8,7 @@
8msgstr ""8msgstr ""
9"Project-Id-Version: \n"9"Project-Id-Version: \n"
10"Report-Msgid-Bugs-To: \n"10"Report-Msgid-Bugs-To: \n"
11"POT-Creation-Date: 2015-10-31 23:16+0300\n"11"POT-Creation-Date: 2015-11-21 22:13+0300\n"
12"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"12"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
13"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"13"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
14"Language-Team: LANGUAGE <LL@li.org>\n"14"Language-Team: LANGUAGE <LL@li.org>\n"
@@ -245,7 +245,7 @@
245245
246#: ../src/app/qml/documentPage/DocumentPageSearchHeader.qml:27246#: ../src/app/qml/documentPage/DocumentPageSearchHeader.qml:27
247#: ../src/app/qml/loView/LOViewDefaultHeader.qml:70247#: ../src/app/qml/loView/LOViewDefaultHeader.qml:70
248#: ../src/app/qml/loView/LOViewPage.qml:191248#: ../src/app/qml/loView/LOViewPage.qml:208
249#: ../src/app/qml/pdfView/PdfViewDefaultHeader.qml:61249#: ../src/app/qml/pdfView/PdfViewDefaultHeader.qml:61
250#: ../src/app/qml/textView/TextViewDefaultHeader.qml:61250#: ../src/app/qml/textView/TextViewDefaultHeader.qml:61
251msgid "Back"251msgid "Back"
@@ -378,8 +378,8 @@
378msgid "GO!"378msgid "GO!"
379msgstr ""379msgstr ""
380380
381#: ../src/app/qml/loView/LOViewPage.qml:34381#: ../src/app/qml/loView/LOViewPage.qml:35
382#: ../src/app/qml/loView/LOViewPage.qml:189382#: ../src/app/qml/loView/LOViewPage.qml:206
383msgid "Slides"383msgid "Slides"
384msgstr ""384msgstr ""
385385
@@ -458,10 +458,10 @@
458msgid "copy %1"458msgid "copy %1"
459msgstr ""459msgstr ""
460460
461#: /home/qtros/dev/ubuntu-docviewer-app-render-engine-tuning-build/po/com.ubuntu.docviewer.desktop.in.in.h:1461#: /home/qtros/dev/build-ubuntu-docviewer-app-zoom-final-UbuntuSDK_for_armhf_GCC_ubuntu_sdk_15_04_vivid-Default/po/com.ubuntu.docviewer.desktop.in.in.h:1
462msgid "Document Viewer"462msgid "Document Viewer"
463msgstr ""463msgstr ""
464464
465#: /home/qtros/dev/ubuntu-docviewer-app-render-engine-tuning-build/po/com.ubuntu.docviewer.desktop.in.in.h:2465#: /home/qtros/dev/build-ubuntu-docviewer-app-zoom-final-UbuntuSDK_for_armhf_GCC_ubuntu_sdk_15_04_vivid-Default/po/com.ubuntu.docviewer.desktop.in.in.h:2
466msgid "documents;viewer;pdf;reader;"466msgid "documents;viewer;pdf;reader;"
467msgstr ""467msgstr ""
468468
=== added file 'src/app/qml/common/ScalingPinchArea.qml'
--- src/app/qml/common/ScalingPinchArea.qml 1970-01-01 00:00:00 +0000
+++ src/app/qml/common/ScalingPinchArea.qml 2015-11-21 19:45:28 +0000
@@ -0,0 +1,55 @@
1import QtQuick 2.4
2
3PinchArea {
4 id: pinchArea
5
6 property var targetFlickable: null
7 property real totalScale: 1
8
9 onPinchUpdated: {
10 pinchUpdatedHandler(pinch)
11 }
12 onPinchFinished: {
13 pinchFinishedHandler()
14 }
15
16 // ------------------------ Desktop DEBUG
17
18// MouseArea {
19// id: testMa
20// anchors.fill: parent
21// visible: Qt.platform.os == "linux"
22// z: 19
23// acceptedButtons: Qt.RightButton
24
25// property int touchPointY
26// property int touchPointX
27// onPressed: {
28// touchPointY = mouse.y
29// touchPointX = mouse.x
30// }
31// onPositionChanged: {
32// var sc = (1 + (mouse.y - touchPointY) / 200)
33// pinchUpdatedHandler({"center" : { "x" : mouse.x, "y" : mouse.y }, "scale" : sc })
34// }
35// onReleased: {
36// pinchFinishedHandler()
37// }
38// }
39
40 // ------------------------ Desktop DEBUG end
41
42 function pinchUpdatedHandler(pinch) {
43 targetFlickable.scale = pinch.scale
44 }
45
46 function pinchFinishedHandler() {
47 var pt = pinchArea.mapFromItem(targetFlickable, -targetFlickable.contentX , -targetFlickable.contentY )
48 // console.log("pinchFinishedHandler", -myItem.contentX, -myItem.contentY, Math.round(pt.x), Math.round(pt.y))
49 targetFlickable.contentX = -pt.x
50 targetFlickable.contentY = -pt.y
51
52 totalScale = targetFlickable.scale * totalScale
53 targetFlickable.scale = 1
54 }
55}
056
=== modified file 'src/app/qml/loView/LOViewPage.qml'
--- src/app/qml/loView/LOViewPage.qml 2015-11-13 17:31:09 +0000
+++ src/app/qml/loView/LOViewPage.qml 2015-11-21 19:45:28 +0000
@@ -22,6 +22,7 @@
22import "../upstreamComponents"22import "../upstreamComponents"
2323
24import "../common/utils.js" as Utils24import "../common/utils.js" as Utils
25import "../common"
25import "KeybHelper.js" as KeybHelper26import "KeybHelper.js" as KeybHelper
2627
27PageWithBottomEdge {28PageWithBottomEdge {
@@ -113,7 +114,7 @@
113 }114 }
114115
115 ItemLayout {116 ItemLayout {
116 item: "loView"117 item: "pinchArea"
117 anchors.fill: parent118 anchors.fill: parent
118119
119 // Keyboard events120 // Keyboard events
@@ -139,58 +140,74 @@
139 }140 }
140 ]141 ]
141142
142 LibreOffice.Viewer {143 ScalingPinchArea {
143 id: loView144 id: pinchArea
144 objectName: "loView"145 objectName: "pinchArea"
145 Layouts.item: "loView"146 Layouts.item: "pinchArea"
147
148 targetFlickable: loView
149 onTotalScaleChanged: targetFlickable.updateContentSize(totalScale)
150
146 anchors {151 anchors {
147 left: parent.left
148 right: parent.right
149 top: parent.top152 top: parent.top
153 left: parent.left
154 right: parent.right
150 bottom: bottomBar.top155 bottom: bottomBar.top
151 }156 }
152157
153 clip: true158 LibreOffice.Viewer {
154 documentPath: file.path159 id: loView
155160 objectName: "loView"
156 // Keyboard events161 Layouts.item: "loView"
157 focus: true162
158 Keys.onPressed: KeybHelper.parseEvent(event)163 anchors.fill: parent
159164
160 Component.onCompleted: {165 clip: true
161 // WORKAROUND: Fix for wrong grid unit size166 documentPath: file.path
162 flickDeceleration = 1500 * units.gridUnit / 8167
163 maximumFlickVelocity = 2500 * units.gridUnit / 8168 // Keyboard events
164 loPageContent.forceActiveFocus()169 focus: true
165 }170 Keys.onPressed: KeybHelper.parseEvent(event)
166171
167 onErrorChanged: {172 Component.onCompleted: {
168 var errorString;173 // WORKAROUND: Fix for wrong grid unit size
169174 flickDeceleration = 1500 * units.gridUnit / 8
170 switch(error) {175 maximumFlickVelocity = 2500 * units.gridUnit / 8
171 case LibreOffice.Error.LibreOfficeNotFound:176 loPageContent.forceActiveFocus()
172 errorString = i18n.tr("LibreOffice binaries not found.")177 }
173 break;178
174 case LibreOffice.Error.LibreOfficeNotInitialized:179 function updateContentSize(tgtScale) {
175 errorString = i18n.tr("Error while loading LibreOffice.")180 zoomFactor = tgtScale
176 break;181 }
177 case LibreOffice.Error.DocumentNotLoaded:182
178 errorString = i18n.tr("Document not loaded.\nThe requested document may be corrupt.")183 onErrorChanged: {
179 break;184 var errorString;
180 }185
181186 switch(error) {
182 if (errorString) {187 case LibreOffice.Error.LibreOfficeNotFound:
183 loPage.pageStack.pop()188 errorString = i18n.tr("LibreOffice binaries not found.")
184189 break;
185 // We create the dialog in the MainView, so that it isn't190 case LibreOffice.Error.LibreOfficeNotInitialized:
186 // initialized by 'loPage' and keep on working after the191 errorString = i18n.tr("Error while loading LibreOffice.")
187 // page is destroyed.192 break;
188 mainView.showErrorDialog(errorString);193 case LibreOffice.Error.DocumentNotLoaded:
189 }194 errorString = i18n.tr("Document not loaded.\nThe requested document may be corrupt.")
190 }195 break;
191196 }
192 Scrollbar { flickableItem: loView; parent: loView.parent }197
193 Scrollbar { flickableItem: loView; parent: loView.parent; align: Qt.AlignBottom }198 if (errorString) {
199 loPage.pageStack.pop()
200
201 // We create the dialog in the MainView, so that it isn't
202 // initialized by 'loPage' and keep on working after the
203 // page is destroyed.
204 mainView.showErrorDialog(errorString);
205 }
206 }
207
208 Scrollbar { flickableItem: loView; parent: loView.parent }
209 Scrollbar { flickableItem: loView; parent: loView.parent; align: Qt.AlignBottom }
210 }
194 }211 }
195212
196 // TODO: When we'll have to merge this with the zooming branch, replace this213 // TODO: When we'll have to merge this with the zooming branch, replace this

Subscribers

People subscribed via source and target branches