feat: share image directly via share menu item #35

avitaker · 2017-04-06T20:43:37Z

Solves issue #30.

Previously, there were permission problems from trying to share the image to image sharing apps, according to the issue tracker. The following changes were made:
FileProvider class is defined in AndroidManifest, along with the required meta-data and associated files (a string was added to strings.xml for the provider authority). This is apparently required for sharing streams in Android M and above.
Temporary file of the image is written to keep track of exact file location, as well as have a temporary cache of previously shared images. This is done in an AsyncTask that is defined at the bottom of SubjectFragment and executed in the updateShareIntent method.
In devices with Android M or above, explicit permission to write and read images is obtained if not already provided.
The FileProvider class is used to create the URI that will eventually be sent in the intent.
An intent flag is added for purposes of magic (I'm not really sure what it does, but it works).

In the limited use case of trying to share images, this code works. Additional changes might be required to distinguish text from image intents to share, because the code right now will set the share intent to an image type if mUriStandardRemote is not null.

Solves issue murraycu#30. Previously, there were permission problems from trying to share the image to image sharing apps, according to the issue tracker. The following changes were made: FileProvider class is defined in AndroidManifest, along with the required meta-data and associated files (a string was added to strings.xml for the provider authority). This is apparently required for sharing streams in Android M and above. Temporary file of the image is written to keep track of exact file location, as well as have a temporary cache of previously shared images. This is done in an AsyncTask that is defined at the bottom of SubjectFragment and executed in the updateShareIntent method. In devices with Android M or above, explicit permission to write and read images is obtained if not already provided. The FileProvider class is used to create the URI that will eventually be sent in the intent. An intent flag is added for purposes of magic (I'm not really sure what it does, but it works). In the limited use case of trying to share images, this code works. Additional changes might be required to distinguish text from image intents to share, because the code right now will set the share intent to an image type if mUriImageRemote is not null.

murraycu

Thanks very much. I have only minor comments. Then I'd like to put this in master.

murraycu · 2017-04-07T09:33:03Z

app/src/main/AndroidManifest.xml

@@ -139,6 +139,16 @@
            <meta-data android:name="android.content.SyncAdapter"
                android:resource="@xml/syncadapter" />
        </service>
+


Could this have a comment saying why it's here, please. For instance, saying that it's here for this particular feature, and what would happen if it wasn't here.

murraycu · 2017-04-07T09:35:29Z

app/app.iml

      <sourceFolder url="file://$MODULE_DIR$/src/androidTest/res" type="java-test-resource" />
      <sourceFolder url="file://$MODULE_DIR$/src/androidTest/resources" type="java-test-resource" />
      <sourceFolder url="file://$MODULE_DIR$/src/androidTest/assets" type="java-test-resource" />
      <sourceFolder url="file://$MODULE_DIR$/src/androidTest/aidl" isTestSource="true" />
      <sourceFolder url="file://$MODULE_DIR$/src/androidTest/java" isTestSource="true" />
      <sourceFolder url="file://$MODULE_DIR$/src/androidTest/rs" isTestSource="true" />
      <sourceFolder url="file://$MODULE_DIR$/src/androidTest/shaders" isTestSource="true" />
+      <sourceFolder url="file://$MODULE_DIR$/src/test/res" type="java-test-resource" />


Could you please remove these .iml file changes from the commit? I do update them every now and then, but I try not to pollute the real changes with these changes to generated files.

murraycu · 2017-04-07T09:36:08Z

app/src/main/java/com/murrayc/galaxyzoo/app/ClassifyFragment.java

@@ -24,6 +24,7 @@
 import android.database.Cursor;
 import android.net.Uri;
 import android.os.Bundle;
+import android.support.annotation.Nullable;


I doubt that this change needs to be in the commit.

murraycu · 2017-04-07T09:37:00Z

app/src/main/java/com/murrayc/galaxyzoo/app/SubjectFragment.java

+        }
+    }
+
+    private class GetImageBitmapAsyncTask extends AsyncTask<String,Integer,Uri> {


spaces after the commas would be nice.

Absolutely, I have added spaces

murraycu · 2017-04-07T09:38:32Z

app/src/main/java/com/murrayc/galaxyzoo/app/SubjectFragment.java


        mShareActionProvider.setShareIntent(shareIntent);
    }
+
+    private static final int REQUEST_EXTERNAL_STORAGE = 1;
+    private static String[] PERMISSIONS_STORAGE = {


Can you use the constants here instead of hard-coding these strings? This seems to be suitable: https://developer.android.com/reference/android/Manifest.permission.html#WRITE_EXTERNAL_STORAGE

murraycu · 2017-04-07T09:42:25Z

app/src/main/java/com/murrayc/galaxyzoo/app/SubjectFragment.java

+            };
+            getImageBitmapAsyncTask.execute(mUriStandardRemote);
+        }
+

        mShareActionProvider.setShareIntent(shareIntent);


