Merge lp:~nikwen/webbrowser-app/open-in-new-background-tab into lp:webbrowser-app

[Niklas Wenzel]

Add the ability to open a link in a new background tab

Added the ability to open a link in a new background tab

This is in some way related to LP: #1339437.
https://bugs.launchpad.net/webbrowser-app/+bug/1339437

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:863
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1363/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/682
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/369/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/121/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/121
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/121/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/121/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/587
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/680
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/680/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16863
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/375/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1363/rebuild

review: Needs Fixing (continuous-integration)

[Niklas Wenzel (nikwen)]

Here's an excerpt from the logs which states that the build failed, but it works fine on my local machine. Would somebody be so kind as to re-run the build, please?

The following tests FAILED:
   2 - tst_QmlTests (Failed)
Errors while running CTest
Makefile:120: recipe for target 'test' failed
make[1]: *** [test] Error 8
make[1]: Leaving directory '/tmp/buildd/webbrowser-app-0.23+15.04.20141218bzr864pkg0vivid121/obj-x86_64-linux-gnu'
dh_auto_test: make -j2 test ARGS+=-j2 returned exit code 2
debian/rules:11: recipe for target 'build' failed
make: *** [build] Error 2
dpkg-buildpackage: error: debian/rules build gave error exit status 2
E: Failed autobuilding of package
[...]
bzr: ERROR: The build failed.
ERROR:pbuilderjenkins:Error during build execution
ERROR:pbuilderjenkins:pdebuild failed
pbuilder returned ERROR.
Build step 'Debian PBuilder NG' marked build as failure

[Olivier Tilloy (osomon)]

Niklas: the build failure is a known issue not related to your change, see bug #1396951 for details.

[Olivier Tilloy (osomon)]

And since I see that you updated the copyright notice, a minor remark: that file was added in 2014, and not modified since then, so the year in the notice should be 2014 only.

[Niklas Wenzel (nikwen)]

Hi Olivier,

Thank you very much for looking into this. :)
Do you have any dates for when the chroots will be set up properly again?
I'll add changing the copyright notice to my TODO list. ;)

[Olivier Tilloy (osomon)]

No date that I can commit on, but I know it’s being worked on. In the meantime, it’s relatively safe to ignore those failures, given that the build succeeds on armhf (where it’s built on real hardware).

[Niklas Wenzel (nikwen)]

Ok, here's the copyright notice fix.

What's about the change in general? As far as I can see, nobody has reviewed it yet, right?

[Olivier Tilloy (osomon)]

Looks good to me, thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:868
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1367/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/813
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/395
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/125
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/125
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/125/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/125/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/708
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/811
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/811/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17105
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/323
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/410
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/410/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1367/rebuild

review: Needs Fixing (continuous-integration)

[Niklas Wenzel (nikwen)]

Thanks for approving. :)

I'll ping you again when the i386 chroot is working properly again, so that you can re-run the tests. ;)

[Olivier Tilloy (osomon)]

Hey Niklas, Giorgio mentioned in https://bugs.launchpad.net/webbrowser-app/+bug/1339437/comments/4 that this option should be added to the desktop version only.

Could you make it conditional? Something like this:

  Actions.OpenLinkInNewBackgroundTab {
    enabled: contextualData.href.toString() && (formFactor == "desktop")
    […]
  }

[Niklas Wenzel (nikwen)]

Hi Olivier,

Thank you for pointing me to this post. I somehow forgot to subscribe to the bug report, so I didn't see it earlier.
Let me comment on the bug report for this. ;)

[Niklas Wenzel (nikwen)]

Here you go. Now it's a setting as well. :)

[Niklas Wenzel (nikwen)]

The setting to enable the "Open in new background tab" option on the phone form factor can be turned on like this:

mkdir ~/.config/webbrowser-app
nano ~/.config/webbrowser-app/settings.conf

Add the following line to the file and save it. Then restart the webbrowser app.

enableBackgroundTabs=true

[Olivier Tilloy (osomon)]

Thanks Niklas, from a quick glance at the diff the change looks good. I wonder whether the setting could be made a boolean instead of a string. If I understand Giorgio’s suggestion correctly, on desktop it’s ok to always have the entry in the context menu, whereas on mobile it’s optional. So basically the setting should be ignored on desktop, and taken into account on mobile only.
By the way, can the name of the setting be updated to make it more explicit that it applies to mobile only?

[Olivier Tilloy (osomon)]

I would rename the setting to something like "allowOpenInBackgroundTabOnMobile". It’s a tad verbose, but it says what it does.

[Niklas Wenzel (nikwen)]

Yes, it could be made a boolean but that would also alter its current behaviour.

The way it's currently implemented allows to disable the entry on the desktop as well. It uses three options: Show, don't show, and decide based on form factor.

We could drop the ability to hide it on the desktop in favour of making it a boolean but I'd vote for the more flexible current approach here.

[Olivier Tilloy (osomon)]

In that case the option can still be made a boolean (and renamed "allowOpenInBackgroundTab"). The option would in fact have three possible values:

  (not set) default behaviour, i.e. enabled on desktop and disabled on mobile
  (True) enabled on all form factors
  (False) disabled on all form factors

The fact that the default behaviour is form-factor dependent should be clearly documented in the code.

[Niklas Wenzel (nikwen)]

Yes, this is how it currently works.
Since C++ (as far as I know) doesn't allow undefined as a value for boolean variables, we need to have a variable with at least 3 different values, hence an enumaration (which will be troublesome in connection with QML), a numerical value (whose meaning won't be self-explaining when passing it to the QML property) or a string, which in this particular case is what was entered in the settings file.

We can't directly provide the "show option or not" value from the settings C++ class because we cannot easily get the form factor there.

This is why I decided to do it this way:

1) Pass the value from the settings.conf file directly to QML as a string.
2) Let QML decide whether to hide the option based on the settings vallue and the form factor.

These are the values the string can have:

  "formfactor_dependend": The default value if the setting isn't set, i.e. decide based on form factor.
  "true": If the option should be shown.
  "false" or anything else: If the option should not be shown.

I do, however, agree that we can rename the settings entry. I'll do that tomorrow and use the suggestion from your last post. ;)

What do you think?

[Olivier Tilloy (osomon)]

That sounds about right (I didn’t remember that we don’t have access to the form factor on the C++ side). Can "formfactor_dependend" be renamed to "default" ?

[Niklas Wenzel (nikwen)]

Here you go. Is there anything left which I should change as well?

Thanks again for looking into this. :)

[Olivier Tilloy (osomon)]

That looks great now, thanks!
A couple of really minor comments:

 - the return type of allowOpenInBackgroundTab() could be "const QString&" to avoid an unnecessary copy

 - "anything else disables background tabs" is confusing, background tabs are not disabled, it’s just that the context menu option is not available. Can it be "anything else disables the option" instead?

[Niklas Wenzel (nikwen)]

Thank you, Olivier. I'll change that later this day. :)

[Niklas Wenzel (nikwen)]

Here you go.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:871
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1426/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1092
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/513
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/184
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/184
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/184/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/184
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/971
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1090
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1090/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17652
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/416
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/622
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/622/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1426/rebuild

review: Needs Fixing (continuous-integration)

[Niklas Wenzel (nikwen)]

Is this actually an issue with my code or with the webbrowser-app in general? I saw the webbrowser_app.tests.test_addressbar_states.TestAddressBarStates.test_looses_focus_when_reloading test fail for other MPs as well, e.g. https://code.launchpad.net/~osomon/webbrowser-app/bottom-edge/+merge/248019, so it might be an issue in trunk.

[Olivier Tilloy (osomon)]

Not an issue with your code, this test is failing on otto (desktop) with other MRs, I need to look into it.

[Olivier Tilloy (osomon)]

Looks good and works well, both on desktop and a mobile device. Thanks!

review: Approve

[Niklas Wenzel (nikwen)]

Thank you, Oliver, for your generally quick replies. And thank you for merging this now. :)

