[Merge] lp:~osomon/webbrowser-app/locationBarController-show-hide into lp:webbrowser-app

[Olivier Tilloy]

Answered inline.

Related to visibility/behavior of the chrome, I just filed bug #1453908. I don’t think its resolution should block merging this in (as the issue already exists with webbrowser-app trunk).

Diff comments:

> === modified file 'debian/control'
> --- debian/control	2015-04-10 13:33:10 +0000
> +++ debian/control	2015-04-17 15:07:58 +0000
> @@ -33,7 +33,7 @@
>  Depends: ${misc:Depends},
>           ${shlibs:Depends},
>           fonts-liberation,
> -         liboxideqt-qmlplugin (>= 1.5),
> +         liboxideqt-qmlplugin (>= 1.7),
>           libqt5sql5-sqlite,
>           qml-module-qt-labs-folderlistmodel,
>           qml-module-qt-labs-settings,
> 
> === modified file 'src/app/ChromeController.qml'
> --- src/app/ChromeController.qml	2015-03-30 18:29:58 +0000
> +++ src/app/ChromeController.qml	2015-04-17 15:07:58 +0000
> @@ -17,7 +17,7 @@
>   */
>  
>  import QtQuick 2.0
> -import com.canonical.Oxide 1.5 as Oxide
> +import com.canonical.Oxide 1.7 as Oxide
>  
>  Item {
>      visible: false
> @@ -25,40 +25,33 @@
>      property var webview
>      property bool forceHide: false
>  
> -    readonly property int mode: {
> -        if (forceHide || webview.fullscreen) {
> -            return Oxide.LocationBarController.ModeHidden
> -        } else if (internal.forceShow) {
> -            return Oxide.LocationBarController.ModeShown
> -        } else {
> -            return Oxide.LocationBarController.ModeAuto
> -        }
> +    onForceHideChanged: {
> +        if (!webview) {
> +            return
> +        }
> +        webview.locationBarController.animated = false
> +        if (forceHide) {

Those two blocks have a similar logic, but the devil is in the detail, the if statements are slightly different, so I don’t think it’s worth trying to extract a pattern here.

> +            webview.locationBarController.mode = Oxide.LocationBarController.ModeHidden
> +        } else if (!webview.fullscreen) {
> +            webview.locationBarController.mode = Oxide.LocationBarController.ModeAuto
> +            webview.locationBarController.show(false)
> +        }
> +        webview.locationBarController.animated = true
>      }
>  
> -    // 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 (500ms) to allow the animation to be committed.
> -    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()
> +            if (webview.fullscreen) {

I think it’s ok to have the chrome animate when going in/out of fullscreen. If you feel strongly otherwise (and have good arguments for it), I’m fine with inhibiting the animation.

> +                webview.locationBarController.mode = Oxide.LocationBarController.ModeHidden
> +            } else if (!forceHide) {
> +                webview.locationBarController.mode = Oxide.LocationBarController.ModeAuto
> +                webview.locationBarController.show(true)
>              }
>          }
>          onLoadingChanged: {
> -            if (webview.loading && !webview.fullscreen) {
> -                internal.forceShow = true
> -                delayedResetMode.restart()
> +            if (webview.loading && !webview.fullscreen && !forceHide) {
> +                webview.locationBarController.show(true)
>              }
>          }
>      }
> 
> === modified file 'src/app/webbrowser/Browser.qml'
> --- src/app/webbrowser/Browser.qml	2015-04-07 17:10:56 +0000
> +++ src/app/webbrowser/Browser.qml	2015-04-17 15:07:58 +0000
> @@ -547,14 +547,12 @@
>                  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
> 
> === modified file 'src/app/webcontainer/WebApp.qml'
> --- src/app/webcontainer/WebApp.qml	2015-04-08 13:20:57 +0000
> +++ src/app/webcontainer/WebApp.qml	2015-04-17 15:07:58 +0000
> @@ -143,17 +143,9 @@
>          }
>  
>          ChromeController {
> -            id: oxideChromeController
>              webview: webapp.currentWebview
>              forceHide: webapp.chromeless
>          }
> -
> -        Binding {
> -            when: webapp.currentWebview
> -            target: webapp.currentWebview ? webapp.currentWebview.locationBarController : null
> -            property: 'mode'
> -            value: oxideChromeController.mode
> -        }
>      }
>  
>      UnityWebApps.UnityWebApps {
> 


-- 
https://code.launchpad.net/~osomon/webbrowser-app/locationBarController-show-hide/+merge/256679
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~osomon/webbrowser-app/locationBarController-show-hide into lp:webbrowser-app.