Closed Bug 886996 Opened 11 years ago Closed 11 years ago

Use an intent to set wallpaper

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(fennec24+)

RESOLVED FIXED
Firefox 25
Tracking Status
fennec 24+ ---

People

(Reporter: wesj, Assigned: shilpanbhagat)

References

Details

Attachments

(1 file, 5 obsolete files)

James Hugman showed me that we don't actually need a permission to set your phone wallpaper. We should do that instead. One less permission. We get a nice cropping tool. You can use it to set contact photos. Example code:

Intent intent = new Intent(Intent.ACTION_ATTACH_DATA);
intent.addCategory(Intent.CATEGORY_DEFAULT);
intent.setDataAndType(IOUtils.uriFromFilename(this, filename, "image/png");
intent.putExtra("mimeType", "image/png");
this.startActivity(Intent.createChooser(intent, this.getText(R.string.menu_set_as)));
tracking-fennec: --- → ?
tracking-fennec: ? → 24+
Assignee: nobody → sbhagat
Permissions for setting wallpaper now removed. Using a chooser intent to do the same.
Attachment #774311 - Flags: review?(margaret.leibovic)
Comment on attachment 774311 [details] [diff] [review]
Patch: Wallpaper can now be set without permissions

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

I just tested a build with this patch, and I got a system notification that my wallpaper was being updated, but I was given a prompt to choose which application I wanted to use to launch a new activity. It looks like you'll need to go in and rip out a lot more of setImageAsWallpaper (such as that notification), since it's not needed anymore. I also don't think we'll need the resizing logic, since the wallpaper activity lets you resize the image yourself. "Set as Wallpaper" might not be the right string for the context menu item anymore either, since it now gives you other options, such as setting the image as a contact photo.

::: mobile/android/base/AndroidManifest.xml.in
@@ -29,5 @@
>      <uses-permission android:name="android.permission.WAKE_LOCK"/>
>      <uses-permission android:name="android.permission.VIBRATE"/>
> -#ifdef MOZ_ANDROID_WALLPAPER
> -    <uses-permission android:name="android.permission.SET_WALLPAPER"/>
> -#endif

Since we won't need to disable this feature anymore, you should remove the logic for the MOZ_ANDROID_WALLPAPER flag:
http://mxr.mozilla.org/mozilla-central/search?string=MOZ_ANDROID_WALLPAPER
Attachment #774311 - Flags: review?(margaret.leibovic) → review-
Based on Margaret's comments, this patch makes the setting of wallpaper synchronous. Removed prompting and the MOZ_ANDROID_WALLPAPER flag
Attachment #774311 - Attachment is obsolete: true
Attachment #777366 - Flags: review?(margaret.leibovic)
Comment on attachment 777366 [details] [diff] [review]
Patch: Wallpaper can now be set without permissions

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

This is looking better, but I want to hold off on an r+ until we get some feedback about the strings from ibarlow.

You should also remove this line:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/confvars.sh#23

::: mobile/android/base/GeckoApp.java
@@ +929,5 @@
>              }
>          });
>      }
>  
>      private void setImageAsWallpaper(final String aSrc) {

If you like, you could take this opportunity to add a comment at the top of this method explaining what it's doing :)

@@ +934,5 @@
> +        boolean isDataURI = aSrc.startsWith("data:");
> +        Bitmap image = null;
> +        InputStream is = null;
> +        ByteArrayOutputStream os = null;
> +        try{

Nit: Space between try and open brace (I know this is just something you're moving over, but might as well fix it while you're touching it).

@@ +955,4 @@
>                  }
> +                byte[] imgBuffer = os.toByteArray();
> +                image = BitmapUtils.decodeByteArray(imgBuffer);
> +            }

I really wish there was a way to avoid downloading the image before showing the activity chooser. I wonder if there's any way we could get the image out of the gecko image cache, but maybe that's something for a follow-up bug (we'd need to talk to some platform folks).

@@ +956,5 @@
> +                byte[] imgBuffer = os.toByteArray();
> +                image = BitmapUtils.decodeByteArray(imgBuffer);
> +            }
> +            if(image != null) {
> +                String path = Media.insertImage(sAppContext.getContentResolver(), image, sAppContext.getString(R.string.wallpaper_title), sAppContext.getString(R.string.wallpaper_description));

Are these strings visible anywhere? Why did you choose this Media API? Does this method permanently store this image on the user's device (I think we should avoid doing that)?

@@ +959,5 @@
> +            if(image != null) {
> +                String path = Media.insertImage(sAppContext.getContentResolver(), image, sAppContext.getString(R.string.wallpaper_title), sAppContext.getString(R.string.wallpaper_description));
> +                Intent intent = new Intent(Intent.ACTION_ATTACH_DATA);
> +                intent.addCategory(Intent.CATEGORY_DEFAULT);
> +                intent.setDataAndType(Uri.parse(path), "image/bmp");

Is there a reason you need to use setDataAndType as opposed to just setData? The docs say setDataAndType should be rarely used:
https://developer.android.com/reference/android/content/Intent.html#setDataAndType%28android.net.Uri,%20java.lang.String%29

@@ +969,5 @@
> +            Log.e(LOGTAG, "Out of Memory when converting to byte array", ome);
> +        } catch(IOException ioe) {
> +            Log.e(LOGTAG, "I/O Exception while setting wallpaper", ioe);
> +        } finally {
> +            if(is != null) {

Nit: Space between if and open paren.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +207,4 @@
>  <!ENTITY wallpaper_fail "Unable to set wallpaper">
> +<!ENTITY wallpaper_title "Wallpaper">
> +<!ENTITY wallpaper_description "Wallpaper Image">
> +<!ENTITY wallpaper_chooser "Set image as">

We should also update the context menu item string to match this. And we should get feedback from ibarlow on these strings.

::: mobile/android/chrome/content/browser.js
@@ +579,1 @@
>      NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.setWallpaper"),

This is the other string we should update.
Attachment #777366 - Flags: review?(margaret.leibovic) → feedback+
OS: Linux → Android
Hardware: x86_64 → ARM
Can someone make a build of this I could try? I don't really understand what's being proposed here.
(In reply to Ian Barlow (:ibarlow) from comment #5)
> Can someone make a build of this I could try? I don't really understand
> what's being proposed here.

To explain a bit more, what this is doing is replacing the "Set as Wallpaper" context menu item with a "Set image as" item. Instead of processing the image and setting the wallpaper in the background (using system notifications/toasts to inform the user of this), this new item launches an intent that opens a system chooser to let the user decide what app they want to use to handle this.

We need help with these questions:

1) What strings should we use?
2) Are we okay with changing this from a background task to a user-blocking task?

Personally, I think this new approach is cool because it's more powerful than just setting the wallpaper.
Hey that is pretty cool!

1. Perhaps simply "Set image as" in the context menu, to be consistent with other parts of Android. Gallery uses "Set picture as", for example. 
2. I'm ok with it, for the same reason as Margaret, that it provides some better customization options for cropping and the like.
Comment on attachment 777366 [details] [diff] [review]
Patch: Wallpaper can now be set without permissions

drive-by

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

>     private void setImageAsWallpaper(final String aSrc) {

>+        ByteArrayOutputStream os = null;
>+        try{

try {

>+            if (isDataURI) {
>+                int dataStart = aSrc.indexOf(',');
>+                byte[] buf = Base64.decode(aSrc.substring(dataStart+1), Base64.DEFAULT);
>+                image = BitmapUtils.decodeByteArray(buf);

Someday we should add a BitmapUtils.decodeFromData (or something) and make it handle other data: formats

>+                // Cannot read from same stream twice. Also, InputStream from
>+                // URL does not support reset. So converting to byte array
>+
>+                while((byteRead = is.read(buf)) != -1) {

while (

>+                image = BitmapUtils.decodeByteArray(imgBuffer);
>+            }
>+            if(image != null) {

if (

>+        } finally {
>+            if(is != null) {

if (


>+            if(os != null) {
if (
(In reply to Ian Barlow (:ibarlow) from comment #8)

> 1. Perhaps simply "Set image as" in the context menu, to be consistent with
> other parts of Android. Gallery uses "Set picture as", for example. 

"Set Image As" until we change them all

> 2. I'm ok with it, for the same reason as Margaret, that it provides some
> better customization options for cropping and the like.

Me too
Updated patch based on the comments above. I guess we should push a patch without the using the image cache from gecko since that would require some platform level changes. I am currently working on that. But this patch does not have to wait for that.
Attachment #777366 - Attachment is obsolete: true
Attachment #784868 - Flags: review?(margaret.leibovic)
This is an optional, additional, WIP patch. It tries to use the image cache from gecko to load the image. Since we are loading things synchronously, it's best to have things run faster. This is a step in that direction.
Attachment #784920 - Flags: feedback?(wjohnston)
Left in some redundant stuff, cleaning up.
Attachment #784920 - Attachment is obsolete: true
Attachment #784920 - Flags: feedback?(wjohnston)
Attachment #784923 - Flags: feedback?(wjohnston)
Comment on attachment 784868 [details] [diff] [review]
Patch: Wallpaper can now be set without permissions

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

Looking good, but we need to make some changes before we can land this.

You need to update testWebContentContextMenu to test for the new string, and you should probably push to try before landing this. But let's do that soon so that we can try to push this patch to fx-team before the merge!

::: mobile/android/base/GeckoApp.java
@@ +973,1 @@
>      private void setImageAsWallpaper(final String aSrc) {

Let's file a follow-up bug to update the message/method name to be "Image:Set" and setImageAs() (or something like that), since this isn't about only wallpaper anymore.

That could be a good mentor bug.

@@ +976,5 @@
> +        InputStream is = null;
> +        ByteArrayOutputStream os = null;
> +        try {
> +            if (isDataURI) {
> +                int dataStart = aSrc.indexOf(',');

Nit: Use double quotes.

@@ +989,3 @@
>  
> +                // Cannot read from same stream twice. Also, InputStream from
> +                // URL does not support reset. So converting to byte array

Nit: Add a period at the end of the sentence.

@@ +1004,2 @@
>  
> +                //Removes the image from storage once the chooser activity ends.

Nit: Space after "//"

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +204,1 @@
>  <!ENTITY wallpaper_fail "Unable to set wallpaper">

This string actually doesn't make sense anymore :( We should have caught this as something to ask for feedback about.

Let's try to get ibarlow's help today, but if he's not around, I think we should just update this to "Unable to set image". This toast shouldn't appear very often, so I don't think we need to spend too much time debating the string.

@@ +204,2 @@
>  <!ENTITY wallpaper_fail "Unable to set wallpaper">
> +<!ENTITY wallpaper_chooser "Set Image as">

"Set Image As", with a capital A.

Also, update this entity name to set_image_chooser_title, since it's not about wallpaper anymore. Also, please add a localization comment about where this is used (as the title of the chooser intent).

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +166,5 @@
>  contextmenu.copyImageLocation=Copy Image Location
>  contextmenu.shareImage=Share Image
>  contextmenu.search=%S Search
>  contextmenu.saveImage=Save Image
> +contextmenu.setWallpaper=Set Image As

You need to rev the entity name, but I think you should change it anyway to match the new string. Let's name it contextmenu.setImageAs.
Attachment #784868 - Flags: review?(margaret.leibovic) → feedback+
Comment on attachment 784923 [details] [diff] [review]
[WIP] Using image cache from gecko to set up wallpaper

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

Drive-by: Let's move this patch into a separate bug, since it's out of scope for the current bug.
Made changes based on the above comments.
Attachment #784868 - Attachment is obsolete: true
Attachment #784923 - Attachment is obsolete: true
Attachment #784923 - Flags: feedback?(wjohnston)
Attachment #785177 - Flags: review?(margaret.leibovic)
Comment on attachment 785177 [details] [diff] [review]
Patch: Wallpaper can now be set without permissions

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

Looks good, thanks. fx-team is closed right now, so you might as well push to try to make sure things are all good while we wait for it to re-open.

And remember to file a follow-up bug about renaming the "Wallpaper:Set" message. We should make that into a mentor bug.
Attachment #785177 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
Looking green enough on try, so I decided to push it :)

https://hg.mozilla.org/integration/fx-team/rev/54eaa5ca86e7

File that follow-up bug though!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54eaa5ca86e7
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Depends on: 1062904
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.