Merge lp:~unity-2d-team/unity-2d/unity-2d_enable-super-numkey-combo into lp:unity-2d

Proposed by Albert Astals Cid
Status: Rejected
Rejected by: Gerry Boland
Proposed branch: lp:~unity-2d-team/unity-2d/unity-2d_enable-super-numkey-combo
Merge into: lp:unity-2d
Diff against target: 292 lines (+148/-35)
6 files modified
launcher/app/launcherview.cpp (+34/-13)
libunity-2d-private/src/hotkey.cpp (+24/-18)
libunity-2d-private/src/hotkey.h (+5/-1)
libunity-2d-private/src/hotkeymonitor.cpp (+2/-2)
libunity-2d-private/src/hotkeymonitor.h (+1/-1)
tests/launcher/enable-super-numkey.rb (+82/-0)
To merge this branch: bzr merge lp:~unity-2d-team/unity-2d/unity-2d_enable-super-numkey-combo
Reviewer Review Type Date Requested Status
Gerry Boland (community) Disapprove
Paweł Stołowski Needs Fixing
Albert Astals Cid (community) Needs Fixing
Michał Sawicz Pending
Review via email: mp+91436@code.launchpad.net

Description of the change

[launcher] Enabled Super+NumKey[0..9] shortcuts for launcher tiles.

We need separate hotkey's for them.

To post a comment you must log in.
Revision history for this message
Albert Astals Cid (aacid) wrote :

This is the heir of https://code.launchpad.net/~unity-2d-team/unity-2d/enable-super-numkey-combo/+merge/89883 that was rejected because was against unity-2d-shell instead of unity-2d

Copying here Michał's comments of what needs to be fixed.

The test is broken:
* if the launcher item is already in "running" state, the test won't really test anything
* if the app in question doesn't start within TIMEOUT, the test will fail even though the feature actually worked
* LibreOffice Impress, for example, doesn't go into "running" state before you actually create a slidedeck, so the test will fail in a clean environment every time
* the result of this test is a workspace cluttered with up to 10 new windows that aren't closed

review: Needs Fixing
Revision history for this message
Paweł Stołowski (stolowski) wrote :

1. I think that the logic based on isX11Keysym in LauncherView::forwardNumericHotkey(), as well as Hotkey constructor are good candidates for introducing two simple classes inherited from HotKey: NumericHotkey and NumpadNumHotKey that hide x11keysm-based logic. They would inherit from Hotkey and from a common abstract class that provides index() interface, hinding index = key - .... calculation/comparison logic. I don't insist, but I prefer to avoid leaking of logic outside of classes and use polymorphic classes instead.

2. I think HotkeyMonitor::getHotkeyFor(Qt::Key ...) should check for equality of isX11keysym flag, and not only key and modifiers values.

review: Needs Fixing
Revision history for this message
Gerry Boland (gerboland) wrote :
review: Disapprove

Unmerged revisions

881. By Albert Astals Cid on 2012-02-03

forgot the test :D

880. By Albert Astals Cid on 2012-02-01

Backport r947 from unity-2d-shell

message:
  [shell][launcher] Enabled Super+NumKey[0..9] shortcuts for launcher tiles.
  We need separate hotkey's for them.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/app/launcherview.cpp'
2--- launcher/app/launcherview.cpp 2011-11-25 11:13:44 +0000
3+++ launcher/app/launcherview.cpp 2012-02-03 14:12:22 +0000
4@@ -58,6 +58,10 @@
5 static const char* SPREAD_DBUS_METHOD_IS_SHOWN = "IsShown";
6 static const char* COMMANDS_LENS_ID = "commands.lens";
7
8+static const int NumPad_Key_0 = 0xffb0;
9+static const int NumPad_Key_1 = 0xffb1;
10+static const int NumPad_Key_9 = 0xffb9;
11+
12 LauncherView::LauncherView(QWidget* parent) :
13 Unity2DDeclarativeView(parent),
14 m_superKeyPressed(false), m_superKeyHeld(false)
15@@ -91,6 +95,17 @@
16 hotkey = HotkeyMonitor::instance().getHotkeyFor(key, Qt::MetaModifier | Qt::ShiftModifier);
17 connect(hotkey, SIGNAL(pressed()), SLOT(forwardNumericHotkey()));
18 }
19+
20+ /* Super+NUMKEY{n} for 0 ≤ n ≤ 9 activates the item with index (n + 9) % 10.*/
21+ /* Qt won't distinguish between keys[0-9] below the Functionkeys and keypad keys[0-9] on the Keypad.
22+ Reference: <X11/keysymdef.h>
23+ */
24+ for (Qt::Key key = (Qt::Key)(NumPad_Key_0); key <= (Qt::Key)(NumPad_Key_9); key = (Qt::Key) (key + 1)) {
25+ Hotkey* hotkey = HotkeyMonitor::instance().getHotkeyFor((Qt::Key)(key), Qt::MetaModifier, true);
26+ connect(hotkey, SIGNAL(pressed()), SLOT(forwardNumericHotkey()));
27+ hotkey = HotkeyMonitor::instance().getHotkeyFor((Qt::Key)(key), Qt::MetaModifier | Qt::ShiftModifier, true);
28+ connect(hotkey, SIGNAL(pressed()), SLOT(forwardNumericHotkey()));
29+ }
30 }
31
32 LauncherView::~LauncherView()
33@@ -207,19 +222,25 @@
34 In other words, the indexes are activated in the same order as
35 the keys appear on a standard keyboard. */
36 Qt::Key key = hotkey->key();
37- if (key >= Qt::Key_1 && key <= Qt::Key_9) {
38- int index = key - Qt::Key_0;
39- if (hotkey->modifiers() & Qt::ShiftModifier) {
40- Q_EMIT newInstanceShortcutPressed(index);
41- } else {
42- Q_EMIT activateShortcutPressed(index);
43- }
44- } else if (key == Qt::Key_0) {
45- if (hotkey->modifiers() & Qt::ShiftModifier) {
46- Q_EMIT newInstanceShortcutPressed(10);
47- } else {
48- Q_EMIT activateShortcutPressed(10);
49- }
50+ int index = 0;
51+ if (hotkey->isX11Keysym()) {
52+ if (key >= NumPad_Key_1 && key <= NumPad_Key_9) {
53+ index = key - NumPad_Key_0;
54+ } else if (key == NumPad_Key_0) {
55+ index = 10;
56+ }
57+ } else {
58+ if (key >= Qt::Key_1 && key <= Qt::Key_9) {
59+ index = key - Qt::Key_0;
60+ } else if (key == Qt::Key_0) {
61+ index = 10;
62+ }
63+ }
64+
65+ if (hotkey->modifiers() & Qt::ShiftModifier) {
66+ Q_EMIT newInstanceShortcutPressed(index);
67+ } else {
68+ Q_EMIT activateShortcutPressed(index);
69 }
70 }
71 }
72
73=== modified file 'libunity-2d-private/src/hotkey.cpp'
74--- libunity-2d-private/src/hotkey.cpp 2011-05-03 15:28:49 +0000
75+++ libunity-2d-private/src/hotkey.cpp 2012-02-03 14:12:22 +0000
76@@ -45,9 +45,9 @@
77 return 0;
78 }
79
80-Hotkey::Hotkey(Qt::Key key, Qt::KeyboardModifiers modifiers, QObject *parent) :
81+Hotkey::Hotkey(Qt::Key key, Qt::KeyboardModifiers modifiers, QObject *parent, bool isX11keysym) :
82 QObject(parent), m_connections(0),
83- m_key(key), m_modifiers(modifiers),
84+ m_key(key), m_modifiers(modifiers), m_isX11keysym(isX11keysym),
85 m_x11key(0), m_x11modifiers(0)
86 {
87 /* Translate the QT modifiers to X11 modifiers */
88@@ -65,23 +65,29 @@
89 m_x11modifiers |= Mod4Mask;
90 }
91
92- /* Translate the QT key to X11 keycode */
93-
94- /* QKeySequence can be used to translate a Qt::Key in a format that is
95- understood by XStringToKeysym if the sequence is composed only by the key */
96- QString keyString = QKeySequence(key).toString();
97- KeySym keysym = XStringToKeysym(keyString.toLatin1().data());
98- if (keysym == NoSymbol) {
99- /* XStringToKeysym doesn’t work well with exotic characters (such as
100- 'É'). Note that this fallback code path looks much simpler but doesn’t
101- work for special keys such as the function keys (e.g. F1), which is
102- why the translation with XStringToKeysym is attempted first. */
103- keysym = (ushort) key;
104+ if (isX11keysym) {
105+ m_x11key = XKeysymToKeycode(QX11Info::display(), key);
106 }
107- m_x11key = XKeysymToKeycode(QX11Info::display(), keysym);
108- if (m_x11key == 0) {
109- UQ_WARNING << "Could not get keycode for keysym" << keysym
110- << "(" << keyString << ")";
111+ else {
112+ /* Translate the QT key to X11 keycode */
113+
114+ /* QKeySequence can be used to translate a Qt::Key in a format that is
115+ understood by XStringToKeysym if the sequence is composed only by the key */
116+ QString keyString = QKeySequence(key).toString();
117+ KeySym keysym = XStringToKeysym(keyString.toLatin1().data());
118+
119+ if (keysym == NoSymbol) {
120+ /* XStringToKeysym doesn’t work well with exotic characters (such as
121+ 'É'). Note that this fallback code path looks much simpler but doesn’t
122+ work for special keys such as the function keys (e.g. F1), which is
123+ why the translation with XStringToKeysym is attempted first. */
124+ keysym = (ushort) key;
125+ }
126+ m_x11key = XKeysymToKeycode(QX11Info::display(), keysym);
127+ if (m_x11key == 0) {
128+ UQ_WARNING << "Could not get keycode for keysym" << keysym
129+ << "(" << keyString << ")";
130+ }
131 }
132 }
133
134
135=== modified file 'libunity-2d-private/src/hotkey.h'
136--- libunity-2d-private/src/hotkey.h 2011-02-23 17:03:50 +0000
137+++ libunity-2d-private/src/hotkey.h 2012-02-03 14:12:22 +0000
138@@ -22,6 +22,8 @@
139
140 #include <QObject>
141
142+typedef unsigned long KeySym;
143+
144 class Hotkey : public QObject
145 {
146 friend class HotkeyMonitor;
147@@ -33,6 +35,7 @@
148 public:
149 Qt::Key key() const { return m_key; }
150 Qt::KeyboardModifiers modifiers() const { return m_modifiers; }
151+ bool isX11Keysym() const { return m_isX11keysym; }
152
153 Q_SIGNALS:
154 void keyChanged(Qt::Key key);
155@@ -45,13 +48,14 @@
156 virtual void disconnectNotify(const char * signal);
157
158 private:
159- Hotkey(Qt::Key key, Qt::KeyboardModifiers modifiers, QObject *parent);
160+ Hotkey(Qt::Key key, Qt::KeyboardModifiers modifiers, QObject *parent, bool isX11keysym = false);
161 bool processNativeEvent(uint x11Keycode, uint x11Modifiers, bool isPressEvent);
162
163 private:
164 uint m_connections;
165 Qt::Key m_key;
166 Qt::KeyboardModifiers m_modifiers;
167+ bool m_isX11keysym;
168 uint m_x11key;
169 uint m_x11modifiers;
170 };
171
172=== modified file 'libunity-2d-private/src/hotkeymonitor.cpp'
173--- libunity-2d-private/src/hotkeymonitor.cpp 2011-10-14 07:07:09 +0000
174+++ libunity-2d-private/src/hotkeymonitor.cpp 2012-02-03 14:12:22 +0000
175@@ -64,7 +64,7 @@
176
177
178 Hotkey*
179-HotkeyMonitor::getHotkeyFor(Qt::Key key, Qt::KeyboardModifiers modifiers)
180+HotkeyMonitor::getHotkeyFor(Qt::Key key, Qt::KeyboardModifiers modifiers, bool isX11keysym)
181 {
182 Q_FOREACH(Hotkey* currentHotkey, m_hotkeys) {
183 if (currentHotkey->key() == key &&
184@@ -73,7 +73,7 @@
185 }
186 }
187
188- Hotkey *hotkey = new Hotkey(key, modifiers, this);
189+ Hotkey *hotkey = new Hotkey(key, modifiers, this, isX11keysym);
190 m_hotkeys.append(hotkey);
191 return hotkey;
192 }
193
194=== modified file 'libunity-2d-private/src/hotkeymonitor.h'
195--- libunity-2d-private/src/hotkeymonitor.h 2011-10-14 07:07:09 +0000
196+++ libunity-2d-private/src/hotkeymonitor.h 2012-02-03 14:12:22 +0000
197@@ -33,7 +33,7 @@
198 static HotkeyMonitor& instance();
199 ~HotkeyMonitor();
200
201- Hotkey* getHotkeyFor(Qt::Key key, Qt::KeyboardModifiers modifiers);
202+ Hotkey* getHotkeyFor(Qt::Key key, Qt::KeyboardModifiers modifiers, bool isX11keysym = false);
203
204 void disableModifiers(Qt::KeyboardModifiers modifiers);
205 void enableModifiers(Qt::KeyboardModifiers modifiers);
206
207=== added file 'tests/launcher/enable-super-numkey.rb'
208--- tests/launcher/enable-super-numkey.rb 1970-01-01 00:00:00 +0000
209+++ tests/launcher/enable-super-numkey.rb 2012-02-03 14:12:22 +0000
210@@ -0,0 +1,82 @@
211+#!/usr/bin/env ruby1.8
212+=begin
213+/*
214+ * This file is part of unity-2d
215+ *
216+ * Copyright 2011 Canonical Ltd.
217+ *
218+ * Authors:
219+ * - Gerry Boland <gerry.boland@canonical.com>
220+ *
221+ * This program is free software; you can redistribute it and/or modify
222+ * it under the terms of the GNU General Public License as published by
223+ * the Free Software Foundation; version 3.
224+ *
225+ * This program is distributed in the hope that it will be useful,
226+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
227+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
228+ * GNU General Public License for more details.
229+ *
230+ * You should have received a copy of the GNU General Public License
231+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
232+ */
233+=end
234+
235+require '../run-tests.rb' unless $INIT_COMPLETED
236+require 'xdo/keyboard'
237+
238+############################# Test Suite #############################
239+context "Launcher Super+NUM key support Tests" do
240+ # Run once at the beginning of this test suite
241+ startup do
242+ $SUT.execute_shell_command 'killall unity-2d-launcher'
243+ $SUT.execute_shell_command 'killall unity-2d-launcher'
244+ end
245+
246+ # Run once at the end of this test suite
247+ shutdown do
248+ end
249+
250+ # Run before each test case begins
251+ setup do
252+ # Execute the application
253+ @app = $SUT.run( :name => UNITY_2D_LAUNCHER,
254+ :arguments => "-testability",
255+ :sleeptime => 2 )
256+ end
257+
258+ # Run after each test case completes
259+ teardown do
260+ $SUT.execute_shell_command "pkill -nf unity-2d-launcher"
261+ end
262+
263+ #####################################################################################
264+ # Test cases
265+ # Test case objectives:
266+ # * To verify Launcher tile shortcuts Super+NumKey[0,9] are enabled
267+ # Pre-conditions
268+ # * None
269+ # Test steps
270+ # * List the numeric shortcuts from LauncherList
271+ # * Simluate the Super+Numkey[0,9].
272+ # * Verify that corresponding application is running.
273+ # Post-conditions
274+ # * None
275+ # References
276+ # * lp: 750514
277+ test "Launcher enable Super+Numkey for tile shortcuts" do
278+ tile_list = @app.Unity2dPanel().LauncherList(:name =>'main').children ({ :shortcutText => /\d/ })
279+ tile_list.delete_if {|x| tile_list.index(x) >= 10 } #Remove duplicates if any.
280+ tile_list.each do |tile|
281+ shortcut = tile["shortcutText"]
282+ numkey="{NUM"+shortcut+"}"
283+ XDo::Keyboard.key_down('SUPER')
284+ XDo::Keyboard.simulate(numkey)
285+ XDo::Keyboard.key_up('SUPER')
286+
287+ verify_true(TIMEOUT, numkey+"-Failed") {
288+ tile['running'] == 'true'
289+ }
290+ end
291+ end
292+end

Subscribers

People subscribed via source and target branches