Merge lp:~fboucault/webbrowser-app/background_open_tabs_adjacent into lp:webbrowser-app/staging

Proposed by Florian Boucault on 2017-02-02
Status: Merged
Merged at revision: 1609
Proposed branch: lp:~fboucault/webbrowser-app/background_open_tabs_adjacent
Merge into: lp:webbrowser-app/staging
Diff against target: 255 lines (+96/-15)
7 files modified
src/app/webbrowser/TabComponent.qml (+14/-11)
src/app/webbrowser/tabs-model.cpp (+9/-0)
src/app/webbrowser/tabs-model.h (+1/-0)
tests/autopilot/webbrowser_app/tests/__init__.py (+9/-0)
tests/autopilot/webbrowser_app/tests/test_contextmenu.py (+30/-4)
tests/autopilot/webbrowser_app/tests/test_tabs.py (+20/-0)
tests/unittests/tabs-model/tst_TabsModelTests.cpp (+13/-0)
To merge this branch: bzr merge lp:~fboucault/webbrowser-app/background_open_tabs_adjacent
Reviewer Review Type Date Requested Status
Olivier Tilloy 2017-02-02 Approve on 2017-02-15
Review via email: mp+316226@code.launchpad.net

This proposal supersedes a proposal from 2017-02-01.

Commit message

Make new tabs opened in the background to be placed next to the tab opening requesting them instead of at the end of the list of tabs.

Description of the change

Make new tabs opened in the background to be placed next to the tab opening requesting them instead of at the end of the list of tabs.

To post a comment you must log in.
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal

Can you please re-target the merge request to lp:webbrowser-app/staging?

The "Open link in new tab" and "Open link in new background tab" context menu entries should also open the new tab next to the caller (and there should be autopilot tests for those use cases).

Also, see one minor comment inline.

review: Needs Fixing
1602. By Florian Boucault on 2017-02-02

Also apply opening new tab adjacent to current tab when using the following contextual actions:
- opening a link in a new tab
- opening a link in a new background tab
- opening an image in a new tab
- opening a video in a new tab

Olivier Tilloy (osomon) wrote :

I am getting 17 failures out of 20 tests when running the test_contextmenu autopilot tests in narrow mode on desktop (gotta modify the initial window width to 50 GU in src/app/BrowserWindow.qml). This is a regression.

review: Needs Fixing
1603. By Florian Boucault on 2017-02-02

TabsModel unit test: deallocate memory properly

1604. By Florian Boucault on 2017-02-02

Fix AP tests in narrow mode

Olivier Tilloy (osomon) wrote :

I just observed that in chromium on desktop, when closing a tab that had been opened from another tab, the parent tab becomes current again, whereas in webbrowser-app the tab to the right of the one being closed becomes current. Maybe not something we want to address in this branch, but it would be a nice improvement to build on top of this branch.

Olivier Tilloy (osomon) wrote :

webbrowser_app.tests.test_tabs.TestTabsManagement.test_ctrl_click_open_link_in_next_tab appears to be reliably failing in narrow mode.

review: Needs Fixing
Olivier Tilloy (osomon) wrote :

> Maybe not something we want to address in this branch, but it would be
> a nice improvement to build on top of this branch.

Actually, I think it would make sense to implement this in this branch. I guess we need to add a property to BrowserTab so that it remembers its "parent" tab, and restores focus on it when closed.

Florian Boucault (fboucault) wrote :

> > Maybe not something we want to address in this branch, but it would be
> > a nice improvement to build on top of this branch.
>
> Actually, I think it would make sense to implement this in this branch. I
> guess we need to add a property to BrowserTab so that it remembers its
> "parent" tab, and restores focus on it when closed.

I think this behaviour could have been implemented before this branch. Therefore I would say it is very independent and think we should implement it separately.

1605. By Florian Boucault on 2017-02-15

Merged staging

1606. By Florian Boucault on 2017-02-15

Fixed AP test test_ctrl_click_open_link_in_next_tab in narrow mode

Olivier Tilloy (osomon) wrote :

LGTM, thanks.

review: Approve
Olivier Tilloy (osomon) wrote :

> I think this behaviour could have been implemented before this branch.
> Therefore I would say it is very independent and think we should
> implement it separately.

Filed bug #1664990 to track its implementation.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/webbrowser/TabComponent.qml'
2--- src/app/webbrowser/TabComponent.qml 2016-11-25 15:49:50 +0000
3+++ src/app/webbrowser/TabComponent.qml 2017-02-15 08:26:53 +0000
4@@ -33,6 +33,7 @@
5 id: tabComponent
6
7 BrowserTab {
8+ id: browserTab
9 anchors.fill: parent
10 incognito: browser ? browser.incognito : false
11 current: browser ? browser.tabsModel && browser.tabsModel.currentTab === this : false
12@@ -84,12 +85,14 @@
13 Actions.OpenLinkInNewTab {
14 objectName: "OpenLinkInNewTabContextualAction"
15 enabled: contextModel && contextModel.linkUrl.toString()
16- onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, true)
17+ onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, true,
18+ true, tabsModel.indexOf(browserTab) + 1)
19 }
20 Actions.OpenLinkInNewBackgroundTab {
21 objectName: "OpenLinkInNewBackgroundTabContextualAction"
22 enabled: contextModel && contextModel.linkUrl.toString()
23- onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, false)
24+ onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, false,
25+ true, tabsModel.indexOf(browserTab) + 1)
26 }
27 Actions.OpenLinkInNewWindow {
28 objectName: "OpenLinkInNewWindowContextualAction"
29@@ -142,7 +145,8 @@
30 enabled: contextModel &&
31 (contextModel.mediaType === Oxide.WebView.MediaTypeImage) &&
32 contextModel.srcUrl.toString()
33- onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true)
34+ onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true,
35+ true, tabsModel.indexOf(browserTab) + 1)
36 }
37 Actions.CopyImage {
38 objectName: "CopyImageContextualAction"
39@@ -165,7 +169,8 @@
40 enabled: contextModel &&
41 (contextModel.mediaType === Oxide.WebView.MediaTypeVideo) &&
42 contextModel.srcUrl.toString()
43- onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true)
44+ onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true,
45+ true, tabsModel.indexOf(browserTab) + 1)
46 }
47 Actions.SaveVideo {
48 objectName: "SaveVideoContextualAction"
49@@ -253,9 +258,9 @@
50 contextMenu: browser && browser.wide ? contextMenuWideComponent : contextMenuNarrowComponent
51
52 onNewViewRequested: {
53- var tab = browser.createTab({"request": request})
54+ var newTab = browser.createTab({"request": request})
55 var setCurrent = (request.disposition == Oxide.NewViewRequest.DispositionNewForegroundTab)
56- internal.addTab(tab, setCurrent)
57+ internal.addTab(newTab, setCurrent, tabsModel.indexOf(browserTab) + 1)
58 if (setCurrent) tabContainer.forceActiveFocus()
59 }
60
61@@ -263,11 +268,9 @@
62 onPrepareToCloseResponse: {
63 if (proceed) {
64 if (tab) {
65- for (var i = 0; i < tabsModel.count; ++i) {
66- if (tabsModel.get(i) === tab) {
67- tabsModel.remove(i)
68- break
69- }
70+ var i = tabsModel.indexOf(tab);
71+ if (i != -1) {
72+ tabsModel.remove(i);
73 }
74
75 // tab.close() destroys the context so add new tab before destroy if required
76
77=== modified file 'src/app/webbrowser/tabs-model.cpp'
78--- src/app/webbrowser/tabs-model.cpp 2015-12-03 10:47:22 +0000
79+++ src/app/webbrowser/tabs-model.cpp 2017-02-15 08:26:53 +0000
80@@ -203,6 +203,15 @@
81 return m_tabs.at(index);
82 }
83
84+/*!
85+ Returns the index position of the first occurrence of tab in the model.
86+ Returns -1 if no item matched.
87+*/
88+int TabsModel::indexOf(QObject* tab) const
89+{
90+ return m_tabs.indexOf(tab);
91+}
92+
93 void TabsModel::move(int from, int to)
94 {
95 if ((from == to) || !checkValidTabIndex(from) || !checkValidTabIndex(to)) {
96
97=== modified file 'src/app/webbrowser/tabs-model.h'
98--- src/app/webbrowser/tabs-model.h 2015-09-02 16:37:20 +0000
99+++ src/app/webbrowser/tabs-model.h 2017-02-15 08:26:53 +0000
100@@ -60,6 +60,7 @@
101 Q_INVOKABLE int insert(QObject* tab, int index);
102 Q_INVOKABLE QObject* remove(int index);
103 Q_INVOKABLE QObject* get(int index) const;
104+ Q_INVOKABLE int indexOf(QObject* tab) const;
105 Q_INVOKABLE void move(int from, int to);
106
107 Q_SIGNALS:
108
109=== modified file 'tests/autopilot/webbrowser_app/tests/__init__.py'
110--- tests/autopilot/webbrowser_app/tests/__init__.py 2016-11-08 15:03:01 +0000
111+++ tests/autopilot/webbrowser_app/tests/__init__.py 2017-02-15 08:26:53 +0000
112@@ -176,6 +176,15 @@
113
114 return new_tab_view
115
116+ def switch_to_tab(self, index):
117+ if self.main_window.wide:
118+ tab = self.main_window.chrome.get_tabs_bar().get_tab(index)
119+ self.pointing_device.click_object(tab)
120+ else:
121+ tabs_view = self.open_tabs_view()
122+ tabs_view.get_previews()[index].select()
123+ tabs_view.visible.wait_for(False)
124+
125 def open_new_window(self):
126 windows = self.app.get_windows(incognito=False)
127 chrome = self.main_window.chrome
128
129=== modified file 'tests/autopilot/webbrowser_app/tests/test_contextmenu.py'
130--- tests/autopilot/webbrowser_app/tests/test_contextmenu.py 2016-09-21 07:33:26 +0000
131+++ tests/autopilot/webbrowser_app/tests/test_contextmenu.py 2017-02-15 08:26:53 +0000
132@@ -28,6 +28,8 @@
133
134 def setUp(self, path):
135 super(TestContextMenuBase, self).setUp(path)
136+ self.open_new_tab(open_tabs_view=not self.main_window.wide)
137+ self.switch_to_tab(0 if self.main_window.wide else 1)
138 self.menu = self.main_window.open_context_menu()
139 # The context menu should never steal active focus from the webview,
140 # but because it’s currently implemented as a dialog on narrow screens,
141@@ -36,8 +38,16 @@
142 webview = self.main_window.get_current_webview()
143 self.assertThat(webview.activeFocus, Equals(True))
144
145- def verify_link_opened_in_a_new_tab(self):
146- self.assert_number_webviews_eventually(2)
147+ def verify_link_opened_in_a_new_tab(self, background=False):
148+ self.assert_number_webviews_eventually(3)
149+ # make sure the new tab is adjacent to the tab that created it,
150+ # in other words that its index is 1 in wide mode, 0 in narrow mode
151+ # http://pad.lv/1499780
152+ if not self.main_window.wide and not background:
153+ new_tab_index = 0
154+ else:
155+ new_tab_index = 1
156+ self.switch_to_tab(new_tab_index)
157 webview = self.main_window.get_current_webview()
158 new_url = self.base_url + "/test1"
159 self.assertThat(webview.url, Eventually(Equals(new_url)))
160@@ -48,8 +58,16 @@
161 self.main_window.wait_until_page_loaded(url)
162 self.main_window.chrome.bookmarked.wait_for(True)
163
164- def verify_image_opened_in_a_new_tab(self):
165- self.assert_number_webviews_eventually(2)
166+ def verify_image_opened_in_a_new_tab(self, background=False):
167+ self.assert_number_webviews_eventually(3)
168+ # make sure the new tab is adjacent to the tab that created it,
169+ # in other words that its index is 1 in wide mode, 0 in narrow mode
170+ # http://pad.lv/1499780
171+ if not self.main_window.wide and not background:
172+ new_tab_index = 0
173+ else:
174+ new_tab_index = 1
175+ self.switch_to_tab(new_tab_index)
176 webview = self.main_window.get_current_webview()
177 self.assertThat(webview.url,
178 Eventually(StartsWith(self.data_uri_prefix)))
179@@ -79,6 +97,10 @@
180 self.menu.click_action("OpenLinkInNewTabContextualAction")
181 self.verify_link_opened_in_a_new_tab()
182
183+ def test_open_link_in_new_background_tab(self):
184+ self.menu.click_action("OpenLinkInNewBackgroundTabContextualAction")
185+ self.verify_link_opened_in_a_new_tab(background=True)
186+
187 def test_open_link_in_new_window(self):
188 self.menu.click_action("OpenLinkInNewWindowContextualAction")
189 self.verify_link_opened_in_a_new_window(False)
190@@ -132,6 +154,10 @@
191 self.menu.click_action("OpenLinkInNewTabContextualAction")
192 self.verify_link_opened_in_a_new_tab()
193
194+ def test_open_link_in_new_background_tab(self):
195+ self.menu.click_action("OpenLinkInNewBackgroundTabContextualAction")
196+ self.verify_link_opened_in_a_new_tab(background=True)
197+
198 def test_open_link_in_new_window(self):
199 self.menu.click_action("OpenLinkInNewWindowContextualAction")
200 self.verify_link_opened_in_a_new_window(False)
201
202=== modified file 'tests/autopilot/webbrowser_app/tests/test_tabs.py'
203--- tests/autopilot/webbrowser_app/tests/test_tabs.py 2016-08-10 15:43:04 +0000
204+++ tests/autopilot/webbrowser_app/tests/test_tabs.py 2017-02-15 08:26:53 +0000
205@@ -222,6 +222,26 @@
206 self.assertThat(len(views), Equals(1))
207 self.check_current_tab(url)
208
209+ # http://pad.lv/1499780
210+ @testtools.skipIf(model() != "Desktop", "on desktop only")
211+ def test_ctrl_click_open_link_in_next_tab(self):
212+ self.open_new_tab(open_tabs_view=not self.main_window.wide)
213+ self.switch_to_tab(0 if self.main_window.wide else 1)
214+
215+ url = self.base_url + "/link"
216+ self.main_window.go_to_url(url)
217+ self.main_window.wait_until_page_loaded(url)
218+ webview = self.main_window.get_current_webview()
219+
220+ self.keyboard.press('Ctrl')
221+ self.pointing_device.click_object(webview)
222+ self.keyboard.release('Ctrl')
223+
224+ # Eventually we should have three webviews
225+ self.assert_number_webviews_eventually(3)
226+ self.switch_to_tab(1)
227+ self.check_current_tab(self.url)
228+
229 def test_open_iframe_target_blank_in_new_tab(self):
230 url = self.base_url + "/fulliframewithblanktargetlink"
231 self.main_window.go_to_url(url)
232
233=== modified file 'tests/unittests/tabs-model/tst_TabsModelTests.cpp'
234--- tests/unittests/tabs-model/tst_TabsModelTests.cpp 2016-04-04 10:17:33 +0000
235+++ tests/unittests/tabs-model/tst_TabsModelTests.cpp 2017-02-15 08:26:53 +0000
236@@ -486,6 +486,19 @@
237 QCOMPARE(model->get(3), (QObject*) nullptr);
238 }
239
240+ void shouldReturnTabIndex()
241+ {
242+ QQuickItem* tab1 = createTab();
243+ model->add(tab1);
244+ QQuickItem* tab2 = createTab();
245+ model->add(tab2);
246+ QQuickItem* nonAddedTab = createTab();
247+ QCOMPARE(model->indexOf(tab1), 0);
248+ QCOMPARE(model->indexOf(tab2), 1);
249+ QCOMPARE(model->indexOf(nonAddedTab), -1);
250+ delete nonAddedTab;
251+ }
252+
253 private:
254 void moveTabs(int from, int to, bool moved, bool indexChanged, int newIndex)
255 {

Subscribers

People subscribed via source and target branches