Do we still need this (now second) setShareIntent()? Maybe the whole instantiation of the shareIntent should move into onPostExecute()?

The second setShareIntent is set after the AsyncTask has started executing, but before it is completed. When the AsyncTask completes, the share intent is updated with the image type and content URI. I had left it there to provide a temporary intent on slower devices or in edge cases where the AsyncTask doesn't complete before the user decides to share, but it can indeed be removed. I will remove it in the next commit.

I would still leave the instantiation outside onPostExecute, since that leaves less main thread work to be done after the AsyncTask is completed.

I would still leave the instantiation outside onPostExecute, since that leaves less main thread work to be done
after the AsyncTask is completed.

OK. That sounds reasonable, but it would be nice to have a comment explaining that choice.

Solves issue murraycu#30. Now added some documentation, removed some generated files and hardcoded strings. Previously, there were permission problems from trying to share the image to image sharing apps, according to the issue tracker. The following changes were made: FileProvider class is defined in AndroidManifest, along with the required meta-data and associated files (a string was added to strings.xml for the provider authority). This is apparently required for sharing streams in Android M and above. Temporary file of the image is written to keep track of exact file location, as well as have a temporary cache of previously shared images. This is done in an AsyncTask that is defined at the bottom of SubjectFragment and executed in the updateShareIntent method. In devices with Android M or above, explicit permission to write and read images is obtained if not already provided. The FileProvider class is used to create the URI that will eventually be sent in the intent. An intent flag is added for purposes of magic (I'm not really sure what it does, but it works). In the limited use case of trying to share images, this code works. Additional changes might be required to distinguish text from image intents to share, because the code right now will set the share intent to an image type if mUriImageRemote is not null.

murraycu · 2017-04-09T20:50:50Z

Thanks. Could you squash these commits into one, please? It looks great.

Solves issue murraycu#30. Now added some documentation, removed some generated files and hardcoded strings. Previously, there were permission problems from trying to share the image to image sharing apps, according to the issue tracker. The following changes were made: FileProvider class is defined in AndroidManifest, along with the required meta-data and associated files (a string was added to strings.xml for the provider authority). This is apparently required for sharing streams in Android M and above. Temporary file of the image is written to keep track of exact file location, as well as have a temporary cache of previously shared images. This is done in an AsyncTask that is defined at the bottom of SubjectFragment and executed in the updateShareIntent method. In devices with Android M or above, explicit permission to write and read images is obtained if not already provided. The FileProvider class is used to create the URI that will eventually be sent in the intent. An intent flag is added for purposes of magic (I'm not really sure what it does, but it works). In the limited use case of trying to share images, this code works. Additional changes might be required to distinguish text from image intents to share, because the code right now will set the share intent to an image type if mUriImageRemote is not null.

murraycu · 2017-04-16T19:10:26Z

Is #36 the new version of this?

avitaker · 2017-04-18T14:19:06Z

Yes, #36 is the new version of this. I fixed the issues you brought up on that one

murraycu · 2017-04-18T16:55:26Z

OK. Next time, you don't need to create a new issue. You can just force push your changes to your existing branch and they will show up in the existing issue. At least that's how I usually do it.

murraycu requested changes Apr 7, 2017

View reviewed changes

avitaker closed this Apr 9, 2017

avitaker reopened this Apr 9, 2017

avitaker closed this Apr 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: share image directly via share menu item #35

feat: share image directly via share menu item #35

avitaker commented Apr 6, 2017 •

edited

Loading

murraycu left a comment

murraycu Apr 7, 2017

avitaker Apr 7, 2017

murraycu Apr 7, 2017

murraycu Apr 7, 2017

murraycu Apr 7, 2017

avitaker Apr 7, 2017

murraycu Apr 7, 2017

avitaker Apr 7, 2017

murraycu Apr 7, 2017

avitaker Apr 7, 2017

murraycu Apr 9, 2017

murraycu commented Apr 9, 2017

murraycu commented Apr 16, 2017

avitaker commented Apr 18, 2017

murraycu commented Apr 18, 2017

feat: share image directly via share menu item #35

feat: share image directly via share menu item #35

Conversation

avitaker commented Apr 6, 2017 • edited Loading

murraycu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murraycu commented Apr 9, 2017

murraycu commented Apr 16, 2017

avitaker commented Apr 18, 2017

murraycu commented Apr 18, 2017

avitaker commented Apr 6, 2017 •

edited

Loading