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

Thank you for the patch!

I really see why you want to implement this feature. However, there are some issues with the code.

What this currently does is the following: it hides the paste option when the user has selected some text and shows it if there is no selection. So currently this does not at all check if the clipboard is empty or not.
In order to change that, you would probably want to use the Clipboard API for that: https://developer.ubuntu.com/api/apps/qml/sdk-15.04.1/Ubuntu.Components.Clipboard/
It should do the trick. ;)

Cheers,
Niklas

Hmm, I did fix that already, i guess the commit didn't push, I'll look into it when I get home

Looks good!

Thanks for changing this. :)

However, there are four lines in the pot file which seem to stem from a merge error. We need to fix this before we can merge the MP. The easiest way to do that would probably be to just run it once again from the Ubuntu SDK and then commit the automatic pot file updates. Would you mind doing that? :)

Thank you very much in advance!

Sure, thanks for the review! These small merge issues always get me

Hmm, still not fixed. Any pointers? I assume this is a bzr-ism or a launchpad-ism I'm not used to

Looks like there have been a few changes in trunk since you branched off your tree. Here is how to resolve this:

$ bzr branch lp:~mcintire-evan/ubuntu-terminal-app/disable-paste
$ bzr branch lp:ubuntu-terminal-app trunk
$ cd disable-paste
$ bzr merge ../trunk
$ bzr conflicts # should list one conflict
$ nano po/com.ubuntu.terminal.pot
$ # Now resolve the conflict by removing the old date and the lines added by bzr
$ bzr resolve po/com.ubuntu.terminal.pot
$ bzr commit -m "Merge trunk"

That's it. :)

Give it a try. I agree that it can be a bit strange at first. ;)

Alright, there we go :)

Also, quick question, does launchpad squish all the commits from a branch into one when it merges

It does, although the individual commits are still there and can be seen under the main merge commit with tools such as 'bzr qlog' ('sudo apt install qbzr' to use it).

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

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

PASSED: Continuous integration, rev:155
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/982/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/527/console

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

PASSED: Continuous integration, rev:155
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/989/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/534/console

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

Tested on my BQ, and looks good. Nice job! :D

Having a look at Niklas' review, I guess the "needs-fixing" is about the conflict with .pot, so there shouldn't be any problem in getting this branch merged.

Anyway, here's some note for a further task:

1) I believe it's better to use the Qt APIs (instead of Ubuntu.Components.Clipboard) since currently the QML GUI does not handle the clipboard (everything is done internally in the terminal widget). This could change in future, but I don't think it's a good idea to change heavily the C++ terminal code yet (Filippo, the former terminal-app maintainer, aimed to keep the code in sync with the upstream project from LxQt).

2) I'd prefer to set "enabled: !terminal.isClipboardEmpty()" instead of "visible" in order to stay consistent with the rest of the platform (i.e. see TextField/TextArea popover).
   The entry in the popover should be always visible, but not triggerable if not necessary (I should check the UI specs though).

3) I'd expect to see a similar MP for the copy action too. Currently the action is still visible when no text is actually selected. (TBD in a different branch though)

4) Probably we'd like to redesign the whole popover, so that it looks similar to the popover used in the TextField/TextArea. We'd need to check if the component is publicly available or if we can get its style through a StyledItem.

About the .pot conflict:
If you don't need to update the translations in your branch, remember to "bzr revert po/*.pot" before committing. That way you're sure you won't get any annoying conflict.

Thank you again! :)

Thanks for the review - I linked your comment in a wishlist bug[1] relating to improving paste functionality, there are actually a few relating to that idea that we could work towards figuring out how we want it :)

[1] https://bugs.launchpad.net/ubuntu-terminal-app/+bug/1396974

Thank you for fixing this, Evan! The MP looks very good. :)

@Stefano: You make a really good point with (1). Let's keep that in mind when we continue to add features. :)

[Another note regarding pot file conflicts: If you have already committed stuff which causes a merge conflict, you can revert the po directory to the state which it was in before your first commit using

bzr revert -r 151 po/

where 151 is the revision before your first commit. That will also be handy sometimes. ;) ]

Let's merge the MP now!

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/terminal-app-autolanding/85/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/3390/console

Merging 'lp:ubuntu-terminal-app' in to 'build_dir'.
Text conflict in po/com.ubuntu.terminal.pot
1 conflicts encountered.
bzr: ERROR: Conflicts from merge

Looks like the pot file merge conflict we've been talking about. Try the command I posted above. ;)

Wow, I made it worse. Just a moment, let me re-merge the trunk in and fix all of this nonsense

Ooh, the magical world of Launchpad trying to merge a .pot update when the
.pot has been updated in the meantime. :)

P.S. Now you know how a DocViewer developer feels, haha :'D

P.S. #2 We probably should prevent this from happening, by avoiding to
update translations every time the app is built in QtCreator
Il 06/feb/2016 21:01, "Evan McIntire" <email address hidden> ha scritto:

Wow, I made it worse. Just a moment, let me re-merge the trunk in and fix
all of this nonsense
--
https://code.launchpad.net/~mcintire-evan/ubuntu-terminal-app/disable-paste/+merge/283244
You are reviewing the proposed merge of
lp:~mcintire-evan/ubuntu-terminal-app/disable-paste into
lp:ubuntu-terminal-app.

@Stefano: Will Launchpad build the pot file itself in that case or how will it pick up new translatable strings then?

@Niklas: Changing the behaviour of the .pot custom target (i.e. removing the "ALL" option) in CMake should be enough, assuming nothing else will be broken.

That would mean that you'd have to explicitely do:
  mkdir build && cd dir
  cmake ../
  make <translations_target_name>
  (bzr commit & bzr push)

So it would be something you should do whenever you're about to release a new version of the app.
We may want to add a script in the root of the project for making this easier for new contributors.

@Stefano: I'm open to that.

However, we need to make sure to still build a new .pot file every once in a while and with enough time before the next release in order to make sure that translators have at least a week to get their translations in.