Resolve TODOs in SDK

Author: lfkellogg

firebase-appdistribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

@@ -15,8 +15,8 @@
 package com.google.firebase.appdistribution;
 
 import android.net.Uri;
-
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import com.google.android.gms.tasks.Task;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.appdistribution.internal.FirebaseAppDistributionProxy;
@@ -143,9 +143,41 @@ public interface FirebaseAppDistribution {
    */
   void startFeedback(@NonNull CharSequence infoText);
 
+  /**
+   * Starts an activity to collect and submit feedback from the tester, along with the given
+   * screenshot.
+   *
+   * <p>Performs the following actions:
+   *
+   * <ol>
+   *   <li>If tester is not signed in, presents the tester with a Google Sign-in UI
+   *   <li>Starts a full screen activity for the tester to compose and submit the feedback
+   * </ol>
+   *
+   * @param infoTextResourceId string resource ID of text to display to the tester before collecting
+   *     feedback data (e.g. Terms and Conditions)
+   * @param screenshot URI to a bitmap containing a screenshot that will be included with the
+   *     report, or null to not include a screenshot
+   */
+  void startFeedback(@NonNull int infoTextResourceId, @Nullable Uri screenshot);
 
-  // TODO: javadoc and infoTextResourceId version
-  void startFeedback(@NonNull CharSequence infoText, @NonNull Uri screenshotUri);
+  /**
+   * Starts an activity to collect and submit feedback from the tester, along with the given
+   * screenshot.
+   *
+   * <p>Performs the following actions:
+   *
+   * <ol>
+   *   <li>If tester is not signed in, presents the tester with a Google Sign-in UI
+   *   <li>Starts a full screen activity for the tester to compose and submit the feedback
+   * </ol>
+   *
+   * @param infoText text to display to the tester before collecting feedback data (e.g. Terms and
+   *     Conditions)
+   * @param screenshot URI to a bitmap containing a screenshot that will be included with the
+   *     report, or null to not include a screenshot
+   */
+  void startFeedback(@NonNull CharSequence infoText, @Nullable Uri screenshotUri);
 
   /** Gets the singleton {@link FirebaseAppDistribution} instance. */
   @NonNull

firebase-appdistribution-api/src/main/java/com/google/firebase/appdistribution/internal/FirebaseAppDistributionProxy.java

@@ -15,8 +15,8 @@
 package com.google.firebase.appdistribution.internal;
 
 import android.net.Uri;
-
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import com.google.android.gms.tasks.Task;
 import com.google.firebase.appdistribution.AppDistributionRelease;
 import com.google.firebase.appdistribution.FirebaseAppDistribution;
@@ -85,7 +85,12 @@ public void startFeedback(@NonNull CharSequence infoText) {
   }
 
   @Override
-  public void startFeedback(@NonNull CharSequence infoText, @NonNull Uri screenshotUri) {
+  public void startFeedback(@NonNull int infoTextResourceId, @Nullable Uri screenshotUri) {
+    delegate.startFeedback(infoTextResourceId, screenshotUri);
+  }
+
+  @Override
+  public void startFeedback(@NonNull CharSequence infoText, @Nullable Uri screenshotUri) {
     delegate.startFeedback(infoText, screenshotUri);
   }
 }

firebase-appdistribution-api/src/main/java/com/google/firebase/appdistribution/internal/FirebaseAppDistributionStub.java

@@ -16,7 +16,6 @@
 
 import android.app.Activity;
 import android.net.Uri;
-
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import com.google.android.gms.tasks.Continuation;
@@ -86,7 +85,12 @@ public void startFeedback(@NonNull CharSequence infoText) {
   }
 
   @Override
-  public void startFeedback(@NonNull CharSequence infoText, @NonNull Uri screenshotUri) {
+  public void startFeedback(@NonNull int infoTextResourceId, @Nullable Uri screenshotUri) {
+    return;
+  }
+
+  @Override
+  public void startFeedback(@NonNull CharSequence infoText, @Nullable Uri screenshotUri) {
     return;
   }
 

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FeedbackActivity.java

@@ -72,7 +72,6 @@ private void setupView() {
     submitButton.setOnClickListener(this::submitFeedback);
 
     try {
-      // TODO: set up loading state and wait for screenshot in the background
       Bitmap thumbnail = readThumbnail();
       ImageView screenshotImageView = this.findViewById(R.id.thumbnail);
       screenshotImageView.setImageBitmap(thumbnail);

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

@@ -318,18 +318,25 @@ public void startFeedback(int infoTextResourceId) {
 
   @Override
   public void startFeedback(@NonNull CharSequence infoText) {
-    // TODO: prevent feedback from starting when another is already in progress. That would allow us
-    //  to remove the de-duping code in the example trigger.
     screenshotTaker
         .takeScreenshot()
         .addOnFailureListener(
-            taskExecutor, e -> LogWrapper.getInstance().e("Failed to take screenshot", e))
+            taskExecutor,
+            e -> {
+              LogWrapper.getInstance().w("Failed to take screenshot for feedback", e);
+              startFeedback(infoText, null);
+            })
         .addOnSuccessListener(
             taskExecutor, screenshotUri -> startFeedback(infoText, screenshotUri));
   }
 
   @Override
-  public void startFeedback(@NonNull CharSequence infoText, @NonNull Uri screenshotUri) {
+  public void startFeedback(@NonNull int infoTextResourceId, @Nullable Uri screenshotUri) {
+    startFeedback(firebaseApp.getApplicationContext().getText(infoTextResourceId), screenshotUri);
+  }
+
+  @Override
+  public void startFeedback(@NonNull CharSequence infoText, @Nullable Uri screenshotUri) {
     testerSignInManager
         .signInTester()
         .addOnFailureListener(
@@ -346,13 +353,16 @@ public void startFeedback(@NonNull CharSequence infoText, @NonNull Uri screensho
   }
 
   private Task<Void> launchFeedbackActivity(
-      String releaseName, CharSequence infoText, Uri screenshotFilename) {
+      String releaseName, CharSequence infoText, @Nullable Uri screenshotUri) {
     return lifecycleNotifier.consumeForegroundActivity(
         activity -> {
+          LogWrapper.getInstance().i("Launching feedback activity");
           Intent intent = new Intent(activity, FeedbackActivity.class);
           intent.putExtra(RELEASE_NAME_EXTRA_KEY, releaseName);
           intent.putExtra(INFO_TEXT_EXTRA_KEY, infoText);
-          intent.putExtra(SCREENSHOT_URI_EXTRA_KEY, screenshotFilename.toString());
+          if (screenshotUri != null) {
+            intent.putExtra(SCREENSHOT_URI_EXTRA_KEY, screenshotUri.toString());
+          }
           activity.startActivity(intent);
         });
   }

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ImageUtils.java

@@ -18,11 +18,10 @@
 import android.graphics.Bitmap;
 import android.graphics.BitmapFactory;
 import android.net.Uri;
-import android.util.Log;
 import com.google.auto.value.AutoValue;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
-import java.io.FileNotFoundException;
+import java.io.IOException;
 import java.io.InputStream;
 
 public class ImageUtils {
@@ -65,42 +64,25 @@ public static Bitmap readScaledImage(
               "Tried to read image with bad dimensions: %dx%d", targetWidth, targetHeight),
           Status.UNKNOWN);
     }
-    ImageSize imageSize = ImageSize.read(waitForInputStream(contentResolver, uri));
-    LogWrapper.getInstance().d("Read screenshot image size: " + imageSize);
 
+    // Read the dimensions of the image first
+    ImageSize imageSize;
+    try (InputStream inputStream = contentResolver.openInputStream(uri)) {
+      imageSize = ImageSize.read(inputStream);
+      LogWrapper.getInstance().d("Read screenshot image size: " + imageSize);
+    } catch (IOException e) {
+      throw new FirebaseAppDistributionException(
+          String.format("Could not read screenshot size from URI %s", uri), Status.UNKNOWN, e);
+    }
+
+    // Read the actual image, scaled using the actual and target dimensions
     final BitmapFactory.Options options = new BitmapFactory.Options();
     options.inSampleSize =
         calculateInSampleSize(imageSize.width(), imageSize.height(), targetWidth, targetHeight);
     // Get a fresh input stream because we've exhausted the last one
-    return BitmapFactory.decodeStream(
-        waitForInputStream(contentResolver, uri), /* outPadding= */ null, options);
-  }
-
-  private static InputStream waitForInputStream(ContentResolver contentResolver, Uri uri)
-      throws FirebaseAppDistributionException {
-    LogWrapper.getInstance().d(TAG, "Trying to read screenshot from URI: " + uri);
-    for (int i = 0; i < MAX_IMAGE_READ_RETRIES; i++) {
-      try {
-        return getInputStream(contentResolver, uri);
-      } catch (IllegalStateException e) {
-        Log.d(TAG, "Screenshot at URI is still pending. Sleeping.", e);
-        try {
-          Thread.sleep(IMAGE_READ_RETRY_SLEEP_MS);
-        } catch (InterruptedException ex) {
-          throw new FirebaseAppDistributionException(
-              "Interrupted while waiting for screenshot to become available", Status.UNKNOWN);
-        }
-      }
-    }
-    throw new FirebaseAppDistributionException(
-        "Timed out waiting for screenshot to be readable.", Status.UNKNOWN);
-  }
-
-  private static InputStream getInputStream(ContentResolver contentResolver, Uri uri)
-      throws FirebaseAppDistributionException {
-    try {
-      return contentResolver.openInputStream(uri);
-    } catch (FileNotFoundException e) {
+    try (InputStream inputStream = contentResolver.openInputStream(uri)) {
+      return BitmapFactory.decodeStream(inputStream, /* outPadding= */ null, options);
+    } catch (IOException e) {
       throw new FirebaseAppDistributionException(
           String.format("Could not read screenshot from URI %s", uri), Status.UNKNOWN, e);
     }

firebase-appdistribution/test-app/src/main/java/com/googletest/firebase/appdistribution/testapp/FeedbackTriggers.java

@@ -22,6 +22,7 @@
 import android.database.ContentObserver;
 import android.database.Cursor;
 import android.net.Uri;
+import android.os.Build;
 import android.os.Handler;
 import android.provider.MediaStore;
 import android.provider.MediaStore.Images.Media;
@@ -37,8 +38,13 @@
 class FeedbackTriggers extends ContentObserver {
 
   private static final String TAG = "FeedbackTriggers";
+  private static final boolean SHOULD_CHECK_IF_PENDING = Build.VERSION.SDK_INT == 29;
   private static final String[] PROJECTION =
-      new String[] {MediaStore.Images.Media.DATA, MediaStore.Images.Media._ID};
+      SHOULD_CHECK_IF_PENDING
+          ? new String[] {MediaStore.Images.Media.DATA}
+          : new String[] {
+            MediaStore.Images.Media.DATA, android.provider.MediaStore.MediaColumns.IS_PENDING
+          };
   private final Set<String> seenImages = new HashSet<>();
 
   private final Context context;
@@ -115,12 +121,11 @@ private void maybeStartFeedbackForScreenshot(Uri uri) {
     try {
       cursor = context.getContentResolver().query(uri, PROJECTION, null, null, null);
       if (cursor != null && cursor.moveToFirst()) {
+        // TODO: check if it's pending
+        // (http://google3/lens/screenshots/demo/java/com/google/android/lensonscreenshots/ScreenshotDetector.java?l=184)
         String path = cursor.getString(cursor.getColumnIndexOrThrow(MediaStore.Images.Media.DATA));
-        Long id = cursor.getLong(cursor.getColumnIndexOrThrow(MediaStore.Images.Media._ID));
         Log.i(TAG, "Path: " + path);
         if (path.toLowerCase().contains("screenshot")) {
-          // TODO: since we have a file path here, should we just use File or path everywhere
-          // instead of content URI?
           FirebaseAppDistribution.getInstance().startFeedback(infoText, uri);
         }
       }