Review: Needs Fixing 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 -- https://code.launchpad.net/~popey/ubuntu-terminal-app/add-control/+merge/282280 Your team Ubuntu Terminal Developers is subscribed to branch lp:ubuntu-terminal-app. -- Mailing list: https://launchpad.net/~ubuntu-touch-coreapps-reviewers Post to : ubuntu-touch-coreapps-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~ubuntu-touch-coreapps-reviewers More help : https://help.launchpad.net/ListHelp
