769438 - Support sharing images via long press

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

We should be able to share an image by long pressing it. If possible we should pass the actual image data, but if the app only supports the url we should allow that as well.

Comment 1

[Arkady Blyakher [:rkd]]

Attachment: share images

Comment 2

[Arkady Blyakher [:rkd]]

Attachment: share images with long press

TODO: don't use Input/Output Stream in Java

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 640410 [details] [diff] [review]
share images with long press

Review of attachment 640410 [details] [diff] [review]:
-----------------------------------------------------------------

Pretty good, especially for the first patch! A few things to clean up and we'll be able to get this in. Great job.

::: mobile/android/base/GeckoApp.java
@@ +1249,5 @@
>              } else if (event.equals("Share:Text")) {
>                  String text = message.getString("text");
>                  GeckoAppShell.openUriExternal(text, "text/plain", "", "", Intent.ACTION_SEND, "");
> +            } else if (event.equals("Share:Image")) {
> +				//RKD

Nuke this

@@ +1253,5 @@
> +				//RKD
> +                String src = message.getString("url");
> +                String type = message.getString("mime");
> +                GeckoAppShell.shareImage(src, type);
> +			}

Wrong indention

@@ +2232,5 @@
>          // etc., and generally mark the profile as 'clean', and then
>          // dirty it again if we get an onResume.
>  
>          GeckoAppShell.sendEventToGecko(GeckoEvent.createStoppingEvent(isApplicationInBackground()));
> +

Unnecessary whitespace

@@ +2301,5 @@
> +        GeckoAppShell.unregisterGeckoEventListener("Share:Image", GeckoApp.mAppContext);
> +
> +        // If we created a directory to store shared images, delete any files in it.
> +        File sdcard = Environment.getExternalStorageDirectory();
> +        File dir = new File(sdcard.getAbsolutePath() + "/fennec");

Let's use 'firefox' for the directory name.

@@ +2305,5 @@
> +        File dir = new File(sdcard.getAbsolutePath() + "/fennec");
> +        if (dir.exists()) {
> +            // Delete any any existing temporary files
> +            File[] files = dir.listFiles();
> +            if (files.length > 0) {

This should check for null, not length.

@@ +2307,5 @@
> +            // Delete any any existing temporary files
> +            File[] files = dir.listFiles();
> +            if (files.length > 0) {
> +                for (File file : files) {
> +                    Log.i(LOGTAG, "Deleting file " + file.getAbsolutePath());

No need to log that, but if you really want to make the message more descriptive ("Deleting temporary shared file" or something)

::: mobile/android/base/GeckoAppShell.java
@@ +1053,5 @@
>              subType = "*";
>          return type + "/" + subType;
>      }
>  
> +	static void shareImage(String aSrc, String aType) {

Fix indention

@@ +1057,5 @@
> +	static void shareImage(String aSrc, String aType) {
> +
> +        Intent intent = new Intent(Intent.ACTION_SEND);
> +
> +        boolean dataURI = aSrc.startsWith("data:");

Can you parse the URI and access the scheme instead? String stuff is yucky.

@@ +1061,5 @@
> +        boolean dataURI = aSrc.startsWith("data:");
> +
> +        // Make our directory structure for temporary files
> +        File sdcard = Environment.getExternalStorageDirectory();
> +        File dir = new File(sdcard.getAbsolutePath() + "/fennec");

You should add GeckoApp.getShareDirectory() or something since this code is duplicated in GeckoApp.

@@ +1071,5 @@
> +            for (File file : files) {
> +                Log.i(LOGTAG, "Deleting file " + file.getAbsolutePath());
> +                file.delete();
> +            }
> +        }

Same here, GeckoApp.deleteSharedFiles() or something like that

@@ +1075,5 @@
> +        }
> +
> +        try {
> +            // Create a temporary file for the image
> +            File imageFile = File.createTempFile(String.valueOf(System.currentTimeMillis()),

Do we really care what the filename is? Seems like a lot of work for no gain. I think we just need something like 'share' for prefix and 'jpg' (or whatever the extension of the original file is) for suffix.

@@ +1078,5 @@
> +            // Create a temporary file for the image
> +            File imageFile = File.createTempFile(String.valueOf(System.currentTimeMillis()),
> +                                                 "." + aType.replace("image/",""),
> +                                                 dir);
> +            OutputStream os = new FileOutputStream(imageFile);

Need to close this and the InputStream in a finally clause

@@ +1088,5 @@
> +                os.write(buf);
> +
> +            } else {
> +                // We are dealing with a URL
> +			    URL url = new URL(aSrc);

Fix indention

@@ +1104,5 @@
> +
> +            os.close();
> +            intent.putExtra(Intent.EXTRA_STREAM, Uri.fromFile(imageFile));
> +
> +            if (aType.startsWith("image/")) {

What's going on here? Do we get a bogus mime type sometimes?

@@ +1118,5 @@
> +               intent.setType("text/plain");
> +            } else {
> +               // Don't fail silently, tell the user that we weren't able to share the image
> +               Toast toast = Toast.makeText(GeckoApp.mAppContext,
> +                                            GeckoApp.mAppContext.getResources().getString(R.string.share_image_failed),

Fix indention

@@ +1126,5 @@
> +            }
> +        }
> +
> +        GeckoApp.mAppContext.startActivity(Intent.createChooser(intent,
> +                                           GeckoApp.mAppContext.getResources().getString(R.string.share_title)));

Fix indention

::: mobile/android/chrome/content/browser.js
@@ +1129,5 @@
>                 function(aTarget) {
>                   aTarget.mozRequestFullScreen();
>                 });
>  
> +	  this.add(Strings.browser.GetStringFromName("contextmenu.shareImage"),

Fix indention here and below

@@ +1158,5 @@
>                   let props = imageCache.findEntryProperties(aTarget.currentURI, aTarget.ownerDocument.characterSet);
>                   let contentDisposition = "";
>                   let type = "";
>                   try {
> +                    contentDisposition = String(props.get("content-disposition", Ci.nsISupportsCString));

Looks like changes leftover from debugging, should put it back

Comment 4

[Arkady Blyakher [:rkd]]

Attachment: share images with long press

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 640713 [details] [diff] [review]
share images with long press

Review of attachment 640713 [details] [diff] [review]:
-----------------------------------------------------------------

Much better, still one or two small things. Fix those and we're good.

::: mobile/android/base/GeckoApp.java
@@ +2219,5 @@
>          // etc., and generally mark the profile as 'clean', and then
>          // dirty it again if we get an onResume.
>  
>          GeckoAppShell.sendEventToGecko(GeckoEvent.createStoppingEvent(isApplicationInBackground()));
> +

Stray whitespace

@@ +2289,3 @@
>          GeckoAppShell.unregisterGeckoEventListener("Sanitize:ClearHistory", GeckoApp.mAppContext);
>  
> +        deleteTempFiles();

We don't need to do this here, right? We should avoid doing I/O in onStop() since it can make things sluggish.

@@ +2317,5 @@
> +
> +    // Delete any files in our temporary directory
> +    public static void deleteTempFiles() {
> +        File[] files  = getTempDirectory().listFiles();
> +        if (files != null) {

if (files == null)
   return;

A little easier to read that way

::: mobile/android/base/GeckoAppShell.java
@@ +1062,5 @@
> +                stream.close();
> +        }catch (IOException e) {}
> +    }
> +
> +	static void shareImage(String aSrc, String aType) {

Fix indention

@@ +1066,5 @@
> +	static void shareImage(String aSrc, String aType) {
> +
> +        Intent intent = new Intent(Intent.ACTION_SEND);
> +
> +        boolean dataURI = aSrc.startsWith("data:");

isDataURI is more clear, otherwise it looks like you are holding the actual URI there.

@@ +1089,5 @@
> +            } else {
> +                // We are dealing with a URL
> +                InputStream is = null;
> +                try {
> +			        URL url = new URL(aSrc);

Fix indention

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +104,5 @@
>  <!ENTITY char_encoding "Character Encoding">
>  
>  <!ENTITY share "Share">
>  <!ENTITY share_title "Share via">
> +<!ENTITY share_image_failed "Unable to share this image">

This seems ok, but get a string from madhava or ibarlow

Comment 6

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

Comment on attachment 640713 [details] [diff] [review]
share images with long press

Drive by


>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>             } else if (event.equals("Share:Text")) {
>                 String text = message.getString("text");
>                 GeckoAppShell.openUriExternal(text, "text/plain", "", "", Intent.ACTION_SEND, "");
>+            } else if (event.equals("Share:Image")) {
>+                String src = message.getString("url");
>+                String type = message.getString("mime");
>+                GeckoAppShell.shareImage(src, type);

Can we not use (or modify) nsIExternalSharingAppService?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1115

(I will ask the same of "Share:Text" too)

>+    // Get/Create a temporary direcory
>+    public static File getTempDirectory() {
>+        File sdcard = Environment.getExternalStorageDirectory();
>+        File dir = new File(sdcard.getAbsolutePath() + "/firefox");

We can't hardcode this. Don't we already have a tmp folder? If you do need to make a folder like this, use the package name, which would equate to something like "org.mozilla.fennec*" or "org.mozilla.firefox*"

But try to use the tmp folder, or maybe the Android cache folder

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>+    static void close(Closeable stream) {
>+        try {
>+            if (stream != null)
>+                stream.close();
>+        }catch (IOException e) {}
>+    }

Too generic of a function. Rename to safeCloseStream ?

>+
>+	static void shareImage(String aSrc, String aType) {

Indent is off in this function

>+
>+        Intent intent = new Intent(Intent.ACTION_SEND);

Remove the initial blank line

>+                os.write(buf);
>+
>+            } else {

Remove the blank line

>+                // We are dealing with a URL
>+                InputStream is = null;
>+                try {
>+			        URL url = new URL(aSrc);
>+                    is = url.openStream();
>+                    byte[] buf = new byte[2048];
>+                    int length;
>+
>+                    while ((length = is.read(buf)) != -1) {
>+                        os.write(buf, 0, length);
>+                    }
>+                } finally {
>+                    close(is);
>+                }

Make sure this code handles the problem in Android where downloading images sometimes fails. I think you are doing it right.
Although, downloading the image seems like a waste. Can't we grab it from cache or fail?

>+        GeckoApp.mAppContext.startActivity(Intent.createChooser(intent,
>+                GeckoApp.mAppContext.getResources().getString(R.string.share_title)));
>+
>+
>+    }

Remove the unneeded blank lines

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY share_image_failed "Unable to share this image">

Maybe we could get away with a generic "share fail" type of string

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+      this.add(Strings.browser.GetStringFromName("contextmenu.shareImage"),

>+                 sendMessageToJava({
>+                   gecko: {
>+                     type: "Share:Image",
>+                     url: src,
>+                     mime: type,
>+                   }
>+                 });

Does the imageSaveableContext make sure the image is in the cache?

Comment 7

[Arkady Blyakher [:rkd]]

Attachment: share images with long press

@@ +2317,5 @@
> +
> +    // Delete any files in our temporary directory
> +    public static void deleteTempFiles() {

This is in onDestroy(), not onStop() so it only affects the shutdown performance and isn't called regularly. ICS keeps apps running in the background, so this doesn't get called that often.


Can we not use (or modify) nsIExternalSharingAppService?

But try to use the tmp folder, or maybe the Android cache folder

These two are related. Android is annoying in that it needs images saved to disk before it can share them, so nsIExternalSharingAppService doesn't look like it will work. However, Android also doesn't let apps share data unless that data is in public storage. The temp that we currently use is associated with the application context, so it's not publicly accessible to other apps. Ditto on Android's cache directory.

Make sure this code handles the problem in Android where downloading images sometimes fails. I think you are doing it right.
Although, downloading the image seems like a waste. Can't we grab it from cache or fail?
We have to download the image because although Android puts an EXTRA_STREAM into the intent for ACTION_SEND, the stream must come from a URI of a file on disk. I'm not a fan of this either, but it looks like it's the only thing that will work.
Right now we have the cases that an image's source is either a url or a data uri. In the case of the former, this patch just shares the image url if sharing the binary data failed. With failure in the latter, I pop up a toast telling the user that we couldn't share this image so that we don't fail silently. 

Maybe we could get away with a generic "share fail" type of string
- I'm definitely open to this idea

Does the imageSaveableContext make sure the image is in the cache?
No, I use this similarly to image saving; I get file type information from the cache.

Comment 8

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c681f745658

Comment 9

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

Comment on attachment 640774 [details] [diff] [review]
share images with long press


>+    public static File getTempDirectory() {
>+        File sdcard = Environment.getExternalStorageDirectory();
>+        File dir = new File(sdcard.getAbsolutePath() + "/firefox");

Don't use "/firefox". Use getPackagename()

          File dir = new File(sdcard.getAbsolutePath(), getPackageName());

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c681f745658

Comment 11

[Cristian Nicolae (:xti)]

Share Image option is available in Context Menu when long tapping on a image. Closing bug as verified fixed on:

Firefox for Android
Version: 21.0a1 (2013-01-29)
Device: Galaxy R
OS: Android 2.3.4