Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Oh, and I'll set up an email to Alan regarding the mailing list thing. :) -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Review: Needs Fixing Thank you for the patch! The code looks good to me and I assume Stefano tested it already. However, I'm sorry that I am _that_ guy again. :p For comments we usually put a whitespace after the two leading slashes. Additionally, we only put semi-colons after QML properties when there are two in the same line. Again, sorry for that, but I think it makes sense to agree on a common code style. Nevertheless, good work! :) -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
It's all good Niklas, let me fix that real quick :) -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Review: Approve continuous-integration PASSED: Continuous integration, rev:169 https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1013/ Executed test runs: None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/565/console Click here to trigger a rebuild: https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1013/rebuild -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Alright, that makes total sense, thanks :) -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
[Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
The proposal to merge lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app has been updated. Status: Needs review => Approved For more details, see: https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 -- 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Review: Approve Yes, it is. You'll really come to appreciate QML after a while. Furthermore, thanks for fixing it. Let's get this merged now. Good job! :) -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Review: Approve continuous-integration PASSED: Continuous integration, rev:170 https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1014/ Executed test runs: None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/566/console Click here to trigger a rebuild: https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1014/rebuild -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
[Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
The proposal to merge lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app has been updated. Status: Approved => Merged For more details, see: https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 -- 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
I should have made it a bit clearer what I've been referring to with the
semicolons. :D
We do use semicolons for executions of commands, e.g. in JavaScript code blocks
surrounded by {} and in other execution blocks like onTriggered or onClicked
listeners.
We do not use semicolons for QML bindings, i.e. when a value is assigned to a
property using a colon.
Examples for when we do use a semicolon:
onTriggered: {
console.log("test");
terminalPage.state = "SELECTION";
}
onTriggered: terminalPage.state = "SELECTION";
Examples for when we don't use semicolons:
width: units.gu(2)
text: i18n.tr("Select")
enabled: !terminal.isSelectionEmpty()
I hope that this has made it a little bit clearer. If not, please ask! ;)
--
https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287
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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
I also didn't know the code in there was JS, that's pretty cool! -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Review: Approve BTW, code looks good! Great work! -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 Your team Ubuntu Terminal Developers is requested to review the proposed merge of lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into 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
[Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Evan McIntire has proposed merging
lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app.
Commit message:
Disable copy if selection is empty
Requested reviews:
Ubuntu Terminal Developers (ubuntu-terminal-dev)
For more details, see:
https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287
Disable copy if selection is empty
--
Your team Ubuntu Terminal Developers is requested to review the proposed merge
of lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into
lp:ubuntu-terminal-app.
=== modified file 'src/app/qml/AlternateActionPopover.qml'
--- src/app/qml/AlternateActionPopover.qml 2016-02-06 20:16:58 +
+++ src/app/qml/AlternateActionPopover.qml 2016-02-07 07:34:33 +
@@ -16,6 +16,7 @@
Action {
text: i18n.tr("Copy")
onTriggered: terminal.copyClipboard();
+enabled: !terminal.isSelectionEmpty();
}
Action {
text: i18n.tr("Paste")
=== modified file 'src/plugin/qmltermwidget/lib/TerminalDisplay.cpp'
--- src/plugin/qmltermwidget/lib/TerminalDisplay.cpp 2016-02-06 20:16:58 +
+++ src/plugin/qmltermwidget/lib/TerminalDisplay.cpp 2016-02-07 07:34:33 +
@@ -2570,11 +2570,18 @@
emitSelection(true,false);
}
+//TODO: These 2 functions are not in the upstream LxQt version
+//See https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-paste/+merge/283244/comments/725331
bool TerminalDisplay::isClipboardEmpty()
{
return QApplication::clipboard()->text().isEmpty();
}
+bool TerminalDisplay::isSelectionEmpty()
+{
+return _screenWindow->selectedText(_preserveLineBreaks).isEmpty();
+}
+
/* - */
/* */
/*Keyboard */
=== modified file 'src/plugin/qmltermwidget/lib/TerminalDisplay.h'
--- src/plugin/qmltermwidget/lib/TerminalDisplay.h 2016-02-06 20:16:58 +
+++ src/plugin/qmltermwidget/lib/TerminalDisplay.h 2016-02-07 07:34:33 +
@@ -482,10 +482,12 @@
*/
void pasteSelection();
-/** Checks if the clipboard is empty
- */
+/** Checks if the clipboard is empty */
bool isClipboardEmpty();
+/** Checks if the selection is empty */
+bool isSelectionEmpty();
+
/**
* Changes whether the flow control warning box should be shown when the flow control
* stop key (Ctrl+S) are pressed.
--
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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
As Stefano said[1], I do think we should avoid adding too much to the C++ code. Whenever we end up redoing the clipboard stuff we could maybe remove this and the isClipboardEmpty() functions? There are a few bugs relating to improving the clipboard experience, maybe we could make a blueprint or something to organize all that and how we want to redo it? [1] - https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-paste/+merge/283244/comments/725331 -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 Your team Ubuntu Terminal Developers is requested to review the proposed merge of lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into 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
Re: [Ubuntu-touch-coreapps-reviewers] [Merge] lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:ubuntu-terminal-app
Review: Approve continuous-integration PASSED: Continuous integration, rev:168 https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1010/ Executed test runs: None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/561/console Click here to trigger a rebuild: https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1010/rebuild -- https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-copy/+merge/285287 Your team Ubuntu Terminal Developers is requested to review the proposed merge of lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into 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
