Merge lp://staging/~nikwen/ubuntu-calculator-app/delete-copy-paste-fix into lp://staging/ubuntu-calculator-app

Proposed by Niklas Wenzel
Status: Rejected
Rejected by: Bartosz Kosiorek
Proposed branch: lp://staging/~nikwen/ubuntu-calculator-app/delete-copy-paste-fix
Merge into: lp://staging/ubuntu-calculator-app
Diff against target: 98 lines (+61/-15)
1 file modified
app/ubuntu-calculator-app.qml (+61/-15)
To merge this branch: bzr merge lp://staging/~nikwen/ubuntu-calculator-app/delete-copy-paste-fix
Reviewer Review Type Date Requested Status
Bartosz Kosiorek Disapprove
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Review via email: mp+264621@code.staging.launchpad.net

Commit message

Implement proper copy/paste functionality (fixes the delete/backspace issue by removing the previous implementation)

Description of the change

Implement proper copy/paste functionality (fixes the delete/backspace issue by removing the previous implementation)

To post a comment you must log in.
Revision history for this message
Niklas Wenzel (nikwen) wrote :

Please don't blame me for LP: #1474072. That needs tackling though. ;)

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Bartosz Kosiorek (gang65) wrote :

Great job.

I have two remarks:
1. You introduced here Paste feature. We are currently not support Pasting in InputTextField. It is readonly, so you could only copy from that. You could check it by long pressing on InputTextField, there is only option "Copy". To introduce Paste feature, we need to enable it both for Phone and for Desktop.

Please create Bug report about allow pasting into calculator. And add Ubuntu UX team. They need to decide how user should be notified if wrong data is pasted. Maybe we should just paste without parsing, and trust user (we will be validate during formula push).

2. My idea was to not introduce any more regression with physical keyboard. Could you please create autopilot tests to handle "Enter", "Backspace" and "Deleted" keys? I think you already fixed that. I don't want to break phsyical keyboard support again :-)
Maybe it will be good to merge first initial physical keyboard support:
https://code.launchpad.net/~gang65/ubuntu-calculator-app/ubuntu-calculator-app-keyboard-test
?

review: Needs Fixing
Revision history for this message
Bartosz Kosiorek (gang65) wrote :

Please split this MR into two parts:
1. fixes the delete and backspace, proper copy
2. paste feature

Revision history for this message
Niklas Wenzel (nikwen) wrote :

Hi, I'm sorry for the late reply. Thanks a lot for reviewing this. :)

I have now split the MP as requested:

1) https://code.launchpad.net/~nikwen/ubuntu-calculator-app/delete-backspace-copy-fix/+merge/266660
2) https://code.launchpad.net/~nikwen/ubuntu-calculator-app/desktop-paste-support/+merge/266662

I'll put autopilot tests on my todo list and will propose a separate MP for that. Thank you for your MP regarding this!

Regarding paste support, I would prefer merging the current desktop-only solution for now until we have a response from the UX team. We can then enable it on mobile as well.
Here is the bug report: https://bugs.launchpad.net/ubuntu-calculator-app/+bug/1480662

Revision history for this message
Bartosz Kosiorek (gang65) wrote :

Thanks for splitting this branch to two separate.

I'm closing this branch, to not confuse developers.

review: Disapprove
Revision history for this message
Niklas Wenzel (nikwen) wrote :

No problem. ;)

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches