590349 - Clipboard (copy/paste) support for Android

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Matt Brubeck (:mbrubeck)]

Support the Android clipboard, so that we can implement copy/cut/paste for Fennec.

See bug 582912 for some front-end patches.

Comment 1

[Matt Brubeck (:mbrubeck)]

Sorry, front-end patch has moved to bug 585875.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Is this bug about nsIClipboard support on Android? And any front-end features would be exposed in a bug like bug 585875?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

yes, this bug is for supporting the OS clipboard on android through nsIClipboard

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch (this time with the new files)

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 469370 [details] [diff] [review]
patch (this time with the new files)

Nice. How did you test this?

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -423,4 +423,20 @@ class GeckoAppShell
>             return false;
>         }
>     }
>+
>+    static String getClipboardText() {
>+        Context context = GeckoApp.surfaceView.getContext();
>+        android.text.ClipboardManager cm = (android.text.ClipboardManager)
>+            context.getSystemService(Context.CLIPBOARD_SERVICE);
>+        if (!cm.hasText())
>+            return null;
>+        return cm.getText().toString();
>+    }
>+
>+    static void setClipboardText(String text) {
>+        Context context = GeckoApp.surfaceView.getContext();
>+        android.text.ClipboardManager cm = (android.text.ClipboardManager)
>+            context.getSystemService(Context.CLIPBOARD_SERVICE);
>+        cm.setText(text);
>+    }
> }
import android.text.*;

>diff --git a/widget/src/android/AndroidBridge.cpp b/widget/src/android/AndroidBridge.cpp
>--- a/widget/src/android/AndroidBridge.cpp
>+++ b/widget/src/android/AndroidBridge.cpp
>@@ -360,6 +362,48 @@ AndroidBridge::MoveTaskToBack()
>     mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jMoveTaskToBack);
> }
> 
>+PRBool
>+AndroidBridge::GetClipboardText(nsAString& aText)
>+{
>+    jstring jstrType =  
>+        static_cast<jstring>(mJNIEnv->
>+                             CallStaticObjectMethod(mGeckoAppShellClass,
>+                                                    jGetClipboardText));
>+    if (!jstrType)
>+        return PR_FALSE;
>+    nsJNIString jniStr(jstrType);
>+    aText.Assign(jniStr);
>+    return PR_TRUE;
>+}
>+
>+void
>+AndroidBridge::SetClipboardText(const nsAString& aText)
>+{
>+    jstring jstr = nsnull;
Move this to where it's assigned.

>+    const PRUnichar* wText;
>+    PRUint32 wTextLen = NS_StringGetData(aText, &wText);
This doesn't look safe given the existence of nsSubstringTuple.. but it looks like a bunch of other code do this, so ok..

>diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h
>+++ b/widget/src/android/AndroidBridge.h
>@@ -126,6 +126,14 @@ public:
> 
>     void MoveTaskToBack();
> 
>+    PRBool GetClipboardText(nsAString& aText);
>+
>+    void SetClipboardText(const nsAString& aText);
>+    
>+    void EmptyClipboard();
>+
>+    PRBool ClipboardHasText();
>+
Let's start switching to bool.

>+NS_IMETHODIMP
>+nsClipboard::SetData(nsITransferable *aTransferable, 
>+                     nsIClipboardOwner *anOwner, PRInt32 aWhichClipboard)
>+{
>+  if (aWhichClipboard != kGlobalClipboard)
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  nsCOMPtr<nsISupports> tmp;
>+  PRUint32 len;
>+  nsresult rv  = aTransferable->GetTransferData(kUnicodeMime, getter_AddRefs(tmp),
>+                                              &len);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<nsISupportsString> supportsString = do_QueryInterface(tmp);
>+  nsString buffer;
nsAutoString for strings inside functions. (unless you have good reason to believe your strings will be greater than 64 characters most of the time)

>+  supportsString->GetData(buffer);
>+  if (supportsString) {
So if we actually hit this check.. we should've crashed earlier, no?

>+    if (AndroidBridge::Bridge())
Return at the top of the function if there's no bridge.

>+      AndroidBridge::Bridge()->SetClipboardText(buffer);
>+  }
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsClipboard::GetData(nsITransferable *aTransferable, PRInt32 aWhichClipboard)
>+{
>+  if (aWhichClipboard != kGlobalClipboard)
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  if (AndroidBridge::Bridge()) {
Return early.

>+    nsString buffer;
nsAutoString

>+    if (!AndroidBridge::Bridge()->GetClipboardText(buffer))
>+      return NS_ERROR_UNEXPECTED;
>+
>+    nsresult rv;
>+    nsCOMPtr<nsISupportsString> dataWrapper =
>+      do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    
>+    rv = dataWrapper->SetData(buffer);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // If our data flavor has already been added, this will fail. But we don't care
>+    aTransferable->AddDataFlavor(kUnicodeMime);
>+
>+     nsCOMPtr<nsISupports> nsisupportsDataWrapper =
Incorrect indentation. Also, I don't think do_QueryInterface to nsISupports is required since nsISupportsString inherits from nsISupports.

>+      do_QueryInterface(dataWrapper);
>+    rv = aTransferable->SetTransferData(kUnicodeMime, nsisupportsDataWrapper,
>+                                buffer.Length() * sizeof(PRUnichar));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+  }
>+  return NS_OK;
>+}
>+

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #6)
> Comment on attachment 469370 [details] [diff] [review]
> patch (this time with the new files)
> 
> Nice. How did you test this?

I wrote some js in about:home to grab text from the clipboard and put it in a div and then set new text to the clipboard. I then pasted that into a text message. Everything worked.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #6)
> Comment on attachment 469370 [details] [diff] [review]
> patch (this time with the new files)
> >+    // If our data flavor has already been added, this will fail. But we don't care
> >+    aTransferable->AddDataFlavor(kUnicodeMime);
> >+
> >+     nsCOMPtr<nsISupports> nsisupportsDataWrapper =
> Incorrect indentation. Also, I don't think do_QueryInterface to nsISupports is
> required since nsISupportsString inherits from nsISupports.
> 

leaving that in the patch because I'm following the lead from here: http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsClipboardHelper.cpp#113

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 469677 [details] [diff] [review]
patch

r=mwu with s/nsString/nsAutoString/ in nsClipboard::GetData and a space added after "jSetClipboardText," in AndroidBridge::SetClipboardText.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

landed this almost a month ago: http://hg.mozilla.org/mozilla-central/rev/6f3b20dd902a