Skip to content

Conversation

@mmeeks
Copy link
Contributor

@mmeeks mmeeks commented Jun 28, 2019

Browsers will allow apps to populate the clipboard, but not read
from it without user input. This patch makes it possible for
context menus with 'Paste' in them to trigger a real OS paste
when hosted inside the webview.

Signed-off-by: Michael Meeks michael.meeks@collabora.com

@tobiasKaminsky
Copy link
Member

Can you give me steps how to test this?
I did this

  • opened up a document within NC app
  • long pressed on text to select it & copy it
  • long pressed on a white area to paste it -> nothing happened
  • edit -> insert text -> worked
    (but latest step also worked without this PR)

@mmeeks
Copy link
Contributor Author

mmeeks commented Jul 1, 2019 via email

@tobiasKaminsky
Copy link
Member

So - you'll need an updated C'bra Online build that posts the PostMessage on paste at the right time to make that work - we're just doing some final testing on that.

Hm, but how/by whom is this postMessage triggered?
(just for understanding)

As this is not in official release, there is no need for backporting it to 3.7.

@tobiasKaminsky
Copy link
Member

Please ping me, once there is a Collabora version, I can test (it can also be a docker tag/snapshot).

@tobiasKaminsky
Copy link
Member

When I now try to long-press to select test, I get following:

2019-07-02-073911

@mmeeks
Copy link
Contributor Author

mmeeks commented Jul 10, 2019

Ah - this is easy; you need to double-tap to select text, not long-press =)

@tobiasKaminsky
Copy link
Member

I did this on our collabora staging server:

  • typed text
  • double click on it
  • select a snippet
  • select copy
  • go to a new line
  • long press -> paste
    2019-07-11-141652
  • at least on my emulator stock keyboard I cannot paste…

@mmeeks
Copy link
Contributor Author

mmeeks commented Jul 11, 2019

Any console output ? I wonder - it could be a cross-site origin issue - we attempt to post directly to the top window, which may cause concern. Thanks.

@tobiasKaminsky
Copy link
Member

No meaningful error/info output.

On my OnePlus3 with Swiftkey I can use the keyboard to paste the text snippet.
Is this the desired way?

@mmeeks
Copy link
Contributor Author

mmeeks commented Jul 11, 2019

Sure, using the system paste functionality should work, this is primarily intended to make non-system paste work ie. via the context menus or menu -> edit -> paste. And you're sure you're using the updated/instrumented app ? - I'd be curious to know if the postmessage arrives of course. Thanks.

@tobiasKaminsky
Copy link
Member

And you're sure you're using the updated/instrumented app ?

I am using the android app with this patch and collabora.nc (our staging server) for testing. This is correct, or?

@tobiasKaminsky
Copy link
Member

@mmeeks @juliushaertl if you also want to test this, in this PR is a QR code where you can install the app as "nextcloud qa" in parallel to existing one.

@juliusknorr
Copy link
Member

@mmeeks The android app doesn't use postmessages for the integration but a call to window.RichDocumentsMobileInterface.paste() on the top frame.

See https://github.com/nextcloud/richdocuments/blob/842d5f2ef665edabaed581701a849736adcfde5c/js/documents.js#L940-L959 for the richdocuments implementation

@tobiasKaminsky
Copy link
Member

Tested together with @juliushaertl and @mmeeks and works 🎉

@nextcloud-android-bot
Copy link
Collaborator

@tobiasKaminsky tobiasKaminsky force-pushed the webview-paste branch 2 times, most recently from cc6ff7a to 63d2736 Compare July 23, 2019 04:36
@nextcloud-android-bot
Copy link
Collaborator

@AndyScherzinger
Copy link
Member

@tobiasKaminsky doesn't this need another rebase and fixes in the score-file?

@tobiasKaminsky
Copy link
Member

Yes, now that #4253 is merged, I'll do a rebase.

@AndyScherzinger
Copy link
Member

@tobiasKaminsky there are two lint warnings:

Field requires API level 24 (current min is 21): android.view.KeyEvent#KEYCODE_PASTE

@tobiasKaminsky
Copy link
Member

@mmeeks this is only possible for at least Nougat (Android 7), but RichDocuments/Collabora works with at least Lollipop (5.0).
Do you have an idea how we can support back to 5.0?
What does happen if I just do nothing on 5.0/6.0? Then pasting will just not work, or?

@mmeeks
Copy link
Contributor Author

mmeeks commented Jul 23, 2019

Ah - if we do nothing in older versions, then there will just be a popup saying "Please use a system paste button" or somesuch, which is fine - we can cope with that.

Ideally we would have an 'internal paste' option inside that dialog I guess.

Wrt. invoking a paste method dynamically, I guess there is a Java mechanism for that that will work if it is there (?) - but no expert; sorry for not noticing that.

@mmeeks
Copy link
Contributor Author

mmeeks commented Jul 23, 2019

Oh, and ... it is possible that there is a 'Paste Key' - that could be inserted instead. Oddly that may work just as well and be more compatible (?).

@tobiasKaminsky
Copy link
Member

I only found https://developer.android.com/guide/topics/text/copy-paste#PastePlainText, but this is for accessing content of clip board.

Pasting into webview/RichDocument is another step, which requires a javascript interface?
Then it would also be possible to copy text in any other android app and paste it into collabora (as right now you can only copy&paste within the same document, or?)

@mmeeks
Copy link
Contributor Author

mmeeks commented Jul 24, 2019

With the other pieces of this work - you should be able to paste form anywhere, whenever you like without using a system-paste event. The problem is that a system paste event cannot be triggered from Javascript. In consequence we have to get it emitted by the native code. That could (probably) be done with a paste key injection instead. I guess some code like:

BaseInputConnection mInputConnection = new BaseInputConnection( findViewById(R.id.main_content), true);
KeyEvent kd = new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_PASTE);
KeyEvent ku = new KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_PASTE);
mInputConnection.sendKeyEvent(kd);
mInputConnection.sendKeyEvent(ku);

Or somesuch; or even InputConnecction's sendKeyEvent ... not sure; for the case where that API is not there (?).

@tobiasKaminsky
Copy link
Member

The problem with your new code idea is that KeyEvent.KEYCODE_PASTE only exists beginning from API 24 only.
As long as we do not find a way to do a paste on <24, nothing will happen on Lollipop and Marshmallow (5.x and 6.x).

mmeeks and others added 2 commits July 25, 2019 14:13
Browsers will allow apps to populate the clipboard, but not read
from it without user input. This patch makes it possible for
context menus with 'Paste' in them to trigger a real OS paste
when hosted inside the webview.

Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #4189 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #4189      +/-   ##
============================================
- Coverage     14.33%   14.31%   -0.03%     
  Complexity        1        1              
============================================
  Files           331      331              
  Lines         31030    31032       +2     
  Branches       4405     4405              
============================================
- Hits           4447     4441       -6     
- Misses        25801    25808       +7     
- Partials        782      783       +1
Impacted Files Coverage Δ Complexity Δ
...loud/android/ui/activity/RichDocumentsWebView.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../third_parties/daveKoeller/AlphanumComparator.java 80.95% <0%> (-2.39%) 0% <0%> (ø)
...in/java/com/owncloud/android/utils/ThemeUtils.java 49.14% <0%> (-1.71%) 0% <0%> (ø)
.../android/authentication/AuthenticatorActivity.java 1.88% <0%> (ø) 0% <0%> (ø) ⬇️

@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #4189 into master will increase coverage by 2.4%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master    #4189     +/-   ##
===========================================
+ Coverage     14.33%   16.74%   +2.4%     
  Complexity        1        1             
===========================================
  Files           331      356     +25     
  Lines         31030    31684    +654     
  Branches       4405     4482     +77     
===========================================
+ Hits           4447     5304    +857     
+ Misses        25801    25487    -314     
- Partials        782      893    +111
Impacted Files Coverage Δ Complexity Δ
...loud/android/ui/activity/RichDocumentsWebView.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../third_parties/daveKoeller/AlphanumComparator.java 80.95% <0%> (-2.39%) 0% <0%> (ø)
...a/com/owncloud/android/utils/FileStorageUtils.java 20.09% <0%> (-0.2%) 0% <0%> (ø)
src/main/java/com/owncloud/android/MainApp.java 54.54% <0%> (-0.16%) 0% <0%> (ø)
.../android/authentication/AuthenticatorActivity.java 1.87% <0%> (-0.02%) 0% <0%> (ø)
...ud/android/providers/DocumentsStorageProvider.java 1.06% <0%> (-0.01%) 0% <0%> (ø)
...roid/ui/activity/ReceiveExternalFilesActivity.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...loud/android/ui/activities/ActivitiesActivity.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...loud/android/datamodel/FilesystemDataProvider.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...owncloud/android/ui/adapter/UploadListAdapter.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 74 more

@nextcloud-android-bot
Copy link
Collaborator

@tobiasKaminsky
Copy link
Member

tobiasKaminsky commented Jul 29, 2019

@mmeeks I would merge it as is:
>= Nougat: we have a proper paste
< Nougat (and >= Lollipop as we only support Collabora since then): we do get the "please use system paste" message.

Or do you have another idea?

@AndyScherzinger AndyScherzinger mentioned this pull request Aug 2, 2019
68 tasks
@tobiasKaminsky
Copy link
Member

Merging after CI is green as it is for >= Nougat an improvement.

@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10476.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

Codacy

286

Lint

TypemasterPR
Warnings5858
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings24
Correctness Warnings69
Internationalization Warnings12
Malicious code vulnerability Warnings4
Multithreaded correctness Warnings9
Performance Warnings111
Security Warnings46
Dodgy code Warnings138
Total413

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings24
Correctness Warnings69
Internationalization Warnings12
Malicious code vulnerability Warnings4
Multithreaded correctness Warnings9
Performance Warnings111
Security Warnings46
Dodgy code Warnings138
Total413

@nextcloud nextcloud deleted a comment from AndyScherzinger Aug 20, 2019
@tobiasKaminsky tobiasKaminsky merged commit 5373660 into nextcloud:master Aug 20, 2019
@tobiasKaminsky tobiasKaminsky added this to the Nextcloud App 3.8.0 milestone Aug 20, 2019
tobiasKaminsky added a commit that referenced this pull request Aug 21, 2019
b36067d Merge pull request #4294 from AlexNi245/#2216-activity-data-divers-design
9624019 activity header has now the same font size as a activity element
fa11565 fixed issue  Avoid reassigning parameters such as 'itemPosition'
5a3db5e replace do while loop in getHeaderPositionForItem with while loop
1dd2a5a change naming of Canvas c to Canvas canvas
f4e964e change class name of ActivityListItemDecoration to StickyHeaderItemDecoration, which is more generic
52a7ffe add license text
6ffa9d0 remove duplicate entry of setContentView
cdd5d38 add getHeaderPositionForItem unit test
0ac5a8b add static import for com.owncloud.android.lib.resources.activities.model.Activity;
4ea9667 unit test for isHeader(int pos)
b61f9e0 format ActivityListAdapter
9012f43 set visibility from ActivityViewHeaderHolder back to protected
7a30070 remove unnecessary TAG field and replace nested if statement with && to fix codeacy-bot issues
a26a895 apply changes to java doc
c41dd38 optimize imports
b4764f7 remove unnecessary files
792a6b9 finish implementation of sticky header implementation. This feature was created according to the this implementation : https://stackoverflow.com/questions/32949971/how-can-i-make-sticky-headers-in-recyclerview-without-external-lib
13e7aff increase height of header element and set backgroundcolor to white
a184345 first implementation of sticky header logic.
cd3d8dc start to implement sticky header behavior
259e106 Merge pull request #4355 from nextcloud/drawer
caf1842 drawer: show only server address
cb7d4a3 revert to old image (#4356)
7e44fec revert to old image
f7c4eec Merge pull request #4345 from nextcloud/push
a3fda5a no need to use owncloudClient
9a023c6 Check if app is excluded from battery optimization (#3589)
527c5db Use conscrypt (#4314)
6014e90 use conscrypt
5373660 Provide a banal 'paste' postmessage implementation. (#4189)
68cebf8 revert DeviceModule back to Kotlin
969ce78 Drone: update FindBugs results to reflect reduced error/warning count [skip ci]
f26095f Merge pull request #4245 from fogninid/fixUploadListComparator
e72a789 show on special vendors "disable power check" in auto upload menu - tint button - change logic when to show battery warning
c8173c3 Use reloading on photo view (#2250)
c38dcf3 Delete temp file on receive external files (#4349)
c155edf Merge pull request #4347 from nextcloud/blacklistThumbnail
a03ff84 daily dev 20190820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants