[Merge] lp:~ahayzen/webbrowser-app/migrate-to-tabs-component into lp:webbrowser-app/staging
Olivier Tilloy
olivier.tilloy at canonical.com
Wed Feb 8 16:23:00 UTC 2017
Review: Needs Fixing
ubuntu-ui-extras is being added to the platform snap (see https://code.launchpad.net/~osomon/ubuntu-app-platform/+git/ubuntu-app-platform/+merge/316684), so you can remove the extra logic from the packaging.
Can you please update the copyright year of all changed files to 2017 ?
When I right-click on a tab and choose "Move to New Window", a new window is indeed opened, but the tab remains (and the webview is blanked) in the original window. I’m seeing the following in the console:
file://…/src/app/webbrowser/TabsBar.qml:122: TypeError: Property 'removeMovingTab' of object TabsBar_QMLTYPE_198(0x2f79320) is not a function
(a new autopilot test for that context menu option would be useful)
When I right-click on a tab and choose "New Tab", the new tab is not getting focus. This doesn't appear to be a regression, but would you mind fixing this while you're at it?
Please see additional comments/questions inline.
Diff comments:
> === modified file 'debian/control'
> --- debian/control 2017-01-05 10:31:30 +0000
> +++ debian/control 2017-02-06 16:56:42 +0000
> @@ -46,6 +46,7 @@
> fonts-liberation,
> liboxideqt-qmlplugin (>= 1.19),
> libqt5sql5-sqlite,
> + qml-module-qtqml-models2,
What is this module needed for? I’m not seeing any explicit import of QtQml.Models in the diff.
> qml-module-qt-labs-folderlistmodel,
> qml-module-qt-labs-settings,
> qml-module-qtquick2 (>= 5.4),
>
> === modified file 'po/webbrowser-app.pot'
> --- po/webbrowser-app.pot 2016-12-18 18:43:50 +0000
> +++ po/webbrowser-app.pot 2017-02-06 16:56:42 +0000
Please revert changes to the pot file.
> @@ -8,7 +8,7 @@
> msgstr ""
> "Project-Id-Version: webbrowser-app\n"
> "Report-Msgid-Bugs-To: \n"
> -"POT-Creation-Date: 2016-12-18 19:37+0100\n"
> +"POT-Creation-Date: 2017-02-03 15:07+0000\n"
> "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
> "Last-Translator: FULL NAME <EMAIL at ADDRESS>\n"
> "Language-Team: LANGUAGE <LL at li.org>\n"
> @@ -378,7 +378,7 @@
> msgid "Erase"
> msgstr ""
>
> -#: src/app/actions/FindInPage.qml:23 src/app/webbrowser/Browser.qml:572
> +#: src/app/actions/FindInPage.qml:23 src/app/webbrowser/Browser.qml:610
> msgid "Find in page"
> msgstr ""
>
> @@ -408,8 +408,8 @@
> msgid "Address;URL;www"
> msgstr ""
>
> -#: src/app/actions/NewTab.qml:23 src/app/webbrowser/Browser.qml:441
> -#: src/app/webbrowser/TabsBar.qml:110
> +#: src/app/actions/NewTab.qml:23 src/app/webbrowser/Browser.qml:477
> +#: src/app/webbrowser/TabsBar.qml:98
> msgid "New Tab"
> msgstr ""
>
> @@ -456,7 +456,7 @@
> msgstr ""
>
> #: src/app/actions/Reload.qml:23 src/app/webbrowser/SadTab.qml:86
> -#: src/app/webbrowser/TabsBar.qml:115 src/app/webcontainer/SadPage.qml:51
> +#: src/app/webbrowser/TabsBar.qml:103 src/app/webcontainer/SadPage.qml:51
> msgid "Reload"
> msgstr ""
>
> @@ -482,7 +482,7 @@
> msgid "Select all"
> msgstr ""
>
> -#: src/app/actions/Share.qml:22 src/app/webbrowser/Browser.qml:552
> +#: src/app/actions/Share.qml:22 src/app/webbrowser/Browser.qml:590
> msgid "Share"
> msgstr ""
>
> @@ -547,14 +547,14 @@
>
> #: src/app/webbrowser/BookmarksView.qml:32
> #: src/app/webbrowser/BookmarksViewWide.qml:32
> -#: src/app/webbrowser/Browser.qml:560 src/app/webbrowser/NewTabView.qml:130
> +#: src/app/webbrowser/Browser.qml:598 src/app/webbrowser/NewTabView.qml:130
> #: src/app/webbrowser/NewTabViewWide.qml:139
> msgid "Bookmarks"
> msgstr ""
>
> #: src/app/webbrowser/BookmarksView.qml:76
> #: src/app/webbrowser/BookmarksViewWide.qml:75
> -#: src/app/webbrowser/Browser.qml:427 src/app/webbrowser/HistoryView.qml:126
> +#: src/app/webbrowser/Browser.qml:463 src/app/webbrowser/HistoryView.qml:126
> #: src/app/webbrowser/HistoryViewWide.qml:407
> msgid "Done"
> msgstr ""
> @@ -562,34 +562,34 @@
> #: src/app/webbrowser/BookmarksView.qml:90
> #: src/app/webbrowser/BookmarksViewWide.qml:89
> #: src/app/webbrowser/HistoryView.qml:140
> -#: src/app/webbrowser/HistoryViewWide.qml:421
> -#: src/app/webbrowser/TabsBar.qml:191 src/app/webbrowser/TabsList.qml:99
> +#: src/app/webbrowser/HistoryViewWide.qml:421 src/app/webbrowser/TabsBar.qml:74
> +#: src/app/webbrowser/TabsList.qml:99
> msgid "New tab"
> msgstr ""
>
> -#: src/app/webbrowser/Browser.qml:540
> +#: src/app/webbrowser/Browser.qml:578
> msgid "New window"
> msgstr ""
>
> -#: src/app/webbrowser/Browser.qml:546
> +#: src/app/webbrowser/Browser.qml:584
> msgid "New private window"
> msgstr ""
>
> -#: src/app/webbrowser/Browser.qml:566 src/app/webbrowser/HistoryView.qml:30
> +#: src/app/webbrowser/Browser.qml:604 src/app/webbrowser/HistoryView.qml:30
> #: src/app/webbrowser/HistoryViewWide.qml:35
> msgid "History"
> msgstr ""
>
> -#: src/app/webbrowser/Browser.qml:579 src/app/webbrowser/DownloadsPage.qml:46
> +#: src/app/webbrowser/Browser.qml:617 src/app/webbrowser/DownloadsPage.qml:46
> msgid "Downloads"
> msgstr ""
>
> -#: src/app/webbrowser/Browser.qml:586 src/app/webbrowser/SettingsPage.qml:41
> +#: src/app/webbrowser/Browser.qml:624 src/app/webbrowser/SettingsPage.qml:41
> msgid "Settings"
> msgstr ""
>
> #. TRANSLATORS: %1 refers to the current number of tabs opened
> -#: src/app/webbrowser/Browser.qml:759 src/app/webbrowser/Browser.qml:797
> +#: src/app/webbrowser/Browser.qml:797 src/app/webbrowser/Browser.qml:835
> #, qt-format
> msgid "(%1)"
> msgstr ""
> @@ -829,11 +829,11 @@
> msgid "Tap to view"
> msgstr ""
>
> -#: src/app/webbrowser/TabsBar.qml:121
> +#: src/app/webbrowser/TabsBar.qml:109
> msgid "Move to New Window"
> msgstr ""
>
> -#: src/app/webbrowser/TabsBar.qml:130
> +#: src/app/webbrowser/TabsBar.qml:126
> msgid "Close Tab"
> msgstr ""
>
> @@ -842,13 +842,13 @@
> msgstr ""
>
> #. TRANSLATORS: %1 refers to the current page’s title
> -#: src/app/webbrowser/webbrowser-app.qml:99
> +#: src/app/webbrowser/webbrowser-app.qml:100
> #: src/app/webcontainer/webapp-container.qml:72
> #, qt-format
> msgid "%1 - Ubuntu Web Browser"
> msgstr ""
>
> -#: src/app/webbrowser/webbrowser-app.qml:101
> +#: src/app/webbrowser/webbrowser-app.qml:102
> #: src/app/webcontainer/webapp-container.qml:74
> msgid "Ubuntu Web Browser"
> msgstr ""
>
> === modified file 'snapcraft.yaml'
> --- snapcraft.yaml 2017-01-31 16:25:39 +0000
> +++ snapcraft.yaml 2017-02-06 16:56:42 +0000
> @@ -62,7 +63,9 @@
> - share/webbrowser-app/*.js
> - share/webbrowser-app/*.png
> - share/webbrowser-app/*.qml
> + - usr/lib/*/libexiv2.so.*
> - usr/lib/*/qt5/qml/Ubuntu/Web
> + - usr/lib/*/qt5/qml/Ubuntu/Components/Extras
ubuntu-ui-extras is being added to the platform snap (see https://code.launchpad.net/~osomon/ubuntu-app-platform/+git/ubuntu-app-platform/+merge/316684), so those changes should be reverted.
> - usr/share/fonts
>
> launcher:
> @@ -70,3 +73,4 @@
> source: snap
> organize:
> webbrowser-app.launcher: bin/webbrowser-app.launcher
> +
Please remove the extra blank line.
>
> === modified file 'src/app/webbrowser/Browser.qml'
> --- src/app/webbrowser/Browser.qml 2017-01-05 10:31:30 +0000
> +++ src/app/webbrowser/Browser.qml 2017-02-06 16:56:42 +0000
> @@ -44,9 +44,45 @@
>
> property bool incognito: false
>
> - property var tabsModel: TabsModel {}
> + property var tabsModel: TabsModel {
> + // These methods are required by the TabsBar component
> + property int selectedIndex: currentIndex
Please make it a readonly property.
> +
> + function addTab() {
> + internal.openUrlInNewTab("", true, true, count)
> + }
> +
> + function addExistingTab(tab) {
> + add(tab);
> +
> + browser.bindExistingTab(tab);
> + }
> +
> + function moveTab(from, to) {
> + if (from == to
> + || from < 0 || from >= count
> + || to < 0 || to >= count) {
> + return;
> + }
> +
> + move(from, to);
> + }
> +
> + function removeTab(index) {
> + internal.closeTab(index, false);
> + }
> +
> + function removeTabWithoutDestroying(index) {
> + internal.closeTab(index, true);
> + }
> +
> + function selectTab(index) {
> + internal.switchToTab(index, true);
> + }
> + }
>
> property BrowserWindow thisWindow
> + property Component windowFactory
>
> function serializeTabState(tab) {
> var state = {}
> @@ -1118,6 +1156,8 @@
> maybeFocusAddressBar()
> } else {
> tabContainer.forceActiveFocus()
> +
Can you please remove the extra blank line?
> + tab.load();
> }
> }
> }
>
> === added file 'src/app/webbrowser/TabsBar.qml'
> --- src/app/webbrowser/TabsBar.qml 1970-01-01 00:00:00 +0000
> +++ src/app/webbrowser/TabsBar.qml 2017-02-06 16:56:42 +0000
> @@ -0,0 +1,133 @@
> +/*
> + * Copyright 2013-2016 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.4
> +import Ubuntu.Components 1.3
> +import Ubuntu.Components.Popups 1.3
> +import "."
> +import ".."
> +import webbrowsercommon.private 0.1
> +
> +import Ubuntu.Components.Extras 0.3 as Extras
Cosmetics: can this import be grouped together with the other Ubuntu.Components imports?
> +
> +Extras.TabsBar {
> + id: tabsBar
> + color: "#D9D9D9" // FIXME: not in palette hardcode for now
> + dragAndDrop {
> + enabled: __platformName != "ubuntumirclient"
> + maxYDiff: height / 12
> + mimeType: "webbrowser/tab-" + (incognito ? "incognito" : "public")
> + previewUrlFromIndex: function(index) {
> + if (tabsBar.model.get(index)) {
> + return PreviewManager.previewPathFromUrl(tabsBar.model.get(index).url)
> + } else {
> + return "";
> + }
> + }
> + }
> + fallbackIcon: "stock_website"
> + windowFactoryProperties: {
> + "incognito": tabsBar.incognito,
> + "height": window.height,
> + "width": window.width,
> + }
> +
> + property bool incognito
> +
> + signal requestNewTab(int index, bool makeCurrent)
> + signal tabClosed(int index, bool moving)
> +
> + onContextMenu: PopupUtils.open(contextualOptionsComponent, tabDelegate, {"targetIndex": index})
> +
> + property Component faviconFactory: Component {
Does faviconFactory really need to be a property? Can't it simply be a nested Component?
> + FaviconFetcher {
> +
> + }
> + }
> +
> + function iconSourceFromModelItem(modelData, index) {
> + var incubator = faviconFactory.incubateObject(
> + parent,
> + {
> + "shouldCache": Qt.binding(function() { return !incognito; }),
> + "url": Qt.binding(function() { return modelData.icon || ""; })
> + }
> + );
> + return incubator.status == Component.Ready ? incubator.object.localUrl || "" : "";
I wonder how this actually works. The incubator may instantiate the component asynchronously, so by the time the function returns its status might not be Component.Ready, and the icon source will be empty.
Also, why make the FaviconFetcher a child of tabsBar.parent, and not of tabsBar itself?
> + }
> +
> + function titleFromModelItem(modelItem) {
> + return modelItem.title ? modelItem.title : (modelItem.url.toString() ? modelItem.url : i18n.tr("New tab"))
> + }
> +
> + actions: [
> + Action {
> + // FIXME: icon from theme is fuzzy at many GUs
> +// iconSource: Qt.resolvedUrl("Tabs/tab_add.png")
> + iconName: "add"
> + objectName: "newTabButton"
> + onTriggered: tabsBar.model.addTab()
> + }
> + ]
> +
> + Component {
> + id: contextualOptionsComponent
> + ActionSelectionPopover {
> + id: menu
> + objectName: "tabContextualActions"
> + property int targetIndex
> + readonly property var tab: tabsBar.model.get(targetIndex)
> +
> + actions: ActionList {
> + Action {
> + objectName: "tab_action_new_tab"
> + text: i18n.tr("New Tab")
> + onTriggered: tabsBar.requestNewTab(menu.targetIndex + 1, false)
> + }
> + Action {
> + objectName: "tab_action_reload"
> + text: i18n.tr("Reload")
> + enabled: menu.tab.url.toString().length > 0
> + onTriggered: menu.tab.reload()
> + }
> + Action {
> + objectName: "tab_action_move_to_new_window"
> + text: i18n.tr("Move to New Window")
> + onTriggered: {
> + // callback function only removes from model
> + // and not destroy as webview is in new window
> + // Create new window and add existing tab
> + var window = tabsBar.windowFactory.createObject(null, windowFactoryProperties);
> + window.model.addExistingTab(menu.tab);
> + window.model.selectTab(window.model.count - 1);
> + window.show();
> +
> + // Just remove from model and do not destroy
> + // as webview is used in other window
> + tabsBar.removeMovingTab(menu.targetIndex);
> + }
> + }
> + Action {
> + objectName: "tab_action_close_tab"
> + text: i18n.tr("Close Tab")
> + onTriggered: tabsBar.tabClosed(menu.targetIndex, false)
> + }
> + }
> + }
> + }
> +}
>
> === modified file 'src/app/webbrowser/tabs-model.cpp'
> --- src/app/webbrowser/tabs-model.cpp 2015-12-03 10:47:22 +0000
> +++ src/app/webbrowser/tabs-model.cpp 2017-02-06 16:56:42 +0000
> @@ -208,9 +208,25 @@
> if ((from == to) || !checkValidTabIndex(from) || !checkValidTabIndex(to)) {
> return;
> }
> - beginMoveRows(QModelIndex(), from, from, QModelIndex(), to);
> - m_tabs.move(from, to);
> - endMoveRows();
> +
> + int diff = to - from;
> + int i=from;
Cosmetics: can you please add whitespaces around the equal sign?
> +
> + // Shuffle index along until destination
> + while (i != to) {
> + if (diff > 0) {
> + beginMoveRows(QModelIndex(), i, i, QModelIndex(), i + 2);
> + m_tabs.move(i + 1, i);
> + i += 1;
> + } else {
> + beginMoveRows(QModelIndex(), i, i, QModelIndex(), i - 1);
> + m_tabs.move(i, i - 1);
> + i -= 1;
> + }
> +
> + endMoveRows();
> + }
So this is like the changes in https://code.launchpad.net/~ahayzen/webbrowser-app/fix-1630211-drag-tabs-quickly-incorrect-position/+merge/307709 (sorry for not letting that one aside for so long), except that the indexes in the first beginMoveRows(…) are different. Why is that so?
> +
> if (m_currentIndex == from) {
> m_currentIndex = to;
> Q_EMIT currentIndexChanged();
>
> === modified file 'src/app/webbrowser/webbrowser-app.qml'
> --- src/app/webbrowser/webbrowser-app.qml 2016-11-08 14:19:52 +0000
> +++ src/app/webbrowser/webbrowser-app.qml 2017-02-06 16:56:42 +0000
> @@ -89,6 +89,7 @@
> id: window
>
> property alias incognito: browser.incognito
> + readonly property alias model: browser.tabsModel
Is this really needed? There's already a 'tabsModel' property (line below).
> readonly property var tabsModel: browser.tabsModel
>
> currentWebview: browser.currentWebview
>
> === modified file 'tests/unittests/qml/tst_TabsBar.qml'
> --- tests/unittests/qml/tst_TabsBar.qml 2016-11-18 15:10:47 +0000
> +++ tests/unittests/qml/tst_TabsBar.qml 2017-02-06 16:56:42 +0000
> @@ -32,6 +34,37 @@
>
> TabsModel {
> id: tabsModel
> +
> + // These methods are required by the TabsBar component
> + property int selectedIndex: currentIndex
> +
> + function addTab() {
> + tabs.requestNewTab(count, true);
> + }
> +
> + function moveTab(from, to) {
> + console.debug("Move!", from, to)
Can this debug statement be removed?
> +
> + if (from == to
> + || from < 0 || from >= count
> + || to < 0 || to >= count) {
> + return;
> + }
> +
> + move(from, to);
> + }
> +
> + // Overload removeTab and add moving property so we can tell when
> + // the tab is closing due to moving to a new window
> + // This is required because we need to avoid destroying the content
> + // of that tab that is moved
> + function removeTab(index, moving) {
> + tabs.tabClosed(index, false);
> + }
> +
> + function selectTab(index) {
> + currentIndex = index;
> + }
> }
>
> Component {
> @@ -58,8 +91,10 @@
> height: 50
>
> model: tabsModel
> - onSwitchToTab: model.currentIndex = index
> +
> + onContextMenu: PopupUtils.open(contextualOptionsComponent, tabDelegate, {"targetIndex": index})
Why do you need to override the onContextMenu handler and define a new contextualOptionsComponent? Wouldn't this work with the stock one provided by the TabsBar component?
> onRequestNewTab: insertTab("", "", "", index)
> +
> function appendTab(url, title, icon) {
> insertTab(url, title, icon, model.count)
> model.currentIndex = model.count - 1
> @@ -220,15 +293,25 @@
>
> function dragTab(tab, dx, index) {
> var c = centerOf(tab)
> - mouseDrag(tab, c.x, c.y, dx, 0)
> - compare(getTabDelegate(index), tab)
> +
> + mousePress(tab, c.x, c.y);
> +
> + // Move tab slowly otherwise it can skip the DropArea
> + for (var j=0; j < dx; j ++) {
Cosmetics: can you please add whitespaces around the equal sign, and remove the one before the ++ operator?
> + mouseMove(tabs, j, c.y)
> + wait(1)
> + }
> +
> + mouseRelease(tab, c.x + dx, c.y);
> +
> + compare(tabsModel.get(index).title, tab.title)
> compare(tabsModel.currentIndex, index)
> wait(500)
> }
>
> // Move the first tab to the right
> - var tab = getTabDelegate(0)
> - dragTab(tab, tab.width * 0.8, 1)
> + var tab = getTabItem(0)
> + dragTab(tab, tab.width, 1)
>
> // Start a move to the right and release too early
> dragTab(tab, tab.width * 0.3, 1)
--
https://code.launchpad.net/~ahayzen/webbrowser-app/migrate-to-tabs-component/+merge/312340
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app/staging.
More information about the Ubuntu-reviews
mailing list