=== added file 'src/app/actions/OpenLinkInNewBackgroundTab.qml'
--- src/app/actions/OpenLinkInNewBackgroundTab.qml	1970-01-01 00:00:00 +0000
+++ src/app/actions/OpenLinkInNewBackgroundTab.qml	2015-01-30 18:14:04 +0000
@@ -0,0 +1,23 @@
+/*
+ * 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 Ubuntu.Components 1.1
+
+Action {
+    text: i18n.tr("Open link in new background tab")
+}

=== modified file 'src/app/webbrowser/Browser.qml'
--- src/app/webbrowser/Browser.qml	2014-12-17 09:22:40 +0000
+++ src/app/webbrowser/Browser.qml	2015-01-30 18:14:04 +0000
@@ -37,6 +37,7 @@
 
     property url homepage
     property QtObject searchEngine
+    property string allowOpenInBackgroundTab
 
     // XXX: we might want to tweak this value depending
     // on the form factor and/or the available memory
@@ -357,6 +358,11 @@
                         enabled: contextualData.href.toString()
                         onTriggered: browser.openUrlInNewTab(contextualData.href, true)
                     }
+                    Actions.OpenLinkInNewBackgroundTab {
+                        enabled: contextualData.href.toString() && ((browser.allowOpenInBackgroundTab === "true") ||
+                                 ((browser.allowOpenInBackgroundTab === "default") && (formFactor === "desktop")))
+                        onTriggered: browser.openUrlInNewTab(contextualData.href, false)
+                    }
                     Actions.BookmarkLink {
                         enabled: contextualData.href.toString() && browser.bookmarksModel
                         onTriggered: browser.bookmarksModel.add(contextualData.href, contextualData.title, "")

=== modified file 'src/app/webbrowser/settings.cpp'
--- src/app/webbrowser/settings.cpp	2014-06-30 10:40:59 +0000
+++ src/app/webbrowser/settings.cpp	2015-01-30 18:14:04 +0000
@@ -32,6 +32,7 @@
     m_homepage = settings.value("homepage", QUrl(DEFAULT_HOMEPAGE)).toUrl();
     QString name = settings.value("searchengine", QString(DEFAULT_SEARCH_ENGINE)).toString();
     m_searchengine = new SearchEngine(name, this);
+    m_allowOpenInBackgroundTab = settings.value("allowOpenInBackgroundTab", "default").toString().toLower();
 }
 
 const QUrl& Settings::homepage() const
@@ -43,3 +44,8 @@
 {
     return m_searchengine;
 }
+
+const QString& Settings::allowOpenInBackgroundTab() const
+{
+    return m_allowOpenInBackgroundTab;
+}

=== modified file 'src/app/webbrowser/settings.h'
--- src/app/webbrowser/settings.h	2014-06-30 10:40:59 +0000
+++ src/app/webbrowser/settings.h	2015-01-30 18:14:04 +0000
@@ -38,10 +38,12 @@
 
     const QUrl& homepage() const;
     SearchEngine* searchEngine() const;
+    const QString& allowOpenInBackgroundTab() const;
 
 private:
     QUrl m_homepage;
     SearchEngine* m_searchengine;
+    QString m_allowOpenInBackgroundTab; //"true" for enabled, "default" for form factor dependend behaviour (currently desktop only), anything else disables the option
 };
 
 #endif // __SETTINGS_H__

=== modified file 'src/app/webbrowser/webbrowser-app.cpp'
--- src/app/webbrowser/webbrowser-app.cpp	2014-12-11 17:04:20 +0000
+++ src/app/webbrowser/webbrowser-app.cpp	2015-01-30 18:14:04 +0000
@@ -100,6 +100,7 @@
         searchEngine->setParent(m_window);
         m_window->setProperty("homepage", settings.homepage());
         m_window->setProperty("searchEngine", QVariant::fromValue(searchEngine));
+        m_window->setProperty("allowOpenInBackgroundTab", settings.allowOpenInBackgroundTab());
         m_window->setProperty("restoreSession", !m_arguments.contains("--new-session"));
         QVariantList urls;
         Q_FOREACH(const QUrl& url, this->urls()) {

=== modified file 'src/app/webbrowser/webbrowser-app.qml'
--- src/app/webbrowser/webbrowser-app.qml	2014-11-03 17:01:29 +0000
+++ src/app/webbrowser/webbrowser-app.qml	2015-01-30 18:14:04 +0000
@@ -25,6 +25,7 @@
 
     property alias searchEngine: browser.searchEngine
     property alias restoreSession: browser.restoreSession
+    property alias allowOpenInBackgroundTab: browser.allowOpenInBackgroundTab
 
     property alias homepage: browser.homepage
     property alias urls: browser.initialUrls