Merge lp:~mcintire-evan/ubuntu-terminal-app/disable-copy into lp:~ubuntu-terminal-dev/ubuntu-terminal-app/reboot

[Evan McIntire]

Disable copy if selection is empty

Disable copy if selection is empty

[Evan McIntire (mcintire-evan)]

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

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

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

review: Approve (continuous-integration)

[Stefano Verzegnassi (verzegnassi-stefano)]

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

I think we can keep them as long as they are pure Qt implementations.
I had a further look at the usage of Ubuntu.Components.Clipboard, in particular for uri type, which seems to be handled differently in Gnome Terminal.

e.g.
1) Open Nautilus
2) Choose a file from your home and copy it (Ctrl+C)
3) Open gnome-terminal and paste (Ctrl+Alt+V)
   RESULT: the object is pasted as "file:///home/<username>/<file_name+ext>"
4) Do the same with ubuntu-terminal-app
   RESULT: the object is pasted as "/home/<username>/<file_name+ext>"

Fortunately, the Ubuntu UITK implementation is just a wrapper around QClipboard, which is already used in QmlTermWidget, so we can go on with a pure Qt/C++ implementation with no many changes.

A different case is the drop of a file into the terminal window, which is something ubuntu-terminal-app does not handle yet.

This could be done through QML, although it would be better to do via C++, since all the mouse/keyb events are currently handled by the terminal widget itself.
In this case we would end up with a bigger change to the source code.

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

Blueprints + bugs sounds as a good plan.

I believe we should review the code of the terminal plugin. I had a look some week ago, and only the console has been ported from QWidget to QtQuick (e.g. see the search functionalities, still QWidget-based).

As we have a list of things we need to improve, we can start planning the changes to the C++ sources, being sure we won't break the compatibility with QTermWidget or cool-retro-term.

I will try to get in touch with Filippo, since he's the author of the porting.

It could be useful to discuss of this somewhere else.
I know Niklas is quite busy in this period, and restoring the weekly meeting could be complicated.
What about moving our discussion on mails?

[Stefano Verzegnassi (verzegnassi-stefano)]

BTW, code looks good! Great work!

review: Approve

[Evan McIntire (mcintire-evan)]

Thanks! Mailing lists could work for me, I don't know where we do that though. I have some other questions/ideas, but those are better left for when we figure out mailing list stuff :)

[Niklas Wenzel (nikwen)]

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

review: Needs Fixing

[Niklas Wenzel (nikwen)]

Oh, and I'll set up an email to Alan regarding the mailing list thing. :)

[Niklas Wenzel (nikwen)]

Email sent. :)

[Evan McIntire (mcintire-evan)]

It's all good Niklas, let me fix that real quick :)

[lp:~mcintire-evan/ubuntu-terminal-app/disable-copy]

169. By Evan McIntire on 2016-02-08

Slight stlye changes

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

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

review: Approve (continuous-integration)

[Niklas Wenzel (nikwen)]

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

[Evan McIntire (mcintire-evan)]

Alright, that makes total sense, thanks :)

[lp:~mcintire-evan/ubuntu-terminal-app/disable-copy]

170. By Evan McIntire on 2016-02-08

Small style change again

[Niklas Wenzel (nikwen)]

For my personal projects I also use a different code style but that's what the terminal app uses. ;)

[Evan McIntire (mcintire-evan)]

I also didn't know the code in there was JS, that's pretty cool!

[Niklas Wenzel (nikwen)]

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

review: Approve

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

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

review: Approve (continuous-integration)

=== modified file 'src/app/qml/AlternateActionPopover.qml'
--- src/app/qml/AlternateActionPopover.qml	2016-02-06 20:16:58 +0000
+++ src/app/qml/AlternateActionPopover.qml	2016-02-08 22:27:23 +0000
@@ -15,12 +15,13 @@
             }
             Action {
                 text: i18n.tr("Copy")
+                enabled: !terminal.isSelectionEmpty()
                 onTriggered: terminal.copyClipboard();
             }
             Action {
                 text: i18n.tr("Paste")
+                enabled: !terminal.isClipboardEmpty()
                 onTriggered: terminal.pasteClipboard();
-                enabled: !terminal.isClipboardEmpty();
             }
         }
     }

=== modified file 'src/plugin/qmltermwidget/lib/TerminalDisplay.cpp'
--- src/plugin/qmltermwidget/lib/TerminalDisplay.cpp	2016-02-06 20:16:58 +0000
+++ src/plugin/qmltermwidget/lib/TerminalDisplay.cpp	2016-02-08 22:27:23 +0000
@@ -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 +0000
+++ src/plugin/qmltermwidget/lib/TerminalDisplay.h	2016-02-08 22:27:23 +0000
@@ -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.