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

[Alan Pope 🍺🐧🐱 🦄]

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

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

[lp:~popey/ubuntu-terminal-app/add-control]

147. By Alan Pope 🍺🐧🐱 🦄 on 2016-01-12

Add author of control key patch

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

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)

[lp:~popey/ubuntu-terminal-app/add-control]

148. By Alan Pope 🍺🐧🐱 🦄 on 2016-01-12

Add control key to more layouts

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

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)

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

FAILED: Continuous integration, rev:148
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/118/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/392/console

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

review: Needs Fixing (continuous-integration)

[Evan McIntire (mcintire-evan)]

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

[Victor Thompson (vthompson)]

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

[Evan McIntire (mcintire-evan)]

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

[Niklas Wenzel (nikwen)]

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. ;)

[Alan Pope 🍺🐧🐱 🦄 (popey)]

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

[Niklas Wenzel (nikwen)]

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

[Alan Pope 🍺🐧🐱 🦄 (popey)]

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 :)

[Bart van Strien (bartbes)]

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?

[Niklas Wenzel (nikwen)]

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!

[Stefano Verzegnassi (verzegnassi-stefano)]

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

[Stefano Verzegnassi (verzegnassi-stefano)]

@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?

[Alan Pope 🍺🐧🐱 🦄 (popey)]

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.

[Niklas Wenzel (nikwen)]

@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

=== modified file 'AUTHORS'
--- AUTHORS	2015-07-06 16:02:18 +0000
+++ AUTHORS	2016-01-12 10:43:25 +0000
@@ -10,3 +10,4 @@
 Michael Terry <[e-mail]>
 Niklas Wenzel <[e-mail]>
 Timo Jyrinki <[e-mail]>
+Bart van Strien <[e-mail]>
 
=== modified file 'po/com.ubuntu.terminal.pot'
--- po/com.ubuntu.terminal.pot	2015-08-09 14:03:25 +0000
+++ po/com.ubuntu.terminal.pot	2016-01-12 10:43:25 +0000
@@ -8,7 +8,7 @@
 msgstr ""
 "Project-Id-Version: \n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2015-08-09 16:01+0200\n"
+"POT-Creation-Date: 2016-01-10 19:21+0100\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <[e-mail]>\n"
@@ -53,7 +53,7 @@
 msgid "Authentication failed"
 msgstr ""
 
-#: ../src/app/qml/KeyboardBar.qml:174
+#: ../src/app/qml/KeyboardBar.qml:178
 msgid "Change Keyboard"
 msgstr ""
 
@@ -177,7 +177,7 @@
 msgid "New tab"
 msgstr ""
 
-#: ../src/app/qml/TerminalPage.qml:164
+#: ../src/app/qml/TerminalPage.qml:165
 msgid "Selection Mode"
 msgstr ""
 
@@ -214,7 +214,7 @@
 msgstr ""
 
 #: ../src/plugin/konsole/Vt102Emulation.cpp:961
-#: ../src/plugin/qmltermwidget/lib/Vt102Emulation.cpp:977
+#: ../src/plugin/qmltermwidget/lib/Vt102Emulation.cpp:985
 msgid ""
 "No keyboard translator available.  The information needed to convert key "
 "presses into characters to send to the terminal is missing."
 
=== modified file 'src/app/qml/KeyboardBar.qml'
--- src/app/qml/KeyboardBar.qml	2015-07-13 20:25:21 +0000
+++ src/app/qml/KeyboardBar.qml	2016-01-12 10:43:25 +0000
@@ -12,6 +12,7 @@
 
     signal simulateCommand(string command);
     signal simulateKey(int key, int mod);
+    signal simulateModifier(int modifiers);
 
     ListModel {
         id: layoutsList
@@ -56,6 +57,7 @@
         layoutObject.z = rootItem.z;
         layoutObject.simulateKey.disconnect(simulateKey);
         layoutObject.simulateCommand.disconnect(simulateCommand);
+        layoutObject.simulateModifier.disconnect(simulateModifier);
     }
 
     function enableLayout(index) {
@@ -68,6 +70,7 @@
         layoutObject.z = rootItem.z + 0.01;
         layoutObject.simulateKey.connect(simulateKey);
         layoutObject.simulateCommand.connect(simulateCommand);
+        layoutObject.simulateModifier.connect(simulateModifier);
     }
 
     function isIndexLayoutValid(index) {
@@ -161,6 +164,7 @@
 
     onSimulateKey: pressFeedbackEffect.start();
     onSimulateCommand: pressFeedbackEffect.start();
+    onSimulateModifier: pressFeedbackEffect.start();
 
     Item {
         id: keyboardContainer
 
=== modified file 'src/app/qml/KeyboardRows/KeyboardLayout.qml'
--- src/app/qml/KeyboardRows/KeyboardLayout.qml	2015-08-09 14:25:36 +0000
+++ src/app/qml/KeyboardRows/KeyboardLayout.qml	2016-01-12 10:43:25 +0000
@@ -28,6 +28,8 @@
             return createKeyActionString(action.key, action.mod, action.text, action.id);
         case "string":
             return createStringActionString(action.string, action.text);
+        case "modifier":
+            return createNextModifierActionString(action.modifier);
         }
     }
 
@@ -56,6 +58,10 @@
         return "Action { " + textString + " onTriggered: simulateCommand(\"" + string + "\"); }";
     }
 
+    function createNextModifierActionString(modifier) {
+        return "Action { text: \"" + modifier + "\"; onTriggered: simulateModifier(Qt." + modifier + "Modifier); }";
+    }
+
     function createEntryString(text, actionString, otherActionsString) {
         var objectString = "
             import QtQuick 2.4
 
=== modified file 'src/app/qml/KeyboardRows/KeyboardRow.qml'
--- src/app/qml/KeyboardRows/KeyboardRow.qml	2015-07-13 20:25:21 +0000
+++ src/app/qml/KeyboardRows/KeyboardRow.qml	2016-01-12 10:43:25 +0000
@@ -14,6 +14,7 @@
     // External signals.
     signal simulateKey(int key, int mod);
     signal simulateCommand(string command);
+    signal simulateModifier(int modifiers);
 
     // Internal variables
     property int _firstVisibleIndex: gridView.contentX / keyWidth
 
=== modified file 'src/app/qml/KeyboardRows/Layouts/ControlKeys.json'
--- src/app/qml/KeyboardRows/Layouts/ControlKeys.json	2015-07-10 14:34:00 +0000
+++ src/app/qml/KeyboardRows/Layouts/ControlKeys.json	2016-01-12 10:43:25 +0000
@@ -4,6 +4,13 @@
     "buttons": [
         {
             "main_action" : {
+                "type": "modifier",
+                "modifier": "Control",
+                "text": "CTRL"
+            }
+        },
+        {
+            "main_action" : {
                 "type": "key",
                 "key" : "R",
                 "mod" : "Control"
 
=== modified file 'src/app/qml/KeyboardRows/Layouts/FunctionKeys.json'
--- src/app/qml/KeyboardRows/Layouts/FunctionKeys.json	2015-07-10 14:34:00 +0000
+++ src/app/qml/KeyboardRows/Layouts/FunctionKeys.json	2016-01-12 10:43:25 +0000
@@ -11,6 +11,13 @@
         },
         {
             "main_action" : {
+                "type": "modifier",
+                "modifier": "Control",
+                "text": "CTRL"
+            }
+        },
+        {
+            "main_action" : {
                 "type": "key",
                 "key" : "F1"
             }
 
=== modified file 'src/app/qml/KeyboardRows/jsonParser.js'
--- src/app/qml/KeyboardRows/jsonParser.js	2015-07-13 17:36:57 +0000
+++ src/app/qml/KeyboardRows/jsonParser.js	2016-01-12 10:43:25 +0000
@@ -22,12 +22,19 @@
         raiseException("string is missing in", stringObject);
 }
 
+function validateModifierAction(modifierObject) {
+    if (!modifierObject.modifier)
+        return raiseException("modifier is missing");
+    if (!isAllowed(modifierObject.modifier, ["Control", "Shift", "Alt"]))
+        return raiseException("modifier is invalid in", modifierObject);
+}
+
 function validateAction(actionObject) {
     if (!actionObject.type)
         raiseException("type is missing in", actionObject);
-    if (!isAllowed(actionObject.type, ["key", "string"]))
+    if (!isAllowed(actionObject.type, ["key", "string", "modifier"]))
         raiseException("type must be either key or string in", actionObject);
-    if (!(actionObject.type === "key" ? actionObject.key : actionObject.string))
+    if (!actionObject[actionObject.type])
         raiseException(actionObject.type + " is missing in", actionObject);
     if (actionObject.id && actionObject.text)
         raiseException("Should not define id and text together in", actionObject);
@@ -45,6 +52,9 @@
     case "string":
         validateStringAction(actionObject);
         break;
+    case "modifier":
+        validateModifierAction(actionObject);
+        break;
     }
 }
 
 
=== modified file 'src/app/qml/TerminalPage.qml'
--- src/app/qml/TerminalPage.qml	2015-07-13 20:25:21 +0000
+++ src/app/qml/TerminalPage.qml	2016-01-12 10:43:25 +0000
@@ -140,6 +140,7 @@
             height: units.gu(5)
             onSimulateKey: terminal.simulateKeyPress(key, mod, true, 0, "");
             onSimulateCommand: terminal.session.sendText(command);
+            onSimulateModifier: terminal.session.addNextModifiers(modifiers);
         }
     }
 
 
=== modified file 'src/plugin/qmltermwidget/lib/Emulation.cpp'
--- src/plugin/qmltermwidget/lib/Emulation.cpp	2014-11-12 00:10:12 +0000
+++ src/plugin/qmltermwidget/lib/Emulation.cpp	2016-01-12 10:43:25 +0000
@@ -55,7 +55,8 @@
   _codec(0),
   _decoder(0),
   _keyTranslator(0),
-  _usesMouse(false)
+  _usesMouse(false),
+  _nextModifiers(0)
 {
   // create screens with a default size
   _screen[0] = new Screen(40,80);
@@ -359,6 +360,11 @@
   return QSize(_currentScreen->getColumns(), _currentScreen->getLines());
 }
 
+void Emulation::addNextModifiers(int modifiers)
+{
+    _nextModifiers ^= modifiers;
+}
+
 ushort ExtendedCharTable::extendedCharHash(ushort* unicodePoints , ushort length) const
 {
     ushort hash = 0;
 
=== modified file 'src/plugin/qmltermwidget/lib/Emulation.h'
--- src/plugin/qmltermwidget/lib/Emulation.h	2014-11-12 00:10:12 +0000
+++ src/plugin/qmltermwidget/lib/Emulation.h	2016-01-12 10:43:25 +0000
@@ -218,6 +218,8 @@
    */
   bool programUsesMouse() const;
 
+  void addNextModifiers(int modifiers);
+
 public slots: 
 
   /** Change the size of the emulation's image */
@@ -442,6 +444,7 @@
   const QTextCodec* _codec;
   QTextDecoder* _decoder;
   const KeyboardTranslator* _keyTranslator; // the keyboard layout
+  int _nextModifiers;
 
 protected slots:
   /** 
 
=== modified file 'src/plugin/qmltermwidget/lib/Vt102Emulation.cpp'
--- src/plugin/qmltermwidget/lib/Vt102Emulation.cpp	2014-11-12 00:10:12 +0000
+++ src/plugin/qmltermwidget/lib/Vt102Emulation.cpp	2016-01-12 10:43:25 +0000
@@ -968,6 +968,14 @@
             textToSend += _codec->fromUnicode(event->text());
         }
 
+        if (textToSend.length() == 1 && _nextModifiers)
+        {
+            char first = textToSend[0];
+            if ((_nextModifiers & Qt::ControlModifier) && first >= 0x4f && first < 0x7f)
+                textToSend[0] = first & 0x1f;
+            _nextModifiers = 0;
+        }
+
         sendData( textToSend.constData() , textToSend.length() );
     }
     else
 
=== modified file 'src/plugin/qmltermwidget/src/ksession.cpp'
--- src/plugin/qmltermwidget/src/ksession.cpp	2014-11-12 00:10:12 +0000
+++ src/plugin/qmltermwidget/src/ksession.cpp	2016-01-12 10:43:25 +0000
@@ -22,6 +22,7 @@
 
 // Own
 #include "ksession.h"
+#include "Emulation.h"
 
 // Qt
 #include <QTextCodec>
@@ -211,6 +212,11 @@
 //    }
 }
 
+void KSession::addNextModifiers(int modifiers)
+{
+    m_session->emulation()->addNextModifiers(modifiers);
+}
+
 void KSession::setFlowControlEnabled(bool enabled)
 {
     m_session->setFlowControlEnabled(enabled);
 
=== modified file 'src/plugin/qmltermwidget/src/ksession.h'
--- src/plugin/qmltermwidget/src/ksession.h	2014-11-12 00:10:12 +0000
+++ src/plugin/qmltermwidget/src/ksession.h	2016-01-12 10:43:25 +0000
@@ -122,6 +122,7 @@
     void sendText(QString text);
     // Send some text to terminal
     void sendKey(int rep, int key, int mod) const;
+    void addNextModifiers(int modifiers);
 
 
 protected slots: