Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thumbnail support #533

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
55a6929
Thumbail Generation study - inconsistent changes
JustFanta01 Apr 11, 2024
f1ca280
Added thumbnail File in CryptoFile and logic for storing and retrievi…
JustFanta01 Apr 13, 2024
ef39584
Remove thumbnail from cache, add check if thumbnail exists only for i…
taglioIsCoding Apr 13, 2024
53779c4
Generate thumbnails only for images
taglioIsCoding Apr 14, 2024
e69bb98
Removed unused imports
taglioIsCoding Apr 19, 2024
ce1c989
Add local LRU cache and first refactor
taglioIsCoding Apr 21, 2024
b30afb1
Removed unused imports + fix typo
JustFanta01 Apr 21, 2024
e0fa1be
changed names for retrieving cloud-related DiskLruCache
JustFanta01 Apr 28, 2024
b31de8e
modified the cachekey for DiskLruCache
JustFanta01 Apr 28, 2024
e20d516
Added also in FormatPre7 the fetch of the thumbnail (not tested yet)
JustFanta01 Apr 28, 2024
ca57efb
Add enum Thumbnail option
taglioIsCoding Apr 29, 2024
041b2d8
minor changes
JustFanta01 Apr 29, 2024
abcc3e2
Change cachekey for thumbnails
JustFanta01 May 7, 2024
d02e811
Use onEach and added the delete operation in the FormatPre7
JustFanta01 May 7, 2024
6daefd7
Added a separate thread to acquire the bitmap of the image and genera…
JustFanta01 Apr 28, 2024
4d0e715
Add thumbanil generator thread pool executor and subsample image stream
JustFanta01 May 1, 2024
fe0151d
Cleanup
taglioIsCoding May 8, 2024
00f766f
Manage rename and move files in cache
taglioIsCoding May 8, 2024
68b8744
Rebase and PR minor changes
JustFanta01 Sep 6, 2024
7110a54
Refactor thumbnail association
JustFanta01 Sep 7, 2024
e05f206
Force thumbnail generation to compare listing performance
JustFanta01 Sep 9, 2024
64ba7cf
Fix mistake
JustFanta01 Sep 10, 2024
911c196
Merge branch 'cryptomator:develop' into feature/thumbnail-playground
JustFanta01 Oct 8, 2024
c25714d
Add multithreaded parallelism in the thumbnail generation
JustFanta01 Sep 24, 2024
82525c0
Add automatic download and generation of thumbnails after scroll and …
JustFanta01 Oct 6, 2024
034a29b
Add AssociateThumbanilUseCase to fetch thumbnails from cache not in M…
JustFanta01 Oct 6, 2024
80c013e
Unify the readGenerateThumbnail() with the original read() and minor …
JustFanta01 Oct 6, 2024
932d7d2
Change thumbnail cachekey, add ThumbnailOption.READONLY and automatic…
JustFanta01 Oct 8, 2024
d85096b
Fix thumbnail settings and clean up
JustFanta01 Oct 12, 2024
a45edea
Minor fixes :)
JustFanta01 Oct 14, 2024
bdc28c2
Merge branch 'cryptomator:develop' into feature/thumbnail-playground
JustFanta01 Jan 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import org.cryptomator.domain.repository.CloudContentRepository
import org.cryptomator.domain.usecases.ProgressAware
import org.cryptomator.domain.usecases.cloud.DataSource
import org.cryptomator.domain.usecases.cloud.DownloadState
import org.cryptomator.domain.usecases.cloud.FileTransferState
import org.cryptomator.domain.usecases.cloud.UploadState
import java.io.File
import java.io.OutputStream
Expand Down Expand Up @@ -95,6 +96,11 @@ internal class CryptoCloudContentRepository(context: Context, cloudContentReposi
cryptoImpl.read(file, data, progressAware)
}

@Throws(BackendException::class)
override fun associateThumbnails(list: List<CryptoNode>, progressAware: ProgressAware<FileTransferState>) {
cryptoImpl.associateThumbnails(list, progressAware)
}

@Throws(BackendException::class)
override fun delete(node: CryptoNode) {
cryptoImpl.delete(node)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package org.cryptomator.data.cloud.crypto

import org.cryptomator.domain.Cloud
import org.cryptomator.domain.CloudFile
import java.io.File
import java.util.Date

class CryptoFile(
Expand All @@ -12,6 +13,8 @@ class CryptoFile(
val cloudFile: CloudFile
) : CloudFile, CryptoNode {

var thumbnail : File? = null

override val cloud: Cloud?
get() = parent.cloud

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package org.cryptomator.data.cloud.crypto

import android.content.Context
import android.graphics.Bitmap
import android.graphics.BitmapFactory
import android.media.ThumbnailUtils
import com.google.common.util.concurrent.ThreadFactoryBuilder
import com.tomclaw.cache.DiskLruCache
import org.cryptomator.cryptolib.api.Cryptor
import org.cryptomator.cryptolib.common.DecryptingReadableByteChannel
import org.cryptomator.cryptolib.common.EncryptingWritableByteChannel
Expand All @@ -9,6 +14,7 @@ import org.cryptomator.domain.Cloud
import org.cryptomator.domain.CloudFile
import org.cryptomator.domain.CloudFolder
import org.cryptomator.domain.CloudNode
import org.cryptomator.domain.CloudType
import org.cryptomator.domain.exception.BackendException
import org.cryptomator.domain.exception.CloudNodeAlreadyExistsException
import org.cryptomator.domain.exception.EmptyDirFileException
Expand All @@ -22,20 +28,36 @@ import org.cryptomator.domain.usecases.UploadFileReplacingProgressAware
import org.cryptomator.domain.usecases.cloud.DataSource
import org.cryptomator.domain.usecases.cloud.DownloadState
import org.cryptomator.domain.usecases.cloud.FileBasedDataSource.Companion.from
import org.cryptomator.domain.usecases.cloud.FileTransferState
import org.cryptomator.domain.usecases.cloud.Progress
import org.cryptomator.domain.usecases.cloud.UploadState
import org.cryptomator.util.SharedPreferencesHandler
import org.cryptomator.util.ThumbnailsOption
import org.cryptomator.util.file.LruFileCacheUtil
import org.cryptomator.util.file.MimeType
import org.cryptomator.util.file.MimeTypeMap
import org.cryptomator.util.file.MimeTypes
import java.io.ByteArrayOutputStream
import java.io.Closeable
import java.io.File
import java.io.FileInputStream
import java.io.FileOutputStream
import java.io.IOException
import java.io.OutputStream
import java.io.PipedInputStream
import java.io.PipedOutputStream
import java.nio.ByteBuffer
import java.nio.channels.Channels
import java.util.LinkedList
import java.util.Queue
import java.util.UUID
import java.util.concurrent.CompletableFuture
import java.util.concurrent.ExecutorService
import java.util.concurrent.Executors
import java.util.concurrent.Future
import java.util.function.Supplier
import kotlin.system.measureTimeMillis
import timber.log.Timber


abstract class CryptoImplDecorator(
Expand All @@ -50,6 +72,59 @@ abstract class CryptoImplDecorator(
@Volatile
private var root: RootCryptoFolder? = null

private val sharedPreferencesHandler = SharedPreferencesHandler(context)

private var diskLruCache: MutableMap<LruFileCacheUtil.Cache, DiskLruCache?> = mutableMapOf()

private val mimeTypes = MimeTypes(MimeTypeMap())

private val thumbnailExecutorService: ExecutorService by lazy {
val threadFactory = ThreadFactoryBuilder().setNameFormat("thumbnail-generation-thread-%d").build()
Executors.newCachedThreadPool(threadFactory)
}

protected fun getLruCacheFor(type: CloudType): DiskLruCache? {
return getOrCreateLruCache(getCacheTypeFromCloudType(type), sharedPreferencesHandler.lruCacheSize())
}

private fun getOrCreateLruCache(cache: LruFileCacheUtil.Cache, cacheSize: Int): DiskLruCache? {
return diskLruCache.computeIfAbsent(cache) {
val cacheFile = LruFileCacheUtil(context).resolve(it)
try {
DiskLruCache.create(cacheFile, cacheSize.toLong())
} catch (e: IOException) {
Timber.tag("CryptoImplDecorator").e(e, "Failed to setup LRU cache for $cacheFile.name")
null
}
}
}

protected fun renameFileInCache(source: CryptoFile, target: CryptoFile) {
val oldCacheKey = generateCacheKey(source)
val newCacheKey = generateCacheKey(target)
source.cloudFile.cloud?.type()?.let { cloudType ->
getLruCacheFor(cloudType)?.let { diskCache ->
if (diskCache[oldCacheKey] != null) {
target.thumbnail = diskCache.put(newCacheKey, diskCache[oldCacheKey])
diskCache.delete(oldCacheKey)
}
}
}
}

private fun getCacheTypeFromCloudType(type: CloudType): LruFileCacheUtil.Cache {
return when (type) {
CloudType.DROPBOX -> LruFileCacheUtil.Cache.DROPBOX
CloudType.GOOGLE_DRIVE -> LruFileCacheUtil.Cache.GOOGLE_DRIVE
CloudType.ONEDRIVE -> LruFileCacheUtil.Cache.ONEDRIVE
CloudType.PCLOUD -> LruFileCacheUtil.Cache.PCLOUD
CloudType.WEBDAV -> LruFileCacheUtil.Cache.WEBDAV
CloudType.S3 -> LruFileCacheUtil.Cache.S3
CloudType.LOCAL -> LruFileCacheUtil.Cache.LOCAL
else -> throw IllegalStateException()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Provide an exception message when throwing IllegalStateException

Throwing an IllegalStateException without a message can make debugging more difficult. Providing a descriptive message helps in identifying the issue quickly.

Apply this diff to include an exception message:

else -> throw IllegalStateException()
+ else -> throw IllegalStateException("Unexpected CloudType: $type")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
else -> throw IllegalStateException()
else -> throw IllegalStateException("Unexpected CloudType: $type")
🧰 Tools
🪛 detekt

[warning] 124-124: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.

(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)

}
}

@Throws(BackendException::class)
abstract fun folder(cryptoParent: CryptoFolder, cleartextName: String): CryptoFolder

Expand Down Expand Up @@ -309,8 +384,22 @@ abstract class CryptoImplDecorator(
@Throws(BackendException::class)
fun read(cryptoFile: CryptoFile, data: OutputStream, progressAware: ProgressAware<DownloadState>) {
val ciphertextFile = cryptoFile.cloudFile

val diskCache = cryptoFile.cloudFile.cloud?.type()?.let { getLruCacheFor(it) }
val cacheKey = generateCacheKey(cryptoFile)
val genThumbnail = isThumbnailGenerationAvailable(diskCache, cryptoFile.name)
var futureThumbnail: Future<*> = CompletableFuture.completedFuture(null)

val thumbnailWriter = PipedOutputStream()
val thumbnailReader = PipedInputStream(thumbnailWriter)

try {
val encryptedTmpFile = readToTmpFile(cryptoFile, ciphertextFile, progressAware)

if (genThumbnail) {
futureThumbnail = startThumbnailGeneratorThread(cryptoFile, diskCache!!, cacheKey, thumbnailReader)
}

progressAware.onProgress(Progress.started(DownloadState.decryption(cryptoFile)))
try {
Channels.newChannel(FileInputStream(encryptedTmpFile)).use { readableByteChannel ->
Expand All @@ -322,7 +411,12 @@ abstract class CryptoImplDecorator(
while (decryptingReadableByteChannel.read(buff).also { read = it } > 0) {
buff.flip()
data.write(buff.array(), 0, buff.remaining())
if (genThumbnail) {
thumbnailWriter.write(buff.array(), 0, buff.remaining())
}

decrypted += read.toLong()

progressAware
.onProgress(
Progress.progress(DownloadState.decryption(cryptoFile)) //
Expand All @@ -332,16 +426,115 @@ abstract class CryptoImplDecorator(
)
}
}
thumbnailWriter.flush()
closeQuietly(thumbnailWriter)
}
} finally {
encryptedTmpFile.delete()
if (genThumbnail) {
futureThumbnail.get()
}
progressAware.onProgress(Progress.completed(DownloadState.decryption(cryptoFile)))
}

closeQuietly(thumbnailReader)
} catch (e: IOException) {
throw FatalBackendException(e)
}
}

private fun closeQuietly(closeable: Closeable) {
try {
closeable.close();
} catch (e: IOException) {
// ignore
}
Comment on lines +449 to +451
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid swallowing exceptions without handling or logging them

Swallowing the IOException without handling or logging it can hide underlying issues and make debugging challenging. Consider logging the exception at a debug level to aid in troubleshooting.

Apply this diff to log the exception:

} catch (e: IOException) {
-    // ignore
+    Timber.d(e, "IOException occurred while closing Closeable")
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (e: IOException) {
// ignore
}
} catch (e: IOException) {
Timber.d(e, "IOException occurred while closing Closeable")
}
🧰 Tools
🪛 detekt

[warning] 449-449: The caught exception is swallowed. The original exception could be lost.

(detekt.exceptions.SwallowedException)

}
Comment on lines +446 to +452
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Log exceptions in closeQuietly method.

The closeQuietly method is a useful utility, but swallowing exceptions without logging them can hide potential issues and make debugging difficult.

Consider logging the exception at a debug level:

 private fun closeQuietly(closeable: Closeable) {
     try {
         closeable.close()
     } catch (e: IOException) {
-        // ignore
+        Timber.d(e, "IOException occurred while closing Closeable")
     }
 }

This change will help in identifying and troubleshooting any issues related to resource closing.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private fun closeQuietly(closeable: Closeable) {
try {
closeable.close();
} catch (e: IOException) {
// ignore
}
}
private fun closeQuietly(closeable: Closeable) {
try {
closeable.close();
} catch (e: IOException) {
Timber.d(e, "IOException occurred while closing Closeable")
}
}
🧰 Tools
🪛 detekt

[warning] 449-449: The caught exception is swallowed. The original exception could be lost.

(detekt.exceptions.SwallowedException)


private fun startThumbnailGeneratorThread(cryptoFile: CryptoFile, diskCache: DiskLruCache, cacheKey: String, thumbnailReader: PipedInputStream): Future<*> {
return thumbnailExecutorService.submit {
try {
val options = BitmapFactory.Options()
val thumbnailBitmap: Bitmap?
options.inSampleSize = 4 // pixel number reduced by a factor of 1/16
val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options)
val thumbnailWidth = 100
val thumbnailHeight = 100
thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight)
if (thumbnailBitmap != null) {
storeThumbnail(diskCache, cacheKey, thumbnailBitmap)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential null Bitmap from BitmapFactory.decodeStream

BitmapFactory.decodeStream can return null if the input stream cannot be decoded into a bitmap (e.g., unsupported formats like SVG). Not handling this case may lead to a NullPointerException when ThumbnailUtils.extractThumbnail is called.

Modify the code to check if bitmap is not null before proceeding:

val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options)
+ if (bitmap != null) {
    val thumbnailWidth = 100
    val thumbnailHeight = 100
    val thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight)
    if (thumbnailBitmap != null) {
        storeThumbnail(diskCache, cacheKey, thumbnailBitmap)
    }
+ } else {
+     Timber.e("Failed to decode bitmap from input stream for file: ${cryptoFile.name}")
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
options.inSampleSize = 4 // pixel number reduced by a factor of 1/16
val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options)
val thumbnailWidth = 100
val thumbnailHeight = 100
thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight)
if (thumbnailBitmap != null) {
storeThumbnail(diskCache, cacheKey, thumbnailBitmap)
}
options.inSampleSize = 4 // pixel number reduced by a factor of 1/16
val bitmap = BitmapFactory.decodeStream(thumbnailReader, null, options)
if (bitmap != null) {
val thumbnailWidth = 100
val thumbnailHeight = 100
thumbnailBitmap = ThumbnailUtils.extractThumbnail(bitmap, thumbnailWidth, thumbnailHeight)
if (thumbnailBitmap != null) {
storeThumbnail(diskCache, cacheKey, thumbnailBitmap)
}
} else {
Timber.e("Failed to decode bitmap from input stream for file: ${cryptoFile.name}")
}

closeQuietly(thumbnailReader)

cryptoFile.thumbnail = diskCache[cacheKey]
} catch (e: Exception) {
Timber.e("Bitmap generation crashed")
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Log exception details when catching exceptions

Catching exceptions without logging the exception details can make it difficult to diagnose problems. Logging the exception provides valuable information for debugging.

Apply this diff to include the exception in the log:

} catch (e: Exception) {
-    Timber.e("Bitmap generation crashed")
+    Timber.e(e, "Bitmap generation crashed")
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Timber.e("Bitmap generation crashed")
}
}
Timber.e(e, "Bitmap generation crashed")
}
}

}

protected fun generateCacheKey(cryptoFile: CryptoFile): String {
return String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode())
}
Comment on lines +481 to +483
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use explicit locale in generateCacheKey method.

The generateCacheKey method effectively generates a unique key for caching thumbnails. However, using the default locale in String.format can lead to inconsistencies across different devices.

To ensure consistent behavior across all devices, specify an explicit locale:

 protected fun generateCacheKey(cryptoFile: CryptoFile): String {
-    return String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode())
+    return String.format(Locale.US, "%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode())
 }

This change ensures that the cache key generation is consistent regardless of the device's locale settings.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
protected fun generateCacheKey(cryptoFile: CryptoFile): String {
return String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode())
}
protected fun generateCacheKey(cryptoFile: CryptoFile): String {
return String.format(Locale.US, "%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode())
}
🧰 Tools
🪛 detekt

[warning] 482-482: String.format("%s-%d", cryptoFile.cloudFile.cloud?.id() ?: "common", cryptoFile.path.hashCode()) uses implicitly default locale for string formatting.

(detekt.potential-bugs.ImplicitDefaultLocale)


private fun isThumbnailGenerationAvailable(cache: DiskLruCache?, fileName: String): Boolean {
return isGenerateThumbnailsEnabled() && sharedPreferencesHandler.generateThumbnails() != ThumbnailsOption.READONLY && cache != null && isImageMediaType(fileName)
}

fun associateThumbnails(list: List<CryptoNode>, progressAware: ProgressAware<FileTransferState>) {
if (!isGenerateThumbnailsEnabled()) {
return
}
val cryptoFileList = list.filterIsInstance<CryptoFile>()
if (cryptoFileList.isEmpty()) {
return
}
val firstCryptoFile = cryptoFileList[0]
val cloudType = (firstCryptoFile).cloudFile.cloud?.type() ?: return
val diskCache = getLruCacheFor(cloudType) ?: return
val toProcess = cryptoFileList.filter { cryptoFile ->
(isImageMediaType(cryptoFile.name) && cryptoFile.thumbnail == null)
}
var associated = 0
val elapsed = measureTimeMillis {
toProcess.forEach { cryptoFile ->
val cacheKey = generateCacheKey(cryptoFile)
val cacheFile = diskCache[cacheKey]
if (cacheFile != null && cryptoFile.thumbnail == null) {
cryptoFile.thumbnail = cacheFile
associated++
val state = FileTransferState { cryptoFile }
val progress = Progress.progress(state).thatIsCompleted()
progressAware.onProgress(progress)
}
}
}
Timber.tag("THUMBNAIL").i("[AssociateThumbnails] associated:${associated} files, elapsed:${elapsed}ms")
}

private fun isGenerateThumbnailsEnabled(): Boolean {
return sharedPreferencesHandler.useLruCache() && sharedPreferencesHandler.generateThumbnails() != ThumbnailsOption.NEVER
}

private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) {
val thumbnailFile: File = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache)
thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, thumbnailFile.outputStream())

try {
cache?.let {
LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile)
} ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache")
} catch (e: IOException) {
Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache")
}

thumbnailFile.delete()
}
Comment on lines +524 to +537
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve resource management in storeThumbnail method.

The storeThumbnail method is generally well-implemented, but there's a potential resource leak if an exception occurs during the bitmap compression or cache storage process. The temporary file might not be deleted in such cases.

Consider using Kotlin's use function or a try-finally block to ensure the temporary file is always deleted:

 private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) {
-    val thumbnailFile: File = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache)
-    thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, thumbnailFile.outputStream())
-
-    try {
-        cache?.let {
-            LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile)
-        } ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache")
-    } catch (e: IOException) {
-        Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache")
+    val thumbnailFile = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache)
+    try {
+        thumbnailFile.outputStream().use { outputStream ->
+            thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, outputStream)
+        }
+        
+        try {
+            cache?.let {
+                LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile)
+            } ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache")
+        } catch (e: IOException) {
+            Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache")
+        }
+    } finally {
+        thumbnailFile.delete()
     }
-
-    thumbnailFile.delete()
 }

This change ensures that the temporary file is always deleted, even if an exception occurs during the process.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) {
val thumbnailFile: File = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache)
thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, thumbnailFile.outputStream())
try {
cache?.let {
LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile)
} ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache")
} catch (e: IOException) {
Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache")
}
thumbnailFile.delete()
}
private fun storeThumbnail(cache: DiskLruCache?, cacheKey: String, thumbnailBitmap: Bitmap) {
val thumbnailFile = File.createTempFile(UUID.randomUUID().toString(), ".thumbnail", internalCache)
try {
thumbnailFile.outputStream().use { outputStream ->
thumbnailBitmap.compress(Bitmap.CompressFormat.JPEG, 100, outputStream)
}
try {
cache?.let {
LruFileCacheUtil.storeToLruCache(it, cacheKey, thumbnailFile)
} ?: Timber.tag("CryptoImplDecorator").e("Failed to store item in LRU cache")
} catch (e: IOException) {
Timber.tag("CryptoImplDecorator").e(e, "Failed to write the thumbnail in DiskLruCache")
}
} finally {
thumbnailFile.delete()
}
}


