[Merge] lp:~osomon/webbrowser-app/locationBarController into lp:webbrowser-app
Alexandre Abreu
alexandre.abreu at canonical.com
Mon Mar 30 17:28:06 UTC 2015
Added some inline comments
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
why the ignoreUnknownSignals?
> 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) {
I'd put the fullscreen test before the forceshow, since unless I am mistaken it override everything else, and we would make sure that any glitch in the forceShow flag does not have impact on this ...
> + 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.
should it be 1000ms or 500ms as in the timer below?
> + 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) {
no fullscreen flag test? ... if in fullscreen there should be no force show no matter what no?
> + 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
One thing that might be a bit confusing is that the chromeController is being instanciated even when the chromeless flag is set. Overall it works, but it forces some logic to be spread w/ webview.oxide && !webview.chromeless scathered a bit, just for the sake of setting the initial locationbarController mode to ModeHidden. I wonder if it is possible to improve this a bit
>
> 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