Merge lp:~uriboni/webbrowser-app/tab-context-menu into lp:webbrowser-app
Status: | Rejected |
---|---|
Rejected by: | Olivier Tilloy on 2015-09-23 |
Proposed branch: | lp:~uriboni/webbrowser-app/tab-context-menu |
Merge into: | lp:webbrowser-app |
Diff against target: |
842 lines (+455/-96) 8 files modified
src/app/webbrowser/Browser.qml (+6/-5) src/app/webbrowser/Chrome.qml (+2/-2) src/app/webbrowser/TabsBar.qml (+42/-3) src/app/webbrowser/tabs-model.cpp (+53/-21) src/app/webbrowser/tabs-model.h (+2/-0) tests/unittests/qml/qml_tree_helpers.js (+43/-0) tests/unittests/qml/tst_TabsBar.qml (+137/-8) tests/unittests/tabs-model/tst_TabsModelTests.cpp (+170/-57) |
To merge this branch: | bzr merge lp:~uriboni/webbrowser-app/tab-context-menu |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Olivier Tilloy | 2015-08-17 | Disapprove on 2015-09-23 | |
PS Jenkins bot | continuous-integration | Needs Fixing on 2015-09-17 | |
Riccardo Padovani (community) | Needs Fixing on 2015-08-31 | ||
Review via email:
|
Commit message
Add a context menu to each tab in the tab bar, allowing to insert a new tab just after, close or reload the current tab.
Description of the change
Add a context menu to each tab in the tab bar, allowing to insert a new tab just after, close or reload the current tab
- 1135. By Ugo Riboni on 2015-08-17
-
Merge changes from trunk
- 1136. By Ugo Riboni on 2015-08-18
-
Access the contextual menu items in a way that is safer and will work regardless of the language used by the system running the tests
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1136
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Riccardo Padovani (rpadovani) wrote : | # |
Cool feature!
Anyway, it needs fix:
## Address bar isn't alway cleaned:
- Open the browser with only one tab
- Open another tab and go to a website
- While on the second tab, open the context menu of the first tab
- Two errors (related, I think): the tab switches to the new tab, and the address bar has the old address.
I think you need to incremente current index if you add a tab to the left of the current tab.
## Closing a left tab is broken
Similar to the previous issue: if you close a tab that is left of the current tab, and you've another tab at right, the tab selector moves to right. You need to decrement the current tab index
## Core dump
Not sure if related (but I wasn't able to have it in trunk) but I've a core dump if I open and close two tabs and navigate in both to the same address: http://
- 1137. By Ugo Riboni on 2015-09-02
-
Fix wrong corner cases in TabsModel related to insert/remove and interactions with currentIndex. Refactor unit tests to more clearly and thouroughly test everything.
- 1138. By Ugo Riboni on 2015-09-02
-
Merge changes from trunk
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1137
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1138
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1137
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Olivier Tilloy (osomon) wrote : | # |
This branch has conflicts when merging into the latest trunk. Can you please resolve them?
- 1139. By Ugo Riboni on 2015-09-14
-
Merge changes from trunk
Ugo Riboni (uriboni) wrote : | # |
> This branch has conflicts when merging into the latest trunk. Can you please
> resolve them?
The conflicting changes were related to the tabs model, which you changed for the "sad tab" MR. The change was about correctly setting the current index when a tab is removed before the current tab (rev 1002.3.23), and corresponding additions to the unit tests.
My branch does the same thing, even if in a different way, so I simply reverted the two conflicting files to my version when merging from trunk. As far as I can tell everything should work as you intended and should be tested too, but please double check.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1139
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 1140. By Ugo Riboni on 2015-09-16
-
Merge changes from trunk
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1140
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Olivier Tilloy (osomon) wrote : | # |
In src/app/
var menu = PopupUtils.
menu.
'targetIndex' should be set at instantiation time, passing properties to PopupUtils.open()
if (tab.url.
This won’t work if the webview hasn’t been instantiated (they aren’t until the tabs are first activated). You’ll need to check whether the webview is not null before calling reload().
In src/app/
if (index < 0) index == 0;
Looks like an assignment was meant, but it’s a silent comparison instead.
In TabsModel:
In tests/unittests
function qmlType() can be simplified, by always returning String(
findChildre
In tests/unittests
it looks like bug #1205144 was fixed in the UITK, can the tests be simplified now?
In tests/unittests
there are inconsistent indentations (4 spaces mixed with 2 spaces)
Olivier Tilloy (osomon) wrote : | # |
Superseded by https:/
FAILED: Continuous integration, rev:1135 jenkins. qa.ubuntu. com/job/ webbrowser- app-ci/ 2111/ jenkins. qa.ubuntu. com/job/ generic- deb-autopilot- vivid-touch/ 3755 jenkins. qa.ubuntu. com/job/ webbrowser- app-vivid- amd64-ci/ 865 jenkins. qa.ubuntu. com/job/ webbrowser- app-vivid- armhf-ci/ 865 jenkins. qa.ubuntu. com/job/ webbrowser- app-vivid- armhf-ci/ 865/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ webbrowser- app-vivid- i386-ci/ 865 jenkins. qa.ubuntu. com/job/ generic- deb-autopilot- runner- vivid-mako/ 3099 jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- vivid-armhf/ 3752 jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- vivid-armhf/ 3752/artifact/ work/output/ *zip*/output. zip s-jenkins. ubuntu- ci:8080/ job/touch- flash-device/ 22620
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/webbrowser- app-ci/ 2111/rebuild
http://