webbrowser-app

Merge lp:~mterry/webbrowser-app/switch-tab-keybinding into lp:webbrowser-app

switch-tab-keybinding
Merge into trunk

Proposed by Michael Terry on 2015-09-28

Status:

Merged

Approved by:

Olivier Tilloy on 2015-10-06

Approved revision:

1205

Merged at revision:

1229

Proposed branch:

lp:~mterry/webbrowser-app/switch-tab-keybinding

Merge into:

lp:webbrowser-app

Diff against target:

96 lines (+40/-18)

2 files modified

src/app/webbrowser/Browser.qml (+34/-18)
tests/autopilot/webbrowser_app/tests/test_keyboard.py (+6/-0)

To merge this branch:

bzr merge lp:~mterry/webbrowser-app/switch-tab-keybinding

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-10-06
Olivier Tilloy		2015-09-28	Approve on 2015-10-06
Review via email: mp+272671@code.launchpad.net

Commit message

Add Ctrl+PageUp and Ctrl+PageDown as alternate keyboard shortcuts for cycling through tabs.

Description of the change

Add Ctrl+PageUp and Ctrl+PageDown as alternate keyboard shortcuts for cycling through tabs.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-29:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-09-29:

LGTM, thanks for this useful contribution.

A couple of minor comments:

- the call to focus recentView should probably be factored out to the implementation of internal.switchToTab()

- can you update the autopilot tests in tests/autopilot/webbrowser_app/tests/test_keyboard.py to also test these new shortcuts (see test_switch_tabs)

lp:~mterry/webbrowser-app/switch-tab-keybinding updated on 2015-09-29

1204. By Michael Terry on 2015-09-29: Add test and consolidate a little code

Revision history for this message

Michael Terry (mterry) wrote on 2015-09-29:

Test added and recentView.focus=true line consolidated.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-09-29:

Thanks!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-29:

FAILED: Continuous integration, rev:1204
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2296/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/4407
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1050
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1050
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1050/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1050
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3586
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4404
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4404/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23754

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Terry (mterry) wrote on 2015-09-30:

I assume those autopilot errors are typical, and not due to this branch?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-09-30:

The autopilot tests are usually fairly stable, but it’s not uncommon to see one or two random failures. The failures here seem completely unrelated to your branch, but given there are 4 of them, which is higher than usual, I’ll trigger a new CI run.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-01:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-01:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-06:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-06:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-10-06:

The following autopilot test is broken:

webbrowser_app.tests.test_tabs.TestTabsManagement.test_selecting_tab_focuses_webview

I can reliably observe the failure on desktop by running the test in narrow mode: modify the width of the window to be 50GU instead of 100GU in src/app/BrowserWindow.qml, and then run the test locally:

cd tests/autopilot
autopilot3 run webbrowser_app.tests.test_tabs.TestTabsManagement.test_selecting_tab_focuses_webview

I’m guessing the change to switchToTab() is what introduced the regression: when selecting a tab from the tabs view (in narrow mode), the recent view is still visible, so we don’t want to focus back the recent view. My bad for suggesting to factor this out, can you please revert that change?

review: Needs Fixing

lp:~mterry/webbrowser-app/switch-tab-keybinding updated on 2015-10-06

1205. By Michael Terry on 2015-10-06: Undo consolidation

Revision history for this message

Michael Terry (mterry) wrote on 2015-10-06:

OK, undid that change. :)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-10-06:

LGTM, let’s see if the CI job passes now.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-06:

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

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Michael Terry

Ubuntu Phablet Team

to status/vote changes:

Rachel Liu

 === modified file 'src/app/webbrowser/Browser.qml'
 --- src/app/webbrowser/Browser.qml	2015-09-28 08:15:10 +0000
 +++ src/app/webbrowser/Browser.qml	2015-10-06 15:46:27 +0000
@@ -1273,6 +1273,24 @@
+             }
+         }
++        function switchToPreviousTab() {
++            if (browser.wide) {
++                internal.switchToTab((tabsModel.currentIndex - 1 + tabsModel.count) % tabsModel.count)
++            } else {
++                internal.switchToTab(tabsModel.count - 1)
++            }
++            if (recentView.visible) recentView.focus = true
++        }
++
++        function switchToNextTab() {
++            if (browser.wide) {
++                internal.switchToTab((tabsModel.currentIndex + 1) % tabsModel.count)
++            } else {
++                internal.switchToTab(tabsModel.count - 1)
++            }
++            if (recentView.visible) recentView.focus = true
++        }
++
          function switchToTab(index) {
              tabsModel.currentIndex = index
              var tab = tabsModel.currentTab
@@ -1581,34 +1599,32 @@
      KeyboardShortcuts {
          id: shortcuts
--        // Ctrl+Tab: cycle through open tabs
++        // Ctrl+Tab or Ctrl+PageDown: cycle through open tabs
          KeyboardShortcut {
              modifiers: Qt.ControlModifier
              key: Qt.Key_Tab
              enabled: chrome.visible || recentView.visible
--            onTriggered: {
--                if (browser.wide) {
--                    internal.switchToTab((tabsModel.currentIndex + 1) % tabsModel.count)
--                } else {
--                    internal.switchToTab(tabsModel.count - 1)
--                }
--                if (recentView.visible) recentView.focus = true
--            }
++            onTriggered: internal.switchToNextTab()
++        }
++        KeyboardShortcut {
++            modifiers: Qt.ControlModifier
++            key: Qt.Key_PageDown
++            enabled: chrome.visible || recentView.visible
++            onTriggered: internal.switchToNextTab()
+         }
--        // Ctrl+Shift+Tab: cycle through open tabs in reverse order
++        // Ctrl+Shift+Tab or Ctrl+PageUp: cycle through open tabs in reverse order
          KeyboardShortcut {
              modifiers: Qt.ControlModifier
              key: Qt.Key_Backtab
              enabled: chrome.visible || recentView.visible
--            onTriggered: {
--                if (browser.wide) {
--                    internal.switchToTab((tabsModel.currentIndex - 1 + tabsModel.count) % tabsModel.count)
--                } else {
--                    internal.switchToTab(tabsModel.count - 1)
--                }
--                if (recentView.visible) recentView.focus = true
--            }
++            onTriggered: internal.switchToPreviousTab()
++        }
++        KeyboardShortcut {
++            modifiers: Qt.ControlModifier
++            key: Qt.Key_PageUp
++            enabled: chrome.visible || recentView.visible
++            onTriggered: internal.switchToPreviousTab()
+         }
          // Ctrl+W or Ctrl+F4: Close the current tab
 === modified file 'tests/autopilot/webbrowser_app/tests/test_keyboard.py'
 --- tests/autopilot/webbrowser_app/tests/test_keyboard.py	2015-08-14 09:25:38 +0000
 +++ tests/autopilot/webbrowser_app/tests/test_keyboard.py	2015-10-06 15:46:27 +0000
@@ -102,6 +102,12 @@
          self.check_tab_number(1)
          self.main_window.press_key('Ctrl+Tab')
          self.check_tab_number(2)
++        self.main_window.press_key('Ctrl+Page_Down')
++        self.check_tab_number(0)
++        self.main_window.press_key('Shift+Ctrl+Tab')
++        self.check_tab_number(2)
++        self.main_window.press_key('Ctrl+Page_Up')
++        self.check_tab_number(1)
      def test_can_switch_tabs_after_suggestions_escape(self):
          self.open_tabs(1)