After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 652223 - Keyboard shortcut for tab switch in LookingGlass
Keyboard shortcut for tab switch in LookingGlass
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.0.x
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-06-09 17:52 UTC by Alexandre Franke
Modified: 2011-10-21 13:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add-Ctrl-PageUp-PageDown-key-shortcuts-for-switching.patch (1.72 KB, patch)
2011-10-08 07:07 UTC, Jason Siefken
needs-work Details | Review
Add Ctrl+PageUp/PageDown key shortcuts for switching tabs (2.01 KB, patch)
2011-10-21 03:51 UTC, Jason Siefken
reviewed Details | Review
Add Ctrl+PageUp/PageDown key shortcuts for switching tabs (2.06 KB, patch)
2011-10-21 05:24 UTC, Jason Siefken
committed Details | Review

Description Alexandre Franke 2011-06-09 17:52:12 UTC
It would be very handy to have a keyboard shortcut (Ctrl+Page{Up,Down} seems the most logical to me) to switch between the Evaluator, Windows, Errors and Extensions tabs.
Comment 1 Dan Winship 2011-06-09 18:12:08 UTC
(In reply to comment #0)
> (Ctrl+Page{Up,Down} seems the most logical to me)

yes, that matches the overview
Comment 2 Jason Siefken 2011-10-08 07:07:43 UTC
Created attachment 198590 [details] [review]
Add-Ctrl-PageUp-PageDown-key-shortcuts-for-switching.patch

Adds Ctrl+PageUp/PageDown keyboard shortcuts for switching tabs in lookingGlass's notebook view.
Comment 3 Jasper St. Pierre (not reading bugmail) 2011-10-08 07:17:07 UTC
Review of attachment 198590 [details] [review]:

::: js/ui/lookingGlass.js
@@ +973,3 @@
+            switch (symbol) {
+                case Clutter.KEY_Page_Up:
+                    nextIndex = this._notebook._selectedIndex < this._notebook._tabs.length - 1 ? this._notebook._selectedIndex + 1 : this._notebook._selectedIndex;

You shouldn't access private members like this. I'd rather something like:

if (modifierState & Clutter.ModifierType.CONTROL_MASK) {
    if (symbol == Clutter.KEY_Page_Up)
        this._notebook.nextTab();
    else if (symbol == Clutter.KEY_Page_Down)
        this._notebook.prevTab();
}

and then implement _notebook.prevTab/nextTab in the proper way.
Comment 4 Jason Siefken 2011-10-21 03:51:10 UTC
Created attachment 199607 [details] [review]
Add Ctrl+PageUp/PageDown key shortcuts for switching tabs
Comment 5 Jasper St. Pierre (not reading bugmail) 2011-10-21 04:53:48 UTC
Review of attachment 199607 [details] [review]:

::: js/ui/lookingGlass.js
@@ +154,3 @@
+
+    nextTab: function() {
+            let nextIndex = this._selectedIndex < this._tabs.length - 1 ? this._selectedIndex + 1 : this._selectedIndex;

Four-space indents. Additionally, I'd do something like:

    let nextIndex = this._selectedIndex;
    if (nextIndex < this._tabs.length - 1)
        nextIndex ++;

@@ +159,3 @@
+
+    prevTab: function() {
+            let nextIndex = this._selectedIndex > 0 ? this._selectedIndex - 1 : 0;

let prevIndex = this._selectedIndex;
    if (prevIndex > 0)
        prevIndex --;

@@ +981,3 @@
+        if (modifierState & Clutter.ModifierType.CONTROL_MASK) {
+            if (symbol == Clutter.KEY_Page_Up) {
+                this._notebook.nextTab();

This is opposite of how gnome-shell does it in the view selector (sorry, I got it backwards).
Comment 6 Jason Siefken 2011-10-21 05:24:12 UTC
Created attachment 199611 [details] [review]
Add Ctrl+PageUp/PageDown key shortcuts for switching tabs
Comment 7 Jasper St. Pierre (not reading bugmail) 2011-10-21 05:28:50 UTC
Review of attachment 199611 [details] [review]:

Looks fine.

::: js/ui/lookingGlass.js
@@ +156,3 @@
+        let nextIndex = this._selectedIndex;
+        if (nextIndex < this._tabs.length - 1) {
+            ++nextIndex;

We tend to prefer postincrement, but it's not super important.
Comment 8 Jasper St. Pierre (not reading bugmail) 2011-10-21 05:30:06 UTC
Review of attachment 199611 [details] [review]:

Oh, before I forget, this is going to need a better commit message:

    lookingGlass: add Ctrl+PageUp/PageDown key shortcuts for switching tabs

    The view selector in the overview does it too, so why not here?
Comment 9 Jasper St. Pierre (not reading bugmail) 2011-10-21 13:22:45 UTC
Attachment 199611 [details] pushed as a69ebc8 - Add Ctrl+PageUp/PageDown key shortcuts for switching tabs


Thanks for submitting the patch!