Merge lp:~popey/ubuntu-terminal-app/add-control into lp:~ubuntu-terminal-dev/ubuntu-terminal-app/reboot

Status: Needs review
Proposed branch: lp:~popey/ubuntu-terminal-app/add-control
Merge into: lp:~ubuntu-terminal-dev/ubuntu-terminal-app/reboot
Diff against target: 306 lines (+68/-7)
14 files modified
AUTHORS (+1/-0)
po/com.ubuntu.terminal.pot (+4/-4)
src/app/qml/KeyboardBar.qml (+4/-0)
src/app/qml/KeyboardRows/KeyboardLayout.qml (+6/-0)
src/app/qml/KeyboardRows/KeyboardRow.qml (+1/-0)
src/app/qml/KeyboardRows/Layouts/ControlKeys.json (+7/-0)
src/app/qml/KeyboardRows/Layouts/FunctionKeys.json (+7/-0)
src/app/qml/KeyboardRows/jsonParser.js (+12/-2)
src/app/qml/TerminalPage.qml (+1/-0)
src/plugin/qmltermwidget/lib/Emulation.cpp (+7/-1)
src/plugin/qmltermwidget/lib/Emulation.h (+3/-0)
src/plugin/qmltermwidget/lib/Vt102Emulation.cpp (+8/-0)
src/plugin/qmltermwidget/src/ksession.cpp (+6/-0)
src/plugin/qmltermwidget/src/ksession.h (+1/-0)
To merge this branch: bzr merge lp:~popey/ubuntu-terminal-app/add-control
Reviewer Review Type Date Requested Status
Stefano Verzegnassi Needs Fixing
Niklas Wenzel (community) Needs Fixing
Evan McIntire Needs Fixing
Victor Thompson (community) Needs Information
Jenkins Bot continuous-integration Needs Fixing
Review via email: mp+282280@code.launchpad.net

Commit message

Adds a toggle-able general purpose control key to the functions keys overlay.

Description of the change

Adds a toggle-able general purpose control key to the functions keys overlay.

To post a comment you must log in.
147. By Alan Pope 🍺🐧🐱 πŸ¦„ on 2016-01-12

Add author of control key patch

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Continuous integration, rev:147
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~popey/ubuntu-terminal-app/add-control/+merge/282280/+edit-commit-message

https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/87/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/353/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/87/rebuild

review: Needs Fixing (continuous-integration)
148. By Alan Pope 🍺🐧🐱 πŸ¦„ on 2016-01-12

Add control key to more layouts

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Continuous integration, rev:148
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~popey/ubuntu-terminal-app/add-control/+merge/282280/+edit-commit-message

https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/88/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/354/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/88/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Evan McIntire (mcintire-evan) wrote :

I was asked in IRC to look over this, and it does all look good to me, and it does work as intended (as far as I can tell from my testing)

review: Approve
Revision history for this message
Victor Thompson (vthompson) wrote :

I'm not sure this "toggleable" key is as usable as it should be. So far as I can tell the other keys are more or less complete actions. Since just executing "CTRL" is a partial action, maybe it should stay highlighted until another key is selected?

Also, why wasn't this added to the control keys? Is there some use case I'm not aware of?

review: Needs Information
Revision history for this message
Evan McIntire (mcintire-evan) wrote :

Actually, changing my review based on what Victor said, there should be some sort of visible indicator that CTRL is toggled, it might be confusing otherwise

review: Needs Fixing
Revision history for this message
Niklas Wenzel (nikwen) wrote :

Thank you for looking into this, Alan! :)

I haven't looked at the code myself yet, but please change the commit message to something that you would want to see in the bazaar history. "Adding commit message as required by fussy jenkins bot" probably won't tell you two years down the road what you implemented in this MP. ;)

The text you put into the description looks fine for me for example. ;)

Revision history for this message
Alan Pope 🍺🐧🐱 πŸ¦„ (popey) wrote :

