From 4ec75321262680dbb72e8629f1e9edbf9e644e69 Mon Sep 17 00:00:00 2001 From: Isira Seneviratne Date: Mon, 22 Jul 2024 09:41:07 +0530 Subject: [PATCH] Addressed code review comments --- .../fragments/detail/VideoDetailFragment.java | 1 + .../feed/notifications/NotificationHelper.kt | 3 +- .../org/schabi/newpipe/player/Player.java | 12 +++- .../settings/ContentSettingsFragment.java | 8 +-- .../external_communication/ShareUtils.java | 64 ++++++++++++------- .../schabi/newpipe/util/image/CoilHelper.kt | 7 +- 6 files changed, 60 insertions(+), 35 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java index 96523321b..11a315d69 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java @@ -1473,6 +1473,7 @@ public final class VideoDetailFragment CoilUtils.dispose(binding.detailThumbnailImageView); CoilUtils.dispose(binding.detailSubChannelThumbnailView); CoilUtils.dispose(binding.overlayThumbnail); + CoilUtils.dispose(binding.detailUploaderThumbnailView); binding.detailThumbnailImageView.setImageBitmap(null); binding.detailSubChannelThumbnailView.setImageBitmap(null); diff --git a/app/src/main/java/org/schabi/newpipe/local/feed/notifications/NotificationHelper.kt b/app/src/main/java/org/schabi/newpipe/local/feed/notifications/NotificationHelper.kt index 659088ef2..4f70cee50 100644 --- a/app/src/main/java/org/schabi/newpipe/local/feed/notifications/NotificationHelper.kt +++ b/app/src/main/java/org/schabi/newpipe/local/feed/notifications/NotificationHelper.kt @@ -76,8 +76,7 @@ class NotificationHelper(val context: Context) { summaryBuilder.setLargeIcon(avatarIcon) - // Show individual stream notifications, set channel icon only if there is actually - // one + // Show individual stream notifications, set channel icon only if there is actually one showStreamNotifications(newStreams, data.serviceId, avatarIcon) // Show summary notification manager.notify(data.pseudoId, summaryBuilder.build()) diff --git a/app/src/main/java/org/schabi/newpipe/player/Player.java b/app/src/main/java/org/schabi/newpipe/player/Player.java index 40da9139d..74d35cf31 100644 --- a/app/src/main/java/org/schabi/newpipe/player/Player.java +++ b/app/src/main/java/org/schabi/newpipe/player/Player.java @@ -192,6 +192,8 @@ public final class Player implements PlaybackListener, Listener { private MediaItemTag currentMetadata; @Nullable private Bitmap currentThumbnail; + @Nullable + private coil.request.Disposable thumbnailDisposable; /*////////////////////////////////////////////////////////////////////////// // Player @@ -772,6 +774,11 @@ public final class Player implements PlaybackListener, Listener { + thumbnails.size() + "]"); } + // Cancel any ongoing image loading + if (thumbnailDisposable != null) { + thumbnailDisposable.dispose(); + } + // Unset currentThumbnail, since it is now outdated. This ensures it is not used in media // session metadata while the new thumbnail is being loaded by Coil. onThumbnailLoaded(null); @@ -780,7 +787,7 @@ public final class Player implements PlaybackListener, Listener { } // scale down the notification thumbnail for performance - final var target = new Target() { + final var thumbnailTarget = new Target() { @Override public void onError(@Nullable final Drawable error) { Log.e(TAG, "Thumbnail - onError() called"); @@ -805,7 +812,8 @@ public final class Player implements PlaybackListener, Listener { result.getIntrinsicHeight(), null)); } }; - CoilHelper.INSTANCE.loadScaledDownThumbnail(context, thumbnails, target); + thumbnailDisposable = CoilHelper.INSTANCE + .loadScaledDownThumbnail(context, thumbnails, thumbnailTarget); } private void onThumbnailLoaded(@Nullable final Bitmap bitmap) { diff --git a/app/src/main/java/org/schabi/newpipe/settings/ContentSettingsFragment.java b/app/src/main/java/org/schabi/newpipe/settings/ContentSettingsFragment.java index 2cda1b4ea..c47abb930 100644 --- a/app/src/main/java/org/schabi/newpipe/settings/ContentSettingsFragment.java +++ b/app/src/main/java/org/schabi/newpipe/settings/ContentSettingsFragment.java @@ -42,12 +42,8 @@ public class ContentSettingsFragment extends BasePreferenceFragment { ImageStrategy.setPreferredImageQuality(PreferredImageQuality .fromPreferenceKey(requireContext(), (String) newValue)); final var loader = Coil.imageLoader(preference.getContext()); - if (loader.getMemoryCache() != null) { - loader.getMemoryCache().clear(); - } - if (loader.getDiskCache() != null) { - loader.getDiskCache().clear(); - } + loader.getMemoryCache().clear(); + loader.getDiskCache().clear(); Toast.makeText(preference.getContext(), R.string.thumbnail_cache_wipe_complete_notice, Toast.LENGTH_SHORT) .show(); diff --git a/app/src/main/java/org/schabi/newpipe/util/external_communication/ShareUtils.java b/app/src/main/java/org/schabi/newpipe/util/external_communication/ShareUtils.java index 21a4b1175..9008a213d 100644 --- a/app/src/main/java/org/schabi/newpipe/util/external_communication/ShareUtils.java +++ b/app/src/main/java/org/schabi/newpipe/util/external_communication/ShareUtils.java @@ -10,6 +10,7 @@ import android.content.Intent; import android.content.pm.PackageManager; import android.content.pm.ResolveInfo; import android.graphics.Bitmap; +import android.graphics.BitmapFactory; import android.net.Uri; import android.os.Build; import android.text.TextUtils; @@ -24,13 +25,16 @@ import androidx.core.content.FileProvider; import org.schabi.newpipe.BuildConfig; import org.schabi.newpipe.R; import org.schabi.newpipe.extractor.Image; -import org.schabi.newpipe.util.image.CoilHelper; import org.schabi.newpipe.util.image.ImageStrategy; -import java.io.File; -import java.io.FileOutputStream; +import java.nio.file.Files; +import java.util.Collections; import java.util.List; +import coil.Coil; +import coil.disk.DiskCache; +import coil.memory.MemoryCache; + public final class ShareUtils { private static final String TAG = ShareUtils.class.getSimpleName(); @@ -335,7 +339,13 @@ public final class ShareUtils { } /** - * Generate a {@link ClipData} with the image of the content shared. + * Generate a {@link ClipData} with the image of the content shared, if it's in the app cache. + * + *

+ * In order not to worry about network issues (timeouts, DNS issues, low connection speed, ...) + * when sharing a content, only images in the {@link MemoryCache} or {@link DiskCache} + * used by the Coil library are used as preview images. If the thumbnail image is not in the + * cache, no {@link ClipData} will be generated and {@code null} will be returned. * *

* In order to display the image in the content preview of the Android share sheet, an URI of @@ -351,11 +361,6 @@ public final class ShareUtils { *

* *

- * This method will call {@link CoilHelper#loadBitmapBlocking(Context, String)} to get the - * thumbnail of the content. - *

- * - *

* Using the result of this method when sharing has only an effect on the system share sheet (if * OEMs didn't change Android system standard behavior) on Android API 29 and higher. *

@@ -369,33 +374,46 @@ public final class ShareUtils { @NonNull final Context context, @NonNull final String thumbnailUrl) { try { - final var bitmap = CoilHelper.INSTANCE.loadBitmapBlocking(context, thumbnailUrl); - if (bitmap == null) { - return null; - } - // Save the image in memory to the application's cache because we need a URI to the // image to generate a ClipData which will show the share sheet, and so an image file final Context applicationContext = context.getApplicationContext(); - final String appFolder = applicationContext.getCacheDir().getAbsolutePath(); - final File thumbnailPreviewFile = new File(appFolder - + "/android_share_sheet_image_preview.jpg"); + final var loader = Coil.imageLoader(context); + final var value = loader.getMemoryCache() + .get(new MemoryCache.Key(thumbnailUrl, Collections.emptyMap())); - // Any existing file will be overwritten with FileOutputStream - final FileOutputStream fileOutputStream = new FileOutputStream(thumbnailPreviewFile); - bitmap.compress(Bitmap.CompressFormat.JPEG, 90, fileOutputStream); - fileOutputStream.close(); + final Bitmap cachedBitmap; + if (value != null) { + cachedBitmap = value.getBitmap(); + } else { + try (var snapshot = loader.getDiskCache().openSnapshot(thumbnailUrl)) { + if (snapshot != null) { + cachedBitmap = BitmapFactory.decodeFile(snapshot.getData().toString()); + } else { + cachedBitmap = null; + } + } + } + + if (cachedBitmap == null) { + return null; + } + + final var path = applicationContext.getCacheDir().toPath() + .resolve("android_share_sheet_image_preview.jpg"); + // Any existing file will be overwritten + try (var outputStream = Files.newOutputStream(path)) { + cachedBitmap.compress(Bitmap.CompressFormat.JPEG, 90, outputStream); + } final ClipData clipData = ClipData.newUri(applicationContext.getContentResolver(), "", FileProvider.getUriForFile(applicationContext, BuildConfig.APPLICATION_ID + ".provider", - thumbnailPreviewFile)); + path.toFile())); if (DEBUG) { Log.d(TAG, "ClipData successfully generated for Android share sheet: " + clipData); } return clipData; - } catch (final Exception e) { Log.w(TAG, "Error when setting preview image for share sheet", e); return null; diff --git a/app/src/main/java/org/schabi/newpipe/util/image/CoilHelper.kt b/app/src/main/java/org/schabi/newpipe/util/image/CoilHelper.kt index ab7cd79a8..2608090dc 100644 --- a/app/src/main/java/org/schabi/newpipe/util/image/CoilHelper.kt +++ b/app/src/main/java/org/schabi/newpipe/util/image/CoilHelper.kt @@ -8,6 +8,7 @@ import androidx.annotation.DrawableRes import androidx.core.graphics.drawable.toBitmapOrNull import coil.executeBlocking import coil.imageLoader +import coil.request.Disposable import coil.request.ImageRequest import coil.size.Size import coil.target.Target @@ -47,7 +48,7 @@ object CoilHelper { loadImageDefault(target, url, R.drawable.placeholder_thumbnail_video) } - fun loadScaledDownThumbnail(context: Context, images: List, target: Target) { + fun loadScaledDownThumbnail(context: Context, images: List, target: Target): Disposable { val url = ImageStrategy.choosePreferredImage(images) val request = getImageRequest(context, url, R.drawable.placeholder_thumbnail_video) .target(target) @@ -79,7 +80,7 @@ object CoilHelper { }) .build() - context.imageLoader.enqueue(request) + return context.imageLoader.enqueue(request) } fun loadDetailsThumbnail(target: ImageView, images: List) { @@ -133,6 +134,8 @@ object CoilHelper { return ImageRequest.Builder(context) .data(takenUrl) .error(placeholderResId) + .memoryCacheKey(takenUrl) + .diskCacheKey(takenUrl) .apply { if (takenUrl != null || showPlaceholderWhileLoading) { placeholder(placeholderResId)