[Merge] lp:~osomon/webbrowser-app/locationBarController into lp:webbrowser-app
Olivier Tilloy
olivier.tilloy at canonical.com
Mon Mar 30 18:38:34 UTC 2015
Thanks for the thorough review! I addressed the issues you pointed out, and answered your comments inline.
Diff comments:
> === modified file 'debian/control'
> --- debian/control 2015-03-23 07:49:08 +0000
> +++ debian/control 2015-03-27 11:43:49 +0000
> @@ -57,7 +57,7 @@
> Depends: ${misc:Depends},
> ${shlibs:Depends},
> fonts-liberation,
> - liboxideqt-qmlplugin (>= 1.3),
> + liboxideqt-qmlplugin (>= 1.5),
> libqt5sql5-sqlite,
> qml-module-qtquick2 | qtdeclarative5-qtquick2-plugin,
> qml-module-qtquick-window2 | qtdeclarative5-window-plugin,
> @@ -97,7 +97,7 @@
> Pre-Depends: ${misc:Pre-Depends}
> Depends: ${misc:Depends},
> ${shlibs:Depends},
> - liboxideqt-qmlplugin (>= 1.4),
> + liboxideqt-qmlplugin (>= 1.5),
> qml-module-qtquick2 | qtdeclarative5-qtquick2-plugin,
> qml-module-qtquick-window2 | qtdeclarative5-window-plugin,
> qtdeclarative5-ubuntu-ui-toolkit-plugin,
>
> === modified file 'src/Ubuntu/Web/UbuntuWebView02.qml'
> --- src/Ubuntu/Web/UbuntuWebView02.qml 2015-03-23 07:49:08 +0000
> +++ src/Ubuntu/Web/UbuntuWebView02.qml 2015-03-27 11:43:49 +0000
> @@ -18,7 +18,7 @@
>
> import QtQuick 2.0
> import QtQuick.Window 2.0
> -import com.canonical.Oxide 1.4 as Oxide
> +import com.canonical.Oxide 1.5 as Oxide
> import Ubuntu.Components 1.1
> import Ubuntu.Components.Popups 1.0
> import "." // QTBUG-34418
>
> === modified file 'src/app/BrowserWindow.qml'
> --- src/app/BrowserWindow.qml 2014-11-03 17:01:29 +0000
> +++ src/app/BrowserWindow.qml 2015-03-27 11:43:49 +0000
> @@ -1,5 +1,5 @@
> /*
> - * Copyright 2014 Canonical Ltd.
> + * Copyright 2014-2015 Canonical Ltd.
> *
> * This file is part of webbrowser-app.
> *
> @@ -39,6 +39,7 @@
>
> Connections {
> target: window.currentWebview
> + ignoreUnknownSignals: true
Not really related to the changes in this MR, but when testing the webapp container with webkit as the rendering engine (forced with --webkit), the webview doesn’t have an onFullscreenChanged signal, so it generates a (harmless) warning. This ignoreUnknownSignals flag suppresses the warning.
> onFullscreenChanged: {
> if (!window.forceFullscreen) {
> if (window.currentWebview.fullscreen) {
>
> === modified file 'src/app/ChromeBase.qml'
> --- src/app/ChromeBase.qml 2014-08-26 08:53:28 +0000
> +++ src/app/ChromeBase.qml 2015-03-27 11:43:49 +0000
> @@ -1,5 +1,5 @@
> /*
> - * Copyright 2014 Canonical Ltd.
> + * Copyright 2014-2015 Canonical Ltd.
> *
> * This file is part of webbrowser-app.
> *
> @@ -23,27 +23,14 @@
> StyledItem {
> id: chrome
>
> - readonly property real visibleHeight: y + height
> property var webview
>
> - readonly property bool moving: (y < 0) && (y > -height)
> -
> states: [
> State {
> name: "shown"
> - },
> - State {
> - name: "hidden"
> + when: chrome.y == 0
> }
> ]
> - state: "shown"
> -
> - y: (state == "shown") ? 0 : -height
> - Behavior on y {
> - SmoothedAnimation {
> - duration: UbuntuAnimation.BriskDuration
> - }
> - }
>
> Rectangle {
> anchors.fill: parent
>
> === added file 'src/app/ChromeController.qml'
> --- src/app/ChromeController.qml 1970-01-01 00:00:00 +0000
> +++ src/app/ChromeController.qml 2015-03-27 11:43:49 +0000
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright 2015 Canonical Ltd.
> + *
> + * This file is part of webbrowser-app.
> + *
> + * webbrowser-app is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * webbrowser-app is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +import QtQuick 2.0
> +import com.canonical.Oxide 1.5 as Oxide
> +
> +Item {
> + visible: false
> +
> + property var webview
> + property bool chromeless: false
> + property bool forceHide: false
> +
> + readonly property int mode: {
> + if (chromeless || forceHide) {
> + return Oxide.LocationBarController.ModeHidden
> + } else if (internal.forceShow) {
> + return Oxide.LocationBarController.ModeShown
> + } else if (webview.fullscreen) {
Right, good catch. Done.
> + return Oxide.LocationBarController.ModeHidden
> + } else {
> + return Oxide.LocationBarController.ModeAuto
> + }
> + }
> +
> + // Work around the lack of a show() method on the location bar controller
> + // (https://launchpad.net/bugs/1422920) by forcing its mode to ModeShown
> + // for long enough (1000ms) to allow the animation to be committed.
500ms indeed, the code is authoritative. Fixed the comment, thanks for pointing it out!
> + QtObject {
> + id: internal
> + property bool forceShow: false
> + }
> + Timer {
> + id: delayedResetMode
> + interval: 500
> + onTriggered: internal.forceShow = false
> + }
> + Connections {
> + target: webview
> + onFullscreenChanged: {
> + if (!webview.fullscreen) {
> + internal.forceShow = true
> + delayedResetMode.restart()
> + }
> + }
> + onLoadingChanged: {
> + if (webview.loading) {
You’re absolutely right. Fixed.
> + internal.forceShow = true
> + delayedResetMode.restart()
> + }
> + }
> + }
> +}
>
> === removed file 'src/app/ChromeStateTracker.qml'
> --- src/app/ChromeStateTracker.qml 2014-08-13 15:43:02 +0000
> +++ src/app/ChromeStateTracker.qml 1970-01-01 00:00:00 +0000
> @@ -1,82 +0,0 @@
> -/*
> - * Copyright 2014 Canonical Ltd.
> - *
> - * This file is part of webbrowser-app.
> - *
> - * webbrowser-app is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; version 3.
> - *
> - * webbrowser-app is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program. If not, see <http://www.gnu.org/licenses/>.
> - */
> -
> -import QtQuick 2.0
> -
> -// A specialized ScrollTracker that handles automatically showing/hiding
> -// the chrome for a given webview, based on scroll events and proximity to
> -// the top/bottom of the page, as well as whether the webview is currently
> -// fullscreen.
> -ScrollTracker {
> - id: chromeStateTracker
> -
> - active: webview && !webview.fullscreen
> -
> - onScrolledUp: {
> - if (!header.moving && chromeStateChangeTimer.settled) {
> - delayedAutoHideTimer.up = true
> - delayedAutoHideTimer.restart()
> - }
> - }
> - onScrolledDown: {
> - if (!header.moving && chromeStateChangeTimer.settled) {
> - delayedAutoHideTimer.up = false
> - delayedAutoHideTimer.restart()
> - }
> - }
> -
> - // Delay the auto-hide/auto-show behaviour of the header, in order
> - // to prevent the view from jumping up and down on touch-enabled
> - // devices when the touch event sequence is not finished.
> - // See https://bugs.launchpad.net/webbrowser-app/+bug/1354700.
> - Timer {
> - id: delayedAutoHideTimer
> - interval: 250
> - property bool up
> - onTriggered: {
> - if (up) {
> - chromeStateTracker.header.state = "shown"
> - } else {
> - if (chromeStateTracker.nearBottom) {
> - chromeStateTracker.header.state = "shown"
> - } else if (!chromeStateTracker.nearTop) {
> - chromeStateTracker.header.state = "hidden"
> - }
> - }
> - }
> - }
> -
> - // After the chrome has stopped moving (either fully shown or fully
> - // hidden), give it some time to "settle". Until it is settled,
> - // scroll events won’t trigger a new change in the chrome’s
> - // visibility, to prevent the chrome from jumping back into view if
> - // it has just been hidden.
> - // See https://bugs.launchpad.net/webbrowser-app/+bug/1354700.
> - Timer {
> - id: chromeStateChangeTimer
> - interval: 50
> - running: !chromeStateTracker.header.moving
> - onTriggered: settled = true
> - property bool settled: true
> - }
> -
> - Connections {
> - target: chromeStateTracker.header
> - onMovingChanged: chromeStateChangeTimer.settled = false
> - }
> -}
>
> === removed file 'src/app/ScrollTracker.qml'
> --- src/app/ScrollTracker.qml 2014-07-29 22:38:58 +0000
> +++ src/app/ScrollTracker.qml 1970-01-01 00:00:00 +0000
> @@ -1,66 +0,0 @@
> -/*
> - * Copyright 2014 Canonical Ltd.
> - *
> - * This file is part of webbrowser-app.
> - *
> - * webbrowser-app is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; version 3.
> - *
> - * webbrowser-app is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program. If not, see <http://www.gnu.org/licenses/>.
> - */
> -
> -import QtQuick 2.0
> -
> -Item {
> - id: scrollTracker
> -
> - property var webview
> - property var header
> -
> - readonly property bool nearTop: webview ? webview.contentY < (internal.headerHeight / internal.contentRatio) : false
> - readonly property bool nearBottom: webview ? (webview.contentY + internal.viewportHeight + internal.headerHeight / internal.contentRatio) > internal.contentHeight : false
> -
> - property bool active: true
> -
> - signal scrolledUp()
> - signal scrolledDown()
> -
> - enabled: false
> - visible: false
> -
> - QtObject {
> - id: internal
> -
> - readonly property real headerHeight: scrollTracker.header ? scrollTracker.header.height : 0
> - readonly property real headerVisibleHeight: scrollTracker.header ? scrollTracker.header.visibleHeight : 0
> -
> - readonly property real contentHeight: scrollTracker.webview ? scrollTracker.webview.contentHeight + headerVisibleHeight : 0.0
> - readonly property real viewportHeight: scrollTracker.webview ? scrollTracker.webview.viewportHeight + headerVisibleHeight : 0.0
> - readonly property real maxContentY: scrollTracker.webview ? scrollTracker.webview.contentHeight - scrollTracker.webview.viewportHeight : 0.0
> -
> - readonly property real contentRatio: scrollTracker.webview ? scrollTracker.webview.viewportHeight / scrollTracker.webview.contentHeight : 1.0
> -
> - readonly property real currentScrollFraction: (maxContentY == 0.0) ? 0.0 : (scrollTracker.webview.contentY / maxContentY)
> - property real previousScrollFraction: 0.0
> - }
> -
> - Connections {
> - target: scrollTracker.active ? scrollTracker.webview : null
> - onContentYChanged: {
> - var old = internal.previousScrollFraction
> - internal.previousScrollFraction = internal.currentScrollFraction
> - if (internal.currentScrollFraction < old) {
> - scrollTracker.scrolledUp()
> - } else if (internal.currentScrollFraction > old) {
> - scrollTracker.scrolledDown()
> - }
> - }
> - }
> -}
>
> === modified file 'src/app/webbrowser/Browser.qml'
> --- src/app/webbrowser/Browser.qml 2015-03-23 07:49:13 +0000
> +++ src/app/webbrowser/Browser.qml 2015-03-27 11:43:49 +0000
> @@ -103,9 +103,9 @@
> anchors {
> left: parent.left
> right: parent.right
> - top: recentView.visible ? invisibleTabChrome.bottom : chrome.bottom
> + top: recentView.visible ? invisibleTabChrome.bottom : parent.top
> }
> - height: parent.height - osk.height - (recentView.visible ? invisibleTabChrome.height : chrome.visibleHeight)
> + height: parent.height - osk.height - (recentView.visible ? invisibleTabChrome.height : 0)
> }
>
> Loader {
> @@ -149,6 +149,8 @@
> webview: browser.currentWebview
> searchUrl: searchEngine.urlTemplate
>
> + y: webview ? webview.locationBarController.offset : 0
> +
> function isCurrentUrlBookmarked() {
> return ((webview && browser.bookmarksModel) ? browser.bookmarksModel.contains(webview.url) : false)
> }
> @@ -217,29 +219,6 @@
> onTriggered: browser.openUrlInNewTab("", true)
> }
> ]
> -
> - Connections {
> - target: browser.currentWebview
> - onLoadingStateChanged: {
> - if (browser.currentWebview.loading) {
> - chrome.state = "shown"
> - } else if (browser.currentWebview.fullscreen) {
> - chrome.state = "hidden"
> - }
> - }
> - onFullscreenChanged: {
> - if (browser.currentWebview.fullscreen) {
> - chrome.state = "hidden"
> - } else {
> - chrome.state = "shown"
> - }
> - }
> - }
> - }
> -
> - ChromeStateTracker {
> - webview: browser.currentWebview
> - header: chrome
> }
>
> Suggestions {
> @@ -517,6 +496,17 @@
>
> enabled: visible && !bottomEdgeHandle.dragging && !recentView.visible
>
> + ChromeController {
> + id: chromeController
> + webview: webviewimpl
> + forceHide: recentView.visible
> + }
> +
> + locationBarController {
> + height: webviewimpl.visible ? chrome.height : 0
> + mode: chromeController.mode
> + }
> +
> //experimental.preferences.developerExtrasEnabled: developerExtrasEnabled
> preferences.localStorageEnabled: true
> preferences.appCacheEnabled: true
>
> === modified file 'src/app/webcontainer/WebApp.qml'
> --- src/app/webcontainer/WebApp.qml 2015-02-19 11:50:21 +0000
> +++ src/app/webcontainer/WebApp.qml 2015-03-27 11:43:49 +0000
> @@ -73,9 +73,9 @@
> left: parent.left
> right: parent.right
> top: parent.top
> - topMargin: webapp.chromeless ? 0 : chromeLoader.item.visibleHeight
> + topMargin: (webapp.oxide || webapp.chromeless) ? 0 : chromeLoader.item.height
> }
> - height: parent.height - osk.height - (webapp.chromeless ? 0 : chromeLoader.item.visibleHeight)
> + height: parent.height - osk.height
> developerExtrasEnabled: webapp.developerExtrasEnabled
> }
>
> @@ -115,25 +115,7 @@
> right: parent.right
> }
> height: units.gu(6)
> -
> - Connections {
> - target: webapp.currentWebview
> - ignoreUnknownSignals: true
> - onLoadingStateChanged: {
> - if (webapp.currentWebview.loading) {
> - chromeLoader.item.state = "shown"
> - } else if (webapp.currentWebview.fullscreen) {
> - chromeLoader.item.state = "hidden"
> - }
> - }
> - onFullscreenChanged: {
> - if (webapp.currentWebview.fullscreen) {
> - chromeLoader.item.state = "hidden"
> - } else {
> - chromeLoader.item.state = "shown"
> - }
> - }
> - }
> + y: (webapp.oxide && webapp.currentWebview) ? webview.currentWebview.locationBarController.offset : 0
> }
> }
>
> @@ -152,18 +134,34 @@
> }
> }
>
> + Binding {
> + when: webapp.oxide && webapp.currentWebview && !webapp.chromeless
> + target: (webapp.oxide && webapp.currentWebview) ? webapp.currentWebview.locationBarController : null
> + property: 'height'
> + value: webapp.currentWebview.visible ? chromeLoader.item.height : 0
> + }
> +
> Loader {
> - sourceComponent: (webapp.oxide && !webapp.chromeless) ? chromeStateTrackerComponent : undefined
> + id: oxideChromeController
> +
> + sourceComponent: webapp.oxide ? oxideChromeControllerComponent : undefined
We could avoid instantiating the chrome controller when in chromeless mode, but I think it would make the code even less readable.
But as soon as your branch to remove webkit support lands, we can easily address that by setting mode to ModeHidden directly when in chromeless mode (as we’ll be allowed to import Oxide directly).
>
> Component {
> - id: chromeStateTrackerComponent
> + id: oxideChromeControllerComponent
>
> - ChromeStateTracker {
> + ChromeController {
> webview: webapp.currentWebview
> - header: chromeLoader.item
> + chromeless: webapp.chromeless
> }
> }
> }
> +
> + Binding {
> + when: webapp.oxide && webapp.currentWebview
> + target: (webapp.oxide && webapp.currentWebview) ? webapp.currentWebview.locationBarController : null
> + property: 'mode'
> + value: webapp.oxide ? oxideChromeController.item.mode : 0
> + }
> }
>
> UnityWebApps.UnityWebApps {
>
--
https://code.launchpad.net/~osomon/webbrowser-app/locationBarController/+merge/254068
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list