I agree that it should be in the CTRL bar too, and that it should be clear it's toggled on.

Revision history for this message
Niklas Wenzel (nikwen) wrote :

I just skimmed through the code and one thing I noticed is that we would probably want the CTRL key to be translatable, so please remove the "text" attributes from the keyboard layouts and use the "id" attribute instead. Furthermore, I'm not sure whether we would want to set the "modifier" attribute. That is used to turn the press of one exact button to be modifier + key and that probably isn't needed here.

(Just to be clear: I haven't reviewed the whole code yet.)

review: Needs Fixing
Revision history for this message
Alan Pope 🍺🐧🐱 πŸ¦„ (popey) wrote :

To be clear, this patch was originally created by @bartbes who asked me to manage the bzr mangling for him.

Neither of us is currently skilled enough in qml to implement the ctrl lock right now. Hints / tips / forks welcome :)

Revision history for this message
Bart van Strien (bartbes) wrote :

Let me preface this by saying I'm the original author of the patch:

I definitely agree it should be highlighted or something similar to indicate it's active. As the patch might show, this was my first foray into QML, and.. I simply wouldn't know how to even implement that.

As for the translation, once again, I blame my lack of experience. I did try, but I couldn't quite figure out how to get it to pass the validation code if there wasn't a text attribute.

Lastly, I'd like to draw your attention to Vt102Emulation.cpp, where at the moment it only supports adding the Control modifier. Perhaps it would make sense to expand this to alt and/or shift too?

Revision history for this message
Niklas Wenzel (nikwen) wrote :

Hi Bart,

In my opinion the patch looks way better than a lot of first-time QML patches. Congrats on that!
Regarding your questions: I guess we can give you a helping hand here. ;)

I'll try to answer the translation question right now.

Translations are handled by JsonTranslator.qml. It contains a getTranslatedNamyById() method which is already capable of translating the string "CTRL". You can find an example call to that method in line 84 of KeyboardLayout.qml. You only need to tell it that you are trying to translate a modifier (arg #1) and then pass the modifier string from the json file (arg #2) and it will provide you with the translated string.

To become more specific, I would suggest to modify the json file to contain the following code:

"main_action" : {
    "type": "modifier",
    "mod": "Control"
}

(The shorter form "mod" to keep it in line with the rest of the syntax if it doesn't break anything else.)

Then add another case to the createKeyText() method of KeyboardLayout.qml to enable translations of modifier keys and use that method in createNextModifierActionString() to determine the text of the action.

That should do the trick. ;)

By the way, while we're at it, would you mind adjusting the error message in line 36 of jsonParser.js to include the new modifier keyword? :)

If you have any further questions, feel free to ask. I agree that the highlighting thing is not easy and it will require some time for me to look into it as well. I'll try to do that when I have some spare time but maybe someone else is capable of doing that earlier.

Thanks again for the great patch!

Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

Niklas asked me to have a look at this review, since he'll be away for a few days.

> Let me preface this by saying I'm the original author
> of the patch:

I love this merge proposal, so thank you very much! :D

> I definitely agree it should be highlighted or something similar
> to indicate it's active. As the patch might show, this was my
> first foray into QML, and.. I simply wouldn't know how to even
> implement that.

I would never say this was your first iteration with QML. Code looks good, well done!.

Given the fact we necessarily have to deal with our keyboard bar, I'd say we need to add a Q_PROPERTY in KSession, which expose a Qt::KeyboardModifiers QFlags[1], instead of adding an "addNextModifiers" slot.
That property should have a getter, a setter and a signal.

I have to admit that I don't really like it as solution, but I can't see any other solid solution for providing a CTRL switch in the terminal-app GUI, unless we rewrite a new OSK keyboard - but we can't.

Anyway, at that point we should be able to check if the button needs to be highlighted or not. But then we would have another problem. :/

Currently "KeyboardButton.qml" requires to specify an Ubuntu.Components.Action (which is automatically generated by "KeyboardLayout.qml").
But UCAction has no useful property for binding the currently active keyboard modifier[2].

Fortunately that could be done by adding a property during the UCAction creation:
e.g. http://paste.ubuntu.com/15019832/

Then we should be finally able to update "KeyboardButton.qml" so that it can read the value of the property if it's defined.

I'd like to know what Niklas thinks of this.

> As for the translation, once again, I blame my lack of experience.
> I did try, but I couldn't quite figure out how to get it to pass
> the validation code if there wasn't a text attribute.

I guess Niklas already answered on this.

> Lastly, I'd like to draw your attention to Vt102Emulation.cpp,
> where at the moment it only supports adding the Control
> modifier. Perhaps it would make sense to expand this to alt
> and/or shift too?

I would say that we can add all the modifiers, since they all seem to be supported according to the comments in the file you mentioned.

PS: Sorry for the long comment. :)

======

[1] http://doc.qt.io/qt-4.8/qt.html#KeyboardModifier-enum
[2] QtQuick.Controls does it instead, see the "checked" property
    ref. http://doc.qt.io/qt-5/qml-qtquick-controls-action.html

review: Needs Fixing
Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

@Alan, any progress on this? Does it make sense to change the ownership of this branch, so that the team can finish the QML implementation?

Revision history for this message
Alan Pope 🍺🐧🐱 πŸ¦„ (popey) wrote :

Sorry, no. If you (or someone else) has some time to look at it and either use or re-work it, that would be great.

Revision history for this message
Niklas Wenzel (nikwen) wrote :

@Stefano: If that is what works, it is what works. ;)
I don't think there is an ideal solution here, but this is what I also came to my mind when I initially had a look at the issue. So I would support that.

Anyway, the reason why we haven't got any reply might be that Bart doesn't see when a new reply is posted. He hasn't subscribed to the branch and only people who do a non-"Comment only" review are subscribed automatically to an MP. It would be great if someone could drop him an email regarding this. :)

Unmerged revisions

148. By Alan Pope 🍺🐧🐱 πŸ¦„ on 2016-01-12

Add control key to more layouts

147. By Alan Pope 🍺🐧🐱 πŸ¦„ on 2016-01-12

Add author of control key patch

146. By Alan Pope 🍺🐧🐱 πŸ¦„ on 2016-01-12

Add toggleable control key

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'AUTHORS'
2--- AUTHORS 2015-07-06 16:02:18 +0000
3+++ AUTHORS 2016-01-12 10:43:25 +0000
4@@ -10,3 +10,4 @@
5 Michael Terry <michael.terry@canonical.com>
6 Niklas Wenzel <nikwen.developer@gmail.com>
7 Timo Jyrinki <timo.jyrinki@canonical.com>
8+Bart van Strien <bart.bes@gmail.com>
9
10=== modified file 'po/com.ubuntu.terminal.pot'
11--- po/com.ubuntu.terminal.pot 2015-08-09 14:03:25 +0000
12+++ po/com.ubuntu.terminal.pot 2016-01-12 10:43:25 +0000
13@@ -8,7 +8,7 @@
14 msgstr ""
15 "Project-Id-Version: \n"
16 "Report-Msgid-Bugs-To: \n"
17-"POT-Creation-Date: 2015-08-09 16:01+0200\n"
18+"POT-Creation-Date: 2016-01-10 19:21+0100\n"
19 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
20 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
21 "Language-Team: LANGUAGE <LL@li.org>\n"
22@@ -53,7 +53,7 @@
23 msgid "Authentication failed"
24 msgstr ""
25
26-#: ../src/app/qml/KeyboardBar.qml:174
27+#: ../src/app/qml/KeyboardBar.qml:178
28 msgid "Change Keyboard"
29 msgstr ""
30
31@@ -177,7 +177,7 @@
32 msgid "New tab"
33 msgstr ""
34
35-#: ../src/app/qml/TerminalPage.qml:164
36+#: ../src/app/qml/TerminalPage.qml:165
37 msgid "Selection Mode"
38 msgstr ""
39
40@@ -214,7 +214,7 @@
41 msgstr ""
42
43 #: ../src/plugin/konsole/Vt102Emulation.cpp:961
44-#: ../src/plugin/qmltermwidget/lib/Vt102Emulation.cpp:977
45+#: ../src/plugin/qmltermwidget/lib/Vt102Emulation.cpp:985
46 msgid ""
47 "No keyboard translator available. The information needed to convert key "
48 "presses into characters to send to the terminal is missing."
49
50=== modified file 'src/app/qml/KeyboardBar.qml'
51--- src/app/qml/KeyboardBar.qml 2015-07-13 20:25:21 +0000
52+++ src/app/qml/KeyboardBar.qml 2016-01-12 10:43:25 +0000
53@@ -12,6 +12,7 @@
54
55 signal simulateCommand(string command);
56 signal simulateKey(int key, int mod);
57+ signal simulateModifier(int modifiers);
58
59 ListModel {
60 id: layoutsList
61@@ -56,6 +57,7 @@
62 layoutObject.z = rootItem.z;
63 layoutObject.simulateKey.disconnect(simulateKey);
64 layoutObject.simulateCommand.disconnect(simulateCommand);
65+ layoutObject.simulateModifier.disconnect(simulateModifier);
66 }
67
68 function enableLayout(index) {
69@@ -68,6 +70,7 @@
70 layoutObject.z = rootItem.z + 0.01;
71 layoutObject.simulateKey.connect(simulateKey);
72 layoutObject.simulateCommand.connect(simulateCommand);
73+ layoutObject.simulateModifier.connect(simulateModifier);
74 }
75
76 function isIndexLayoutValid(index) {
77@@ -161,6 +164,7 @@
78
79 onSimulateKey: pressFeedbackEffect.start();
80 onSimulateCommand: pressFeedbackEffect.start();
81+ onSimulateModifier: pressFeedbackEffect.start();
82
83 Item {
84 id: keyboardContainer
85
86=== modified file 'src/app/qml/KeyboardRows/KeyboardLayout.qml'
87--- src/app/qml/KeyboardRows/KeyboardLayout.qml 2015-08-09 14:25:36 +0000
88+++ src/app/qml/KeyboardRows/KeyboardLayout.qml 2016-01-12 10:43:25 +0000
89@@ -28,6 +28,8 @@
90 return createKeyActionString(action.key, action.mod, action.text, action.id);
91 case "string":
92 return createStringActionString(action.string, action.text);
93+ case "modifier":
94+ return createNextModifierActionString(action.modifier);
95 }
96 }
97
98@@ -56,6 +58,10 @@
99 return "Action { " + textString + " onTriggered: simulateCommand(\"" + string + "\"); }";
100 }
101
102+ function createNextModifierActionString(modifier) {
103+ return "Action { text: \"" + modifier + "\"; onTriggered: simulateModifier(Qt." + modifier + "Modifier); }";
104+ }
105+
106 function createEntryString(text, actionString, otherActionsString) {
107 var objectString = "
108 import QtQuick 2.4
109
110=== modified file 'src/app/qml/KeyboardRows/KeyboardRow.qml'
111--- src/app/qml/KeyboardRows/KeyboardRow.qml 2015-07-13 20:25:21 +0000
112+++ src/app/qml/KeyboardRows/KeyboardRow.qml 2016-01-12 10:43:25 +0000
113@@ -14,6 +14,7 @@
114 // External signals.
115 signal simulateKey(int key, int mod);
116 signal simulateCommand(string command);
117+ signal simulateModifier(int modifiers);
118
119 // Internal variables
120 property int _firstVisibleIndex: gridView.contentX / keyWidth
121
122=== modified file 'src/app/qml/KeyboardRows/Layouts/ControlKeys.json'
123--- src/app/qml/KeyboardRows/Layouts/ControlKeys.json 2015-07-10 14:34:00 +0000
124+++ src/app/qml/KeyboardRows/Layouts/ControlKeys.json 2016-01-12 10:43:25 +0000
125@@ -4,6 +4,13 @@
126 "buttons": [
127 {
128 "main_action" : {
129+ "type": "modifier",
130+ "modifier": "Control",
131+ "text": "CTRL"
132+ }
133+ },
134+ {
135+ "main_action" : {
136 "type": "key",
137 "key" : "R",
138 "mod" : "Control"
139
140=== modified file 'src/app/qml/KeyboardRows/Layouts/FunctionKeys.json'
141--- src/app/qml/KeyboardRows/Layouts/FunctionKeys.json 2015-07-10 14:34:00 +0000
142+++ src/app/qml/KeyboardRows/Layouts/FunctionKeys.json 2016-01-12 10:43:25 +0000
143@@ -11,6 +11,13 @@
144 },
145 {
146 "main_action" : {
147+ "type": "modifier",
148+ "modifier": "Control",
149+ "text": "CTRL"
150+ }
151+ },
152+ {
153+ "main_action" : {
154 "type": "key",
155 "key" : "F1"
156 }
157
158=== modified file 'src/app/qml/KeyboardRows/jsonParser.js'
159--- src/app/qml/KeyboardRows/jsonParser.js 2015-07-13 17:36:57 +0000
160+++ src/app/qml/KeyboardRows/jsonParser.js 2016-01-12 10:43:25 +0000
161@@ -22,12 +22,19 @@
162 raiseException("string is missing in", stringObject);
163 }
164
165+function validateModifierAction(modifierObject) {
166+ if (!modifierObject.modifier)
167+ return raiseException("modifier is missing");
168+ if (!isAllowed(modifierObject.modifier, ["Control", "Shift", "Alt"]))
169+ return raiseException("modifier is invalid in", modifierObject);
170+}
171+
172 function validateAction(actionObject) {
173 if (!actionObject.type)
174 raiseException("type is missing in", actionObject);
175- if (!isAllowed(actionObject.type, ["key", "string"]))
176+ if (!isAllowed(actionObject.type, ["key", "string", "modifier"]))
177 raiseException("type must be either key or string in", actionObject);
178- if (!(actionObject.type === "key" ? actionObject.key : actionObject.string))
179+ if (!actionObject[actionObject.type])
180 raiseException(actionObject.type + " is missing in", actionObject);
181 if (actionObject.id && actionObject.text)
182 raiseException("Should not define id and text together in", actionObject);
183@@ -45,6 +52,9 @@
184 case "string":
185 validateStringAction(actionObject);
186 break;
187+ case "modifier":
188+ validateModifierAction(actionObject);
189+ break;
190 }
191 }
192
193
194=== modified file 'src/app/qml/TerminalPage.qml'
195--- src/app/qml/TerminalPage.qml 2015-07-13 20:25:21 +0000
196+++ src/app/qml/TerminalPage.qml 2016-01-12 10:43:25 +0000
197@@ -140,6 +140,7 @@
198 height: units.gu(5)
199 onSimulateKey: terminal.simulateKeyPress(key, mod, true, 0, "");
200 onSimulateCommand: terminal.session.sendText(command);
201+ onSimulateModifier: terminal.session.addNextModifiers(modifiers);
202 }
203 }
204
205
206=== modified file 'src/plugin/qmltermwidget/lib/Emulation.cpp'
207--- src/plugin/qmltermwidget/lib/Emulation.cpp 2014-11-12 00:10:12 +0000
208+++ src/plugin/qmltermwidget/lib/Emulation.cpp 2016-01-12 10:43:25 +0000
209@@ -55,7 +55,8 @@
210 _codec(0),
211 _decoder(0),
212 _keyTranslator(0),
213- _usesMouse(false)
214+ _usesMouse(false),
215+ _nextModifiers(0)
216 {
217 // create screens with a default size
218 _screen[0] = new Screen(40,80);
219@@ -359,6 +360,11 @@
220 return QSize(_currentScreen->getColumns(), _currentScreen->getLines());
221 }
222
223+void Emulation::addNextModifiers(int modifiers)
224+{
225+ _nextModifiers ^= modifiers;
226+}
227+
228 ushort ExtendedCharTable::extendedCharHash(ushort* unicodePoints , ushort length) const
229 {
230 ushort hash = 0;
231
232=== modified file 'src/plugin/qmltermwidget/lib/Emulation.h'
233--- src/plugin/qmltermwidget/lib/Emulation.h 2014-11-12 00:10:12 +0000
234+++ src/plugin/qmltermwidget/lib/Emulation.h 2016-01-12 10:43:25 +0000
235@@ -218,6 +218,8 @@
236 */
237 bool programUsesMouse() const;
238
239+ void addNextModifiers(int modifiers);
240+
241 public slots:
242
243 /** Change the size of the emulation's image */
244@@ -442,6 +444,7 @@
245 const QTextCodec* _codec;
246 QTextDecoder* _decoder;
247 const KeyboardTranslator* _keyTranslator; // the keyboard layout
248+ int _nextModifiers;
249
250 protected slots:
251 /**
252
253=== modified file 'src/plugin/qmltermwidget/lib/Vt102Emulation.cpp'
254--- src/plugin/qmltermwidget/lib/Vt102Emulation.cpp 2014-11-12 00:10:12 +0000
255+++ src/plugin/qmltermwidget/lib/Vt102Emulation.cpp 2016-01-12 10:43:25 +0000
256@@ -968,6 +968,14 @@
257 textToSend += _codec->fromUnicode(event->text());
258 }
259
260+ if (textToSend.length() == 1 && _nextModifiers)
261+ {
262+ char first = textToSend[0];
263+ if ((_nextModifiers & Qt::ControlModifier) && first >= 0x4f && first < 0x7f)
264+ textToSend[0] = first & 0x1f;
265+ _nextModifiers = 0;
266+ }
267+
268 sendData( textToSend.constData() , textToSend.length() );
269 }
270 else
271
272=== modified file 'src/plugin/qmltermwidget/src/ksession.cpp'
273--- src/plugin/qmltermwidget/src/ksession.cpp 2014-11-12 00:10:12 +0000
274+++ src/plugin/qmltermwidget/src/ksession.cpp 2016-01-12 10:43:25 +0000
275@@ -22,6 +22,7 @@
276
277 // Own
278 #include "ksession.h"
279+#include "Emulation.h"
280
281 // Qt
282 #include <QTextCodec>
283@@ -211,6 +212,11 @@
284 // }
285 }
286
287+void KSession::addNextModifiers(int modifiers)
288+{
289+ m_session->emulation()->addNextModifiers(modifiers);
290+}
291+
292 void KSession::setFlowControlEnabled(bool enabled)
293 {
294 m_session->setFlowControlEnabled(enabled);
295
296=== modified file 'src/plugin/qmltermwidget/src/ksession.h'
297--- src/plugin/qmltermwidget/src/ksession.h 2014-11-12 00:10:12 +0000
298+++ src/plugin/qmltermwidget/src/ksession.h 2016-01-12 10:43:25 +0000
299@@ -122,6 +122,7 @@
300 void sendText(QString text);
301 // Send some text to terminal
302 void sendKey(int rep, int key, int mod) const;
303+ void addNextModifiers(int modifiers);
304
305
306 protected slots:

Subscribers

People subscribed via source and target branches