private fun isImageMediaType(filename: String): Boolean {
return (mimeTypes.fromFilename(filename) ?: MimeType.WILDCARD_MIME_TYPE).mediatype == "image"
}

@Throws(BackendException::class, IOException::class)
private fun readToTmpFile(cryptoFile: CryptoFile, file: CloudFile, progressAware: ProgressAware<DownloadState>): File {
val encryptedTmpFile = File.createTempFile(UUID.randomUUID().toString(), ".crypto", internalCache)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ open class CryptoImplVaultFormat7 : CryptoImplDecorator {
val shortFileName = BaseEncoding.base64Url().encode(hash) + LONG_NODE_FILE_EXT
var dirFolder = cloudContentRepository.folder(getOrCreateCachingAwareDirIdInfo(cryptoParent).cloudFolder, shortFileName)

// if folder already exists in case of renaming
if (!cloudContentRepository.exists(dirFolder)) {
dirFolder = cloudContentRepository.create(dirFolder)
}
Expand Down Expand Up @@ -380,6 +379,7 @@ open class CryptoImplVaultFormat7 : CryptoImplDecorator {

@Throws(BackendException::class)
override fun move(source: CryptoFile, target: CryptoFile): CryptoFile {
renameFileInCache(source, target)
return if (source.cloudFile.parent.name.endsWith(LONG_NODE_FILE_EXT)) {
val targetDirFolder = cloudContentRepository.folder(target.cloudFile.parent, target.cloudFile.name)
val cryptoFile: CryptoFile = if (target.cloudFile.name.endsWith(LONG_NODE_FILE_EXT)) {
Expand Down Expand Up @@ -449,6 +449,15 @@ open class CryptoImplVaultFormat7 : CryptoImplDecorator {
} else {
cloudContentRepository.delete(node.cloudFile)
}

val cacheKey = generateCacheKey(node)
node.cloudFile.cloud?.type()?.let { cloudType ->
getLruCacheFor(cloudType)?.let { diskCache ->
if (diskCache[cacheKey] != null) {
diskCache.delete(cacheKey)
}
}
}
}
}

Expand Down Expand Up @@ -493,7 +502,7 @@ open class CryptoImplVaultFormat7 : CryptoImplDecorator {
cryptoFile, //
cloudContentRepository.write( //
targetFile, //
data.decorate(from(encryptedTmpFile)),
data.decorate(from(encryptedTmpFile)), //
UploadFileReplacingProgressAware(cryptoFile, progressAware), //
replace, //
encryptedTmpFile.length()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,7 @@ internal class CryptoImplVaultFormatPre7(
.filterIsInstance<CloudFile>()
.map { node ->
ciphertextToCleartextNode(cryptoFolder, dirId, node)
}
.toList()
.filterNotNull()
}.toList().filterNotNull()
}

@Throws(BackendException::class)
Expand Down Expand Up @@ -228,6 +226,7 @@ internal class CryptoImplVaultFormatPre7(
@Throws(BackendException::class)
override fun move(source: CryptoFile, target: CryptoFile): CryptoFile {
assertCryptoFileAlreadyExists(target)
renameFileInCache(source, target)
return file(target, cloudContentRepository.move(source.cloudFile, target.cloudFile), source.size)
}

Expand All @@ -248,6 +247,15 @@ internal class CryptoImplVaultFormatPre7(
evictFromCache(node)
} else if (node is CryptoFile) {
cloudContentRepository.delete(node.cloudFile)

val cacheKey = generateCacheKey(node)
node.cloudFile.cloud?.type()?.let { cloudType ->
getLruCacheFor(cloudType)?.let { diskCache ->
if (diskCache[cacheKey] != null) {
diskCache.delete(cacheKey)
}
}
}
}
}

Expand Down
Loading