Introduce landscapist-core and landscapist-image modules#809
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new Kotlin Multiplatform image-loading system (landscapist-core) with caching, fetchers, decoders (static/progressive/region), scheduling, events, and Compose integration (landscapist-image), platform implementations and placeholder plugins, build-convention additions, demo app updates, and many unit/instrumentation tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Compose UI
participant LS as Landscapist
participant MC as MemoryCache
participant DC as DiskCache
participant Fetcher as ImageFetcher
participant Scheduler as DecodeScheduler
participant Decoder as ImageDecoder
UI->>LS: request load(ImageRequest)
LS->>MC: lookup(cacheKey)
alt memory hit
MC-->>LS: CachedImage
LS-->>UI: ImageResult.Success (MEMORY)
else memory miss
LS->>DC: get(cacheKey)
alt disk hit
DC-->>LS: bytes (Snapshot)
LS->>Scheduler: schedule decode(bytes, priority)
Scheduler->>Decoder: decode(bytes)
Decoder-->>LS: DecodeResult.Success(bitmap)
LS->>MC: set(cacheKey, CachedImage)
LS-->>UI: ImageResult.Success (DISK)
else disk miss
LS->>Fetcher: fetch(request)
Fetcher-->>LS: FetchResult.Success(bytes)
alt progressive supported
LS->>Decoder: decodeProgressive(bytes)
Decoder-->>LS: ProgressiveDecodeResult.Intermediate
LS-->>UI: ImageResult.Success (INTERMEDIATE)
Decoder-->>LS: ProgressiveDecodeResult.Complete
else
LS->>Scheduler: schedule decode(bytes, priority)
Scheduler->>Decoder: decode(bytes)
end
Decoder-->>LS: DecodeResult.Success(bitmap)
LS->>DC: edit(cacheKey) -> write bytes
LS->>MC: set(cacheKey, CachedImage)
LS-->>UI: ImageResult.Success (NETWORK)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (23)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
gradle/libs.versions.toml-49-51 (1)
49-51: Duplicate library declaration for ui-test-junit4.Line 49 (
androidx-compose-ui-test) and line 51 (androidx-compose-ui-test-junit4) both reference the same artifactui-test-junit4. Consider removing one to avoid confusion.🔎 Suggested fix
androidx-compose-ui-test = { group = "androidx.compose.ui", name = "ui-test-junit4" } androidx-compose-ui-test-manifest = { group = "androidx.compose.ui", name = "ui-test-manifest" } -androidx-compose-ui-test-junit4 = { group = "androidx.compose.ui", name = "ui-test-junit4" }Then update
app/build.gradle.ktsto uselibs.androidx.compose.ui.testinstead oflibs.androidx.compose.ui.test.junit4.landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/LandscapistConfig.kt-53-55 (1)
53-55: Documentation mismatch: comment says "25% of available memory, max 256MB" but value is 64MB.The comment describes a dynamic calculation ("25% of available memory, max 256MB"), but the actual value is a hardcoded 64MB constant. Either update the comment to match the actual behavior, or implement the dynamic calculation if that's the intended design.
🔎 Suggested documentation fix
- /** Default memory cache size: 25% of available memory, max 256MB. */ + /** Default memory cache size: 64MB. */ public const val DEFAULT_MEMORY_CACHE_SIZE: Long = 64 * 1024 * 1024L // 64MBlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/AnimatedImageDetector.kt-69-75 (1)
69-75: Incomplete PNG signature check - only validates first 4 of 8 bytes.The PNG specification requires an 8-byte signature:
89 50 4E 47 0D 0A 1A 0A. This check only validates the first 4 bytes, which could match non-PNG data that happens to start with\x89PNG.🔎 Suggested fix for complete PNG signature
private fun isPng(data: ByteArray): Boolean { if (data.size < 8) return false return data[0] == 0x89.toByte() && data[1] == 'P'.code.toByte() && data[2] == 'N'.code.toByte() && - data[3] == 'G'.code.toByte() + data[3] == 'G'.code.toByte() && + data[4] == 0x0D.toByte() && + data[5] == 0x0A.toByte() && + data[6] == 0x1A.toByte() && + data[7] == 0x0A.toByte() }landscapist-core/src/desktopMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.desktop.kt-99-114 (1)
99-114:getScaledInstancemay not complete scaling beforedrawImagereturns.
Image.getScaledInstance()returns anImagethat loads asynchronously. Passingnullas theImageObservertodrawImage()means the method may return before the scaled image is fully rendered, potentially resulting in incomplete or blank output.Consider using
Graphics2D.drawImage(image, x, y, width, height, observer)directly for synchronous scaling, or useMediaTrackerto wait for the scaled instance.🔎 Suggested fix using direct drawImage scaling
private fun scaleImage( image: BufferedImage, targetWidth: Int, targetHeight: Int, ): BufferedImage { val scaledImage = BufferedImage(targetWidth, targetHeight, BufferedImage.TYPE_INT_ARGB) val graphics = scaledImage.createGraphics() + graphics.setRenderingHint( + java.awt.RenderingHints.KEY_INTERPOLATION, + java.awt.RenderingHints.VALUE_INTERPOLATION_BILINEAR, + ) - graphics.drawImage( - image.getScaledInstance(targetWidth, targetHeight, java.awt.Image.SCALE_SMOOTH), - 0, - 0, - null, - ) + graphics.drawImage(image, 0, 0, targetWidth, targetHeight, null) graphics.dispose() return scaledImage }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/MemoryCache.kt-76-80 (1)
76-80: Defaultresizeimplementation doesn't updatemaxSize.The default implementation only calls
trimToSizewhen the new size is smaller but doesn't actually update themaxSizeproperty. Implementations that rely on this default will trim entries but continue reporting the oldmaxSize, which could cause confusion.🔎 Consider making resize abstract or documenting the limitation
Either make
resizeabstract to force implementations to handlemaxSizeupdates, or document that implementations must override this method to update their internalmaxSize:/** * Resizes the cache to the new maximum size. * If the new size is smaller than current size, entries will be evicted. + * + * Note: Default implementation only trims entries. Implementations should + * override this method to also update their internal maxSize tracking. * * @param newMaxSize The new maximum size in bytes. */ public fun resize(newMaxSize: Long) {Committable suggestion skipped: line range outside the PR's diff.
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.android.kt-111-163 (1)
111-163: Dead code:partialBitmapis never assigned.The variable
partialBitmap(line 111) is declared but never set. TheOnPartialImageListenercallback at line 133-138 doesn't provide access to partial bitmaps—it's called when decoding fails partway through, not for intermediate progressive scans. Lines 151-161 are unreachable.Either remove the dead code or implement actual partial bitmap capture if the API supports it.
🔎 Suggested cleanup
private fun decodeWithImageDecoder( data: ByteArray, originalWidth: Int, originalHeight: Int, targetWidth: Int?, targetHeight: Int?, config: LandscapistConfig, ): Flow<ProgressiveDecodeResult> = flow { val source = android.graphics.ImageDecoder.createSource(ByteBuffer.wrap(data)) - var partialBitmap: Bitmap? = null try { val bitmap = android.graphics.ImageDecoder.decodeBitmap(source) { decoder, info, _ -> // ... sizing logic ... decoder.setAllocator(android.graphics.ImageDecoder.ALLOCATOR_SOFTWARE) decoder.setOnPartialImageListener { exception -> false // Continue decoding } } emit( ProgressiveDecodeResult.Complete( bitmap = bitmap, width = originalWidth, height = originalHeight, ), ) } catch (e: Exception) { - partialBitmap?.let { partial -> - emit( - ProgressiveDecodeResult.Intermediate( - bitmap = partial, - width = partial.width, - height = partial.height, - progress = 0.5f, - isPreview = true, - ), - ) - } emit(ProgressiveDecodeResult.Error(e)) } }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/network/KtorImageFetcher.kt-106-123 (1)
106-123: Potential resource leak:HttpClientis never closed.The
HttpClientcreated increate()is never explicitly closed. In long-running applications, this could leak connections. Consider:
- Implementing
Closeable/AutoCloseableonKtorImageFetcher- Documenting that callers should manage the
HttpClientlifecycle- Or ensuring the client is reused globally and closed on application termination
🔎 Suggested approach
+import java.io.Closeable + public class KtorImageFetcher( private val httpClient: HttpClient, private val networkConfig: NetworkConfig, -) : ImageFetcher { +) : ImageFetcher, Closeable { + + override fun close() { + httpClient.close() + } // ... }Committable suggestion skipped: line range outside the PR's diff.
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/scheduler/DecodeScheduler.kt-207-233 (1)
207-233: Potential double-counting and logic issues incancelByTag.
Line 212-214: Filters
pendingRequestsby tag, then looks up those IDs inactiveRequests. But a request's lifecycle moves from pending → active → completed. If a request is inactiveRequests, it may no longer be inpendingRequests.Line 231: Returns
count + idsToRemove.size, but if the same request is found in both lookups, it's double-counted.Consider simplifying:
🔎 Suggested fix
public suspend fun cancelByTag(tag: String): Int { return mutex.withLock { - var count = 0 - - // Cancel active requests with this tag - val activeToRemove = pendingRequests.entries - .filter { it.value.tag == tag } - .mapNotNull { activeRequests[it.key] } - - for (deferred in activeToRemove) { - deferred.cancel() - count++ - } - - // Remove pending requests with this tag - val idsToRemove = pendingRequests.entries - .filter { it.value.tag == tag } - .map { it.key } - - for (id in idsToRemove) { - pendingRequests.remove(id) - activeRequests.remove(id) - } - - count + idsToRemove.size + val idsToRemove = pendingRequests.entries + .filter { it.value.tag == tag } + .map { it.key } + + for (id in idsToRemove) { + pendingRequests.remove(id) + activeRequests.remove(id)?.cancel() + } + + idsToRemove.size } }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/interceptor/Interceptor.kt-99-113 (1)
99-113: MissingpriorityandprogressiveEnabledfields when rebuildingImageRequest.The
HeaderInterceptorrebuilds theImageRequestbut doesn't copy thepriorityandprogressiveEnabledfields from the original request. This causes these settings to be reset to their defaults.The same issue exists in
UrlRewriteInterceptor(lines 132-141).🔎 Proposed fix for HeaderInterceptor
val newRequest = ImageRequest( model = request.model, memoryCachePolicy = request.memoryCachePolicy, diskCachePolicy = request.diskCachePolicy, headers = headers + request.headers, transformations = request.transformations, targetWidth = request.targetWidth, targetHeight = request.targetHeight, tag = request.tag, + priority = request.priority, + progressiveEnabled = request.progressiveEnabled, )landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/Landscapist.kt-224-232 (1)
224-232: diskPath calculated before commit may reference non-existent file.The
diskPathis computed on line 230 beforeeditor.commit()is called on line 229. While the path will eventually be valid after commit, if there's an error during commit, thediskPathwould reference a non-existent file that gets passed to subsequent code.Consider moving the path calculation after the commit, or using the editor's
dataPathfor consistency.🔎 Proposed fix
if (request.diskCachePolicy.writeEnabled && diskCache != null) { diskCache.edit(cacheKey)?.use { editor -> diskCache.fileSystem.sink(editor.dataPath).buffer().use { sink -> sink.write(fetchResult.data) } editor.commit() - diskPath = (diskCache.directory / cacheKey.diskKey).toString() + diskPath = editor.dataPath.toString() } }
🧹 Nitpick comments (34)
landscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/LandscapistPainter.skia.kt (1)
25-27: Consider enhancing the documentation.The KDoc could be more comprehensive by mentioning the supported data type (Skia Bitmap), the fallback behavior for unsupported types (EmptyPainter), and how null values are handled.
🔎 Suggested documentation enhancement
/** - * Creates and remembers a [Painter] from Skia Bitmap data. + * Creates and remembers a [Painter] from Skia Bitmap data. + * + * @param data The data to convert to a Painter. Supported types: + * - [org.jetbrains.skia.Bitmap]: Converted to [BitmapPainter] + * - Other types (including null): Returns [EmptyPainter] + * @return A memoized [Painter] instance */landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.android.kt (1)
204-221: Consider iteration over recursion for edge-case safety.The recursive call on line 209 handles empty entries, but if
bitmapsBySizesomehow contains multiple consecutive empty lists, recursion could stack up. Converting to iteration would be more defensive.🔎 Suggested iterative approach
private fun evictOne(): Boolean { - val firstEntry = bitmapsBySize.firstEntry() ?: return false - val bitmaps = firstEntry.value - if (bitmaps.isEmpty()) { - bitmapsBySize.remove(firstEntry.key) - return evictOne() - } + while (true) { + val firstEntry = bitmapsBySize.firstEntry() ?: return false + val bitmaps = firstEntry.value + if (bitmaps.isEmpty()) { + bitmapsBySize.remove(firstEntry.key) + continue + } - val pooled = bitmaps.removeAt(0) - if (bitmaps.isEmpty()) { - bitmapsBySize.remove(firstEntry.key) - } + val pooled = bitmaps.removeAt(0) + if (bitmaps.isEmpty()) { + bitmapsBySize.remove(firstEntry.key) + } - pooled.bitmap.recycle() - _currentSize.addAndGet(-pooled.byteCount.toLong()) - _evictions.incrementAndGet() - return true + pooled.bitmap.recycle() + _currentSize.addAndGet(-pooled.byteCount.toLong()) + _evictions.incrementAndGet() + return true + } }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.kt (1)
153-167: Consider using@Volatileor synchronized initialization.The race condition is acknowledged, but without
@Volatile, thepoolassignment on line 165 may not be visible to other threads immediately, potentially causing multiple pool creations. While the impact is minor (extra initialization), adding@Volatilewould be a low-cost improvement.🔎 Suggested fix
public object GlobalBitmapPool { - private var pool: BitmapPool? = null + @Volatile + private var pool: BitmapPool? = nulllandscapist-zoomable/src/androidMain/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomableContent.android.kt (2)
86-92: Consider closing on background thread for consistency.For consistency with the suggested fix above, the disposal cleanup could also close the decoder on a background thread. However, since this runs during teardown, blocking briefly may be acceptable.
94-101: Redundant variable assignment.
currentDecoderon line 98 is immediately assigned fromdecoder(line 95) and used in the same scope without any intervening mutation. This appears to be leftover from refactoring.🔎 Proposed simplification
// Use explicit decoder if provided, otherwise use the one we created val decoder = explicitDecoder ?: createdDecoder // Use sub-sampling when enabled and decoder is available - val currentDecoder = decoder - if (config.enableSubSampling && currentDecoder != null) { + if (config.enableSubSampling && decoder != null) { val subSamplingState = rememberSubSamplingState( - decoder = currentDecoder, + decoder = decoder, config = config.subSamplingConfig, )landscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashDecoder.kt (1)
299-327: Consider extractingbase64Decodeto a shared utility.This
base64Decodeimplementation is nearly identical to the one in ThumbHashPlugin.kt (shown in relevant snippets). The only difference is that the plugin version also strips\nand\rcharacters.Consider consolidating into a single shared utility to avoid duplication and ensure consistent behavior.
🔎 Suggested consolidation
private fun base64Decode(input: String): ByteArray? { val base64Chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" - val cleanInput = input.replace("=", "") + val cleanInput = input.replace("=", "").replace("\n", "").replace("\r", "") if (cleanInput.isEmpty()) return nulllandscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashPlugin.kt (1)
98-128: Duplicate base64 decoding logic.This
base64Decodeimplementation is nearly identical to the one inThumbHashDecoder.kt(lines 298-326). Consider consolidating into a single shared utility to avoid code duplication and potential divergence.The only difference is the additional newline stripping here (
.replace("\n", "").replace("\r", "")), which could be added to a shared implementation.landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/transformation/Transformation.kt (1)
29-35: Consider documenting expected input types per platform.Using
Anyis a pragmatic multiplatform approach, but implementations will need runtime casting which risksClassCastExceptionif passed unexpected types. Consider adding KDoc specifying what types each platform expects (e.g.,Bitmapon Android,UIImageon Apple) to guide implementers.🔎 Suggested documentation enhancement
/** * Transforms the input image. * - * @param input The input bitmap data (platform-specific type). + * @param input The input bitmap data (platform-specific type): + * - Android: `android.graphics.Bitmap` + * - iOS/macOS: `platform.UIKit.UIImage` or `platform.AppKit.NSImage` + * - Desktop: `org.jetbrains.skia.Image` * @return The transformed bitmap data. */ public suspend fun transform(input: Any): Anylandscapist-core/build.gradle.kts (1)
44-49: Minor redundancy in opt-in declarations.The
RequiresOptInandExperimentalCoroutinesApiopt-ins are declared twice: once vialanguageSettings.optIn(lines 47-48) for all source sets, and again viafreeCompilerArgs(lines 117-118) for JVM compiles. This is harmless but redundant.🔎 Consider consolidating opt-ins
Keep the
languageSettings.optInapproach (lines 47-48) and remove the duplicates fromfreeCompilerArgs:tasks.withType<KotlinJvmCompile>().configureEach { compilerOptions { jvmTarget.set(JvmTarget.JVM_17) freeCompilerArgs.addAll( "-Xexplicit-api=strict", - "-opt-in=kotlin.RequiresOptIn", - "-opt-in=kotlinx.coroutines.ExperimentalCoroutinesApi", ) } }Also applies to: 112-121
landscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/ImageBitmapConverter.skia.kt (1)
30-35: Consider logging the exception before returning null.The caught exception is swallowed without any logging or error context, making it difficult to diagnose image decoding failures in production. Consider logging the exception with appropriate context (e.g., data size, platform).
🔎 Suggested improvement
is RawImageData -> { try { val skiaImage = Image.makeFromEncoded(data.data) skiaImage.toComposeImageBitmap() } catch (e: Exception) { + // Log for debugging: println("Failed to decode image: ${e.message}, size: ${data.data.size}") null } }landscapist-image/build.gradle.kts (1)
46-50: Opt-in annotations are duplicated.The opt-in annotations are applied twice:
- Lines 47-49 for all source sets via
languageSettings.optIn()- Lines 103-105 for
KotlinJvmCompiletasks viafreeCompilerArgsThe source set configuration should be sufficient. The task-level configuration is redundant unless there's a specific need for JVM-only targets.
🔎 Consider removing duplicate opt-ins from task configuration
tasks.withType<KotlinJvmCompile>().configureEach { compilerOptions { jvmTarget.set(JvmTarget.JVM_17) freeCompilerArgs.addAll( "-Xexplicit-api=strict", - "-opt-in=kotlin.RequiresOptIn", - "-opt-in=kotlinx.coroutines.ExperimentalCoroutinesApi", - "-opt-in=com.skydoves.landscapist.InternalLandscapistApi", ) } }Also applies to: 98-107
landscapist-image/src/androidMain/kotlin/com/skydoves/landscapist/image/LocalLandscapist.android.kt (1)
35-38: Consider usingapplicationContextfor more stable caching.Using
LocalContext.currentdirectly as arememberkey may lead to unnecessary recreations when the context instance changes (e.g., Activity recreation). UsingapplicationContextprovides a more stable reference:🔎 Proposed fix
val context = LocalContext.current - return remember(context) { - Landscapist.builder(context).build() + val appContext = context.applicationContext + return remember(appContext) { + Landscapist.builder(appContext).build() }This ensures the
Landscapistinstance survives configuration changes and isn't unnecessarily recreated. However, if Activity-specific context features are needed (unlikely for disk cache initialization), the current approach would be appropriate.landscapist-core/src/desktopMain/kotlin/com/skydoves/landscapist/core/Landscapist.desktop.kt (1)
26-34: Consider platform-specific cache directories for improved Desktop cross-platform compatibility.While the current implementation using
~/.cache/landscapistworks correctly across platforms (directory creation is handled automatically), it follows Linux XDG conventions. On macOS and Windows, users expect cache to be stored in platform-specific locations:
- Windows:
%LOCALAPPDATA%\landscapist- macOS:
~/Library/Caches/landscapistUsing platform-specific conventions would improve the user experience on non-Linux desktops without affecting functionality.
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.android.kt (1)
206-214: Consider logging the swallowedIllegalArgumentExceptionfor debugging.When bitmap reuse fails, the exception is caught and retried without
inBitmap, which is correct. However, logging this at debug level would help diagnose bitmap pool issues without affecting production behavior.🔎 Proposed enhancement
var bitmap = try { BitmapFactory.decodeByteArray(data, 0, data.size, decodeOptions) } catch (e: IllegalArgumentException) { // inBitmap was not compatible, try again without reuse + // Log.d("AndroidImageDecoder", "Bitmap reuse failed, retrying without inBitmap", e) decodeOptions.inBitmap = null BitmapFactory.decodeByteArray(data, 0, data.size, decodeOptions) } ?: return DecodeResult.Error(landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/model/ImageResult.kt (2)
39-41:data: Anysacrifices type safety for flexibility.Using
Anyfor the image data accommodates platform-specific types (Android Bitmap, Skia Image, etc.), but consumers must cast. Consider documenting expected types or using a type parameter if feasible in future iterations.
93-96: Consider making at least one failure property required.Both
throwableandmessageare nullable with defaults. AFailurewithnullfor both provides no diagnostic information. Consider requiring at least one, or adding validation.🔎 Alternative approach
public data class Failure( val throwable: Throwable? = null, val message: String? = null, ) : ImageResult() { init { require(throwable != null || message != null) { "Failure must have either throwable or message" } } }landscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LandscapistImageState.kt (1)
95-99: Consider adding error data field for consistency.Looking at the existing
ImageLoadState.Failureinlandscapist/src/commonMain/kotlin/com/skydoves/landscapist/ImageLoadState.kt(lines 37-38), it includes bothdata: Any?andreason: Throwable?. The newLandscapistImageState.Failureonly hasreason. If error placeholders or fallback images are needed, thedatafield might be required.🔎 Optional: Add data field for consistency
public data class Failure( + val data: Any? = null, val reason: Throwable?, ) : LandscapistImageState()landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.kt (1)
55-60: Consider documenting expected bitmap types per platform.The
bitmap: Anyfield loses compile-time type safety. Consider adding KDoc listing the expected types per platform (e.g.,android.graphics.Bitmapon Android,org.jetbrains.skia.Imageon Skia platforms) to help consumers understand what to expect.landscapist-core/src/skiaMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.skia.kt (2)
51-57: Document that width/height are placeholders.The values
width = 0andheight = 0are intentional placeholders, but consumers may not expect this. The inline comment helps, but consider adding this to theProgressiveDecodeResult.Completedocumentation or ensuring the UI layer explicitly handles zero dimensions.
63-66:supportsProgressiveDecodereturningtruemay be misleading.This implementation doesn't actually perform progressive decoding - it wraps raw data for the Compose layer to decode. Returning
truesuggests progressive JPEG/PNG decoding is supported, which could mislead callers. Consider returningfalseor renaming the semantic.🔎 Alternative: Return false to indicate no native progressive support
override fun supportsProgressiveDecode(data: ByteArray, mimeType: String?): Boolean { - // Progressive effect is simulated in UI layer for Skia platforms - return true + // Skia platforms don't support native progressive decoding. + // The UI layer simulates progressive effect via blur animation. + return false }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/scheduler/DecodePriority.kt (1)
65-71: Lambda field in data class affects equality semantics.The
decodersuspend lambda is included in the auto-generatedequals/hashCode. TwoPrioritizedRequestinstances with identicalid,priority,createdAt, andtagbut different lambda instances will not be equal, even if they represent the same logical request. If request deduplication byidis needed, consider excludingdecoderfrom equality:🔎 Optional: Custom equals/hashCode excluding decoder
public data class PrioritizedRequest<T>( val id: String, val priority: DecodePriority, val createdAt: Long, val tag: String? = null, val decoder: suspend () -> T, ) : Comparable<PrioritizedRequest<T>> { override fun equals(other: Any?): Boolean { if (this === other) return true if (other == null || this::class != other::class) return false other as PrioritizedRequest<*> return id == other.id } override fun hashCode(): Int = id.hashCode() // ... compareTo remains unchanged }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/TwoTierMemoryCache.kt (1)
54-80: Minor: Unconditional weak cache cleanup is correct but could use a clarifying comment.Line 76 removes the key from weakCache after the
?.get()check returns null. This is correct cleanup of collected references, but the control flow might be clearer with a brief comment.🔎 Optional clarification
// Clean up null weak reference + // (reached only when weakCache[memoryKey]?.get() returned null) weakCache.remove(memoryKey)landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/LruMemoryCache.kt (1)
98-107: Consider extracting shared eviction logic to reduce duplication.This
evictIfNeededimplementation is nearly identical to the one inTwoTierMemoryCache. Consider extracting common eviction logic to a shared utility or abstract base class for maintainability.landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.android.kt (2)
253-305: Code duplication withImageDecoder.android.kt.Both
calculateSampleSizeandhasAlphalogic are duplicated fromImageDecoder.android.kt(see lines 239-272 in that file). Consider extracting these to a shared utility to maintain consistency and reduce maintenance burden.
307-309: Remove customemitAllextension; use the built-in implementation fromkotlinx.coroutines.flow.Kotlin's standard library already provides
emitAllforFlowCollectorwith identical behavior. Replace this custom implementation by importing and using the standard version.+import kotlinx.coroutines.flow.emitAll ... - private suspend fun <T> kotlinx.coroutines.flow.FlowCollector<T>.emitAll(flow: Flow<T>) { - flow.collect { emit(it) } - }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/scheduler/DecodeScheduler.kt (1)
145-160: Inefficient busy-loop inwaitForTurn.The
while(true)loop withyield()consumes CPU cycles polling for priority changes. For high-throughput scenarios with many pending requests, consider using aChannelor suspend-based signaling mechanism to wake waiters only when priorities change.landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.kt (1)
35-54: Consider documenting platform-specific bitmap types.The
bitmapproperty is typed asAny, which is necessary for KMP but loses type safety. Consider adding KDoc noting the expected types per platform (e.g.,android.graphics.Bitmapon Android,org.jetbrains.skia.Imageon desktop).🔎 Suggested documentation
/** * An intermediate image during progressive decode. * - * @property bitmap The partially decoded bitmap. + * @property bitmap The partially decoded bitmap. Platform-specific type: + * - Android: `android.graphics.Bitmap` + * - Desktop/iOS: `org.jetbrains.skia.Image` * @property width The width of the image.landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.android.kt (2)
46-48: Consider logging swallowed exceptions for debuggability.When
BitmapRegionDecoder.newInstancethrows, the exception is silently discarded. This makes debugging image loading failures harder. Consider logging the exception or providing a result type that includes error details.🔎 Proposed improvement
} catch (e: Exception) { + // Log for debugging: e.g., Log.w("RegionDecoder", "Failed to create decoder", e) null }
129-148: Original exception context is lost in fallback path.When
inBitmapdecoding fails withIllegalArgumentException, the original exceptioneis discarded. If the fallback also fails, onlye2is reported. Consider preserving context by usingaddSuppressedor logging the original exception.🔎 Proposed improvement
} catch (e: IllegalArgumentException) { // inBitmap was not suitable, try without reuse options.inBitmap = null try { val bitmap = decoder.decodeRegion(rect, options) // ... success handling } catch (e2: Exception) { + e2.addSuppressed(e) // Preserve original inBitmap failure context RegionDecodeResult.Error(throwable = e2, region = region) } }landscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LandscapistImage.kt (1)
86-91: Lambda key inremembermay cause unnecessary recompositions.The
requestBuilderlambda is used as a key inremember. Since lambdas capture their context and create new instances on each composition, this may cause the request to be rebuilt more often than necessary. Consider using a stable key or documenting that callers should useremembered lambdas.landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/DiskLruCache.kt (1)
251-266: Potential inconsistency ifmetadata()fails afteratomicMove.After
atomicMovesucceeds (line 259), iffileSystem.metadata(finalPath)throws,abort()is called which attempts to deletetempPath(which no longer exists). This leaves an orphaned file atfinalPaththat isn't tracked inentries. The file will be recovered on next initialization, butcurrentSizemay be inaccurate until then.Consider handling this edge case by attempting to delete
finalPathin the catch block or tracking the entry with size 0 as a fallback.🔎 Proposed improvement
try { // Move temp file to final location if (fileSystem.exists(tempPath)) { fileSystem.delete(finalPath) fileSystem.atomicMove(tempPath, finalPath) val size = fileSystem.metadata(finalPath).size ?: 0L onCommit(size) } } catch (_: IOException) { - abort() + // Cleanup: try to remove the potentially orphaned final file + try { + fileSystem.delete(finalPath) + } catch (_: IOException) { + // Best effort cleanup failed; file will be recovered on next init + } }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/interceptor/Interceptor.kt (1)
149-175: Edge case: negativemaxRetriesreturnsLoadingwithout attempting the request.If
maxRetriesis set to a negative value, the while loop conditionattempts <= maxRetries(0 <= -1) is false, and the function returnsImageResult.Loadingwithout ever attempting to load the image.Consider adding validation or using
coerceAtLeast(0):🔎 Proposed fix
public class RetryInterceptor( - private val maxRetries: Int = 3, + private val maxRetries: Int = 3.coerceAtLeast(0), private val retryOnError: (Throwable?) -> Boolean = { true }, ) : Interceptor {Or add a
requirecheck:init { require(maxRetries >= 0) { "maxRetries must be non-negative" } }landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/Landscapist.kt (2)
298-309: Request ID based on hashCode may cause scheduling conflicts for different models.The
requestIdconstruction usinghashCode()could result in ID collisions when different image models produce the same hash value with identical dimensions. While this primarily affects scheduling order rather than correctness, it could cause unexpected priority behavior.Consider using a more unique identifier like the model's
toString()or a combination with a unique counter.
535-538: Consider using Dispatchers.IO for I/O operations.The default dispatcher is
Dispatchers.Default, which is optimized for CPU-bound work. Image loading involves network I/O and disk I/O operations which are better suited forDispatchers.IO.While decoding is CPU-bound (making Default appropriate for that phase), the overall flow includes significant I/O work. Consider defaulting to
Dispatchers.IOor documenting whyDispatchers.Defaultwas chosen.🔎 Proposed change
- private var dispatcher: CoroutineDispatcher = Dispatchers.Default + private var dispatcher: CoroutineDispatcher = Dispatchers.IO
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kotlin-js-store/wasm/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (97)
app/build.gradle.ktsapp/src/androidTest/kotlin/com/github/skydoves/landscapistdemo/LandscapistImageTest.ktapp/src/main/kotlin/com/github/skydoves/landscapistdemo/ui/MainPosters.ktbom/build.gradle.ktsbuild-logic/convention/build.gradle.ktsbuild-logic/convention/src/main/kotlin/KmpLibraryConventionPlugin.ktbuild-logic/convention/src/main/kotlin/com/skydoves/landscapist/KmpLibrary.ktgradle/libs.versions.tomllandscapist-core/api/android/landscapist-core.apilandscapist-core/api/desktop/landscapist-core.apilandscapist-core/build.gradle.ktslandscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/Landscapist.android.ktlandscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.android.ktlandscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.android.ktlandscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.android.ktlandscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.android.ktlandscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/memory/AndroidMemoryPressureHandler.ktlandscapist-core/src/appleMain/kotlin/com/skydoves/landscapist/core/Landscapist.apple.ktlandscapist-core/src/appleMain/kotlin/com/skydoves/landscapist/core/cache/WeakRef.apple.ktlandscapist-core/src/appleMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.apple.ktlandscapist-core/src/appleMain/kotlin/com/skydoves/landscapist/core/event/TimeUtils.apple.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/DownsamplingStrategy.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/ImageRequest.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/Landscapist.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/LandscapistConfig.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/CacheKey.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/DiskCache.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/DiskLruCache.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/LruMemoryCache.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/MemoryCache.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/TwoTierMemoryCache.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/WeakRef.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/AnimatedImageDetector.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/event/EventListener.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/event/ImageLoadEvent.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/interceptor/Interceptor.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/memory/MemoryPressureListener.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/model/CachePolicy.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/model/DataSource.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/model/ImageResult.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/network/ImageFetcher.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/network/KtorImageFetcher.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/request/Disposable.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/request/RequestManager.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/scheduler/DecodePriority.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/scheduler/DecodeScheduler.ktlandscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/transformation/Transformation.ktlandscapist-core/src/desktopMain/kotlin/com/skydoves/landscapist/core/Landscapist.desktop.ktlandscapist-core/src/desktopMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.desktop.ktlandscapist-core/src/jvmMain/kotlin/com/skydoves/landscapist/core/cache/WeakRef.jvm.ktlandscapist-core/src/jvmMain/kotlin/com/skydoves/landscapist/core/event/TimeUtils.jvm.ktlandscapist-core/src/skiaMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.skia.ktlandscapist-core/src/skiaMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.skia.ktlandscapist-core/src/skiaMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.skia.ktlandscapist-core/src/wasmJsMain/kotlin/com/skydoves/landscapist/core/Landscapist.wasmJs.ktlandscapist-core/src/wasmJsMain/kotlin/com/skydoves/landscapist/core/cache/WeakRef.wasmJs.ktlandscapist-core/src/wasmJsMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.wasmJs.ktlandscapist-core/src/wasmJsMain/kotlin/com/skydoves/landscapist/core/event/TimeUtils.wasmJs.ktlandscapist-image/api/android/landscapist-image.apilandscapist-image/api/desktop/landscapist-image.apilandscapist-image/build.gradle.ktslandscapist-image/src/androidMain/kotlin/com/skydoves/landscapist/image/ImageBitmapConverter.android.ktlandscapist-image/src/androidMain/kotlin/com/skydoves/landscapist/image/LandscapistPainter.android.ktlandscapist-image/src/androidMain/kotlin/com/skydoves/landscapist/image/LocalLandscapist.android.ktlandscapist-image/src/androidMain/kotlin/com/skydoves/landscapist/image/ProvideImageSource.android.ktlandscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/EmptyPainter.ktlandscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/ImageBitmapConverter.ktlandscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LandscapistImage.ktlandscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LandscapistImageState.ktlandscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LandscapistPainter.ktlandscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LocalLandscapist.ktlandscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/ProvideImageSource.ktlandscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/ImageBitmapConverter.skia.ktlandscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/LandscapistPainter.skia.ktlandscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/LocalLandscapist.skia.ktlandscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/ProvideImageSource.skia.ktlandscapist-placeholder/api/android/landscapist-placeholder.apilandscapist-placeholder/api/desktop/landscapist-placeholder.apilandscapist-placeholder/src/androidMain/kotlin/com/skydoves/landscapist/placeholder/blurhash/BlurHashPlugin.android.ktlandscapist-placeholder/src/androidMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashPlugin.android.ktlandscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/blurhash/BlurHashDecoder.ktlandscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/blurhash/BlurHashPlugin.ktlandscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/progressive/ProgressiveLoadingPlugin.ktlandscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashDecoder.ktlandscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashPlugin.ktlandscapist-placeholder/src/skiaMain/kotlin/com/skydoves/landscapist/placeholder/blurhash/BlurHashPlugin.skia.ktlandscapist-placeholder/src/skiaMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashPlugin.skia.ktlandscapist-zoomable/src/androidMain/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomableContent.android.ktlandscapist/api/android/landscapist.apilandscapist/api/desktop/landscapist.apilandscapist/src/commonMain/kotlin/com/skydoves/landscapist/ImageOptions.ktlandscapist/src/commonMain/kotlin/com/skydoves/landscapist/constraints/ConstraintsExtensions.ktsettings.gradle.kts
🧰 Additional context used
🧬 Code graph analysis (16)
landscapist-image/build.gradle.kts (1)
build-logic/convention/src/main/kotlin/KmpLibraryConventionPlugin.kt (2)
apply(27-48)apply(28-47)
landscapist-image/src/androidMain/kotlin/com/skydoves/landscapist/image/ProvideImageSource.android.kt (1)
landscapist/src/androidMain/kotlin/com/skydoves/landscapist/LocalImageSourceFile.kt (1)
ProvideImageSourceFile(49-57)
build-logic/convention/src/main/kotlin/KmpLibraryConventionPlugin.kt (1)
build-logic/convention/src/main/kotlin/com/skydoves/landscapist/KmpLibrary.kt (1)
configureKmpLibrary(28-96)
landscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/ProvideImageSource.skia.kt (1)
landscapist/src/skiaMain/kotlin/com/skydoves/landscapist/LocalImageSourceBytes.kt (1)
ProvideImageSourceBytes(48-56)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/TwoTierMemoryCache.kt (2)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/LruMemoryCache.kt (2)
evictIfNeeded(98-107)trimToSize(82-91)landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/cache/MemoryCache.kt (1)
trimToSize(68-68)
landscapist-zoomable/src/androidMain/kotlin/com/skydoves/landscapist/zoomable/internal/ZoomableContent.android.kt (1)
landscapist-zoomable/src/commonMain/kotlin/com/skydoves/landscapist/zoomable/subsampling/SubSamplingState.kt (1)
rememberSubSamplingState(45-56)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/network/KtorImageFetcher.kt (1)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/ImageRequest.kt (1)
headers(82-85)
landscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashPlugin.kt (3)
landscapist-placeholder/src/androidMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashPlugin.android.kt (1)
decodeThumbHashToImageBitmap(22-28)landscapist-placeholder/src/skiaMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashPlugin.skia.kt (1)
decodeThumbHashToImageBitmap(26-40)landscapist-placeholder/src/commonMain/kotlin/com/skydoves/landscapist/placeholder/thumbhash/ThumbHashDecoder.kt (1)
base64Decode(299-327)
app/src/main/kotlin/com/github/skydoves/landscapistdemo/ui/MainPosters.kt (1)
landscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LandscapistImage.kt (1)
LandscapistImage(72-77)
landscapist-core/build.gradle.kts (1)
build-logic/convention/src/main/kotlin/KmpLibraryConventionPlugin.kt (2)
apply(27-48)apply(28-47)
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.android.kt (1)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.kt (1)
decodeRegion(133-133)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/event/ImageLoadEvent.kt (3)
landscapist-core/src/appleMain/kotlin/com/skydoves/landscapist/core/event/TimeUtils.apple.kt (1)
currentTimeMillis(21-22)landscapist-core/src/jvmMain/kotlin/com/skydoves/landscapist/core/event/TimeUtils.jvm.kt (1)
currentTimeMillis(18-18)landscapist-core/src/wasmJsMain/kotlin/com/skydoves/landscapist/core/event/TimeUtils.wasmJs.kt (1)
currentTimeMillis(21-21)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.kt (2)
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.android.kt (1)
createBitmapPool(29-37)landscapist-core/src/skiaMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.skia.kt (1)
createBitmapPool(25-25)
app/src/androidTest/kotlin/com/github/skydoves/landscapistdemo/LandscapistImageTest.kt (1)
landscapist-image/src/commonMain/kotlin/com/skydoves/landscapist/image/LandscapistImage.kt (1)
LandscapistImage(72-77)
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ProgressiveDecoder.android.kt (1)
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.android.kt (2)
calculateSampleSize(240-267)hasAlpha(269-273)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/scheduler/DecodeScheduler.kt (2)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/Landscapist.kt (1)
decoder(531-533)landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.android.kt (1)
decoder(65-169)
🪛 detekt (1.23.8)
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/request/Disposable.kt
[warning] 72-72: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 73-73: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
landscapist-image/src/skiaMain/kotlin/com/skydoves/landscapist/image/ImageBitmapConverter.skia.kt
[warning] 33-33: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/RegionDecoder.android.kt
[warning] 46-46: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 129-129: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
landscapist-core/src/skiaMain/kotlin/com/skydoves/landscapist/core/bitmappool/BitmapPool.skia.kt
[warning] 45-45: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 47-47: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/decoder/ImageDecoder.android.kt
[warning] 208-208: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Tests
- GitHub Check: API check
- GitHub Check: Spotless check
| assert(loaded) { | ||
| "Image failed to load within timeout. " + | ||
| "State history: $stateHistory, Final state: ${imageState::class.simpleName}" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use JUnit assertions instead of Kotlin assert().
Kotlin's assert() can be disabled with JVM flags and may silently pass. Use JUnit's Assert.assertTrue() or assertThat() for reliable test behavior.
🔎 Proposed fix
+import org.junit.Assert.assertTrue
+import org.junit.Assert.fail
...
// Wait for image to load (max 10 seconds)
val loaded = latch.await(10, TimeUnit.SECONDS)
- assert(loaded) {
- "Image failed to load within timeout. " +
- "State history: $stateHistory, Final state: ${imageState::class.simpleName}"
- }
+ assertTrue(
+ "Image failed to load within timeout. State history: $stateHistory, Final state: ${imageState::class.simpleName}",
+ loaded
+ )
...
composeTestRule.runOnIdle {
- assert(imageState is LandscapistImageState.Success) {
- "Expected Success state but got: $imageState"
- }
+ assertTrue(
+ "Expected Success state but got: $imageState",
+ imageState is LandscapistImageState.Success
+ )
}Also applies to: 100-102
🤖 Prompt for AI Agents
In
app/src/androidTest/kotlin/com/github/skydoves/landscapistdemo/LandscapistImageTest.kt
around lines 89-92 (and similarly lines 100-102), the test uses Kotlin's
assert() which can be disabled at runtime; replace these with JUnit assertions
(e.g., org.junit.Assert.assertTrue or assertThat) so the test always fails when
the condition is false, passing the same failure message and condition; add the
necessary JUnit import if missing.
|
|
||
| // Wait for image to load (max 10 seconds) | ||
| val loaded = latch.await(10, TimeUnit.SECONDS) | ||
| assert(loaded) { "Image failed to load within timeout - this indicates ANR or freeze!" } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same issue: replace assert() with JUnit assertions.
The second test method also uses assert() which should be replaced with JUnit assertions for consistency and reliability.
Also applies to: 142-144
🤖 Prompt for AI Agents
In
app/src/androidTest/kotlin/com/github/skydoves/landscapistdemo/LandscapistImageTest.kt
around lines 136 and 142-144, replace usages of Kotlin's assert() with JUnit
assertions: change the assert(loaded) check to a JUnit-style assertion (e.g.,
assertTrue or assertThat with a clear failure message) and similarly convert any
other assert() calls in the 142-144 range to appropriate JUnit assertions so
tests are consistent and reliable in instrumentation runs.
| wasmJs { | ||
| browser { | ||
| testTask { | ||
| enabled = false | ||
| } | ||
| } | ||
| nodejs { | ||
| testTask { | ||
| enabled = false | ||
| } | ||
| } | ||
| binaries.library() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and examine its full content
find . -name "KmpLibrary.kt" -type fRepository: skydoves/landscapist
Length of output: 143
🏁 Script executed:
# Read the entire KmpLibrary.kt file to examine the structure and verify the claim
cat -n ./build-logic/convention/src/main/kotlin/com/skydoves/landscapist/KmpLibrary.ktRepository: skydoves/landscapist
Length of output: 2990
🏁 Script executed:
# Check for js() target declarations in similar files
rg -n "js\s*\{" --type=kotlin -A 2 -B 2Repository: skydoves/landscapist
Length of output: 1638
🏁 Script executed:
# Search for withJs() in hierarchy templates to understand the pattern
rg "withJs\(\)" --type=kotlin -B 5 -A 5Repository: skydoves/landscapist
Length of output: 3504
🏁 Script executed:
# Check what targets are declared in ComposeMultiplatform.kt and ComposeMultiplatformWasm.kt
echo "=== ComposeMultiplatform.kt targets ===" &&
head -60 ./build-logic/convention/src/main/kotlin/com/skydoves/landscapist/ComposeMultiplatform.kt | grep -E "^\s*(js|wasmJs|android|jvm|ios|macos)\s*\(" &&
echo "=== ComposeMultiplatformWasm.kt targets ===" &&
head -60 ./build-logic/convention/src/main/kotlin/com/skydoves/landscapist/ComposeMultiplatformWasm.kt | grep -E "^\s*(js|wasmJs|android|jvm|ios|macos)\s*\("Repository: skydoves/landscapist
Length of output: 185
🏁 Script executed:
# Get more context - check the first 100 lines of both files for target declarations
echo "=== ComposeMultiplatform.kt (first 100 lines) ===" &&
head -100 ./build-logic/convention/src/main/kotlin/com/skydoves/landscapist/ComposeMultiplatform.kt &&
echo -e "\n=== ComposeMultiplatformWasm.kt (first 100 lines) ===" &&
head -100 ./build-logic/convention/src/main/kotlin/com/skydoves/landscapist/ComposeMultiplatformWasm.ktRepository: skydoves/landscapist
Length of output: 4889
🏁 Script executed:
# Check if there are any js() target declarations anywhere in the build-logic directory
rg "^\s*js\s*\(" build-logic/ --type=kotlinRepository: skydoves/landscapist
Length of output: 46
🏁 Script executed:
# Double-check by searching for any js configuration across the entire codebase
rg "js\(" --type=kotlin | grep -v "nodejs\|wasmJs" | head -20Repository: skydoves/landscapist
Length of output: 46
🏁 Script executed:
# Search for any gradle build files or documentation that might explain the withJs() pattern
find . -name "build.gradle.kts" -o -name "README.md" | head -10Repository: skydoves/landscapist
Length of output: 345
Remove withJs() from the hierarchy template or add a js() target declaration.
The hierarchy template includes withJs() (line 79), but only wasmJs is declared as a target (lines 37-49). The js() target is never configured. This pattern exists across multiple configuration files (ComposeMultiplatform.kt, ComposeMultiplatformWasm.kt). Since only wasmJs is supported, remove the withJs() call and rely on withWasmJs() instead, or add an explicit js() target if JS support is intended.
🤖 Prompt for AI Agents
In build-logic/convention/src/main/kotlin/com/skydoves/landscapist/KmpLibrary.kt
around lines 37-49 (and note the hierarchy call at ~line 79), the project config
declares only a wasmJs target but the shared hierarchy template includes
withJs(); either remove the withJs() call from the hierarchy/template or add an
explicit js() target declaration to match the hierarchy. To fix: if you only
support Wasm+JS, remove the withJs() invocation and rely on withWasmJs()/wasmJs
{ ... } blocks; if you intend to support plain JS as well, add a js { ... }
target configuration matching the wasmJs setup so the hierarchy and targets are
consistent.
| okio = "3.10.2" | ||
| atomicfu = "0.27.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions of okio and atomicfu
echo "=== Okio latest version ==="
curl -s "https://api.github.com/repos/square/okio/releases/latest" | jq -r '.tag_name'
echo ""
echo "=== AtomicFU latest version ==="
curl -s "https://api.github.com/repos/Kotlin/kotlinx-atomicfu/releases/latest" | jq -r '.tag_name'Repository: skydoves/landscapist
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Try alternative method for okio and check the actual file
echo "=== Checking okio via Maven Central ==="
curl -s "https://repo1.maven.org/maven2/com/squareup/okio/okio/maven-metadata.xml" | grep -oP '(?<=<latest>)[^<]+' | head -1
echo ""
echo "=== Checking gradle/libs.versions.toml ==="
cat -n gradle/libs.versions.toml | sed -n '25,35p'
cat -n gradle/libs.versions.toml | sed -n '82,90p'Repository: skydoves/landscapist
Length of output: 1240
Update dependency versions to latest stable releases.
okio should be updated to 3.16.4 (currently 3.10.2) and atomicfu to 0.29.0 (currently 0.27.0).
🤖 Prompt for AI Agents
In gradle/libs.versions.toml around lines 28 to 29, the pinned dependency
versions are out of date: update okio from "3.10.2" to "3.16.4" and atomicfu
from "0.27.0" to "0.29.0". Edit those two entries to use the new version strings
and save the file; run a dependency refresh/build afterwards to ensure
compatibility.
| public class AndroidMemoryPressureHandler( | ||
| private val manager: MemoryPressureManager, | ||
| ) : ComponentCallbacks2 { | ||
|
|
||
| private var registered = false | ||
| private var application: Application? = null | ||
|
|
There was a problem hiding this comment.
Add thread safety for concurrent register/unregister calls.
The registered flag and application field are accessed without synchronization, creating a race condition if register() or unregister() are called concurrently from multiple threads. This could lead to duplicate registrations or incomplete cleanup.
🔎 Proposed fix
public class AndroidMemoryPressureHandler(
private val manager: MemoryPressureManager,
) : ComponentCallbacks2 {
- private var registered = false
- private var application: Application? = null
+ @Volatile
+ private var registered = false
+ @Volatile
+ private var application: Application? = nullAdditionally, consider wrapping the register/unregister logic in synchronized(this) blocks to prevent interleaved execution.
📝 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.
| public class AndroidMemoryPressureHandler( | |
| private val manager: MemoryPressureManager, | |
| ) : ComponentCallbacks2 { | |
| private var registered = false | |
| private var application: Application? = null | |
| public class AndroidMemoryPressureHandler( | |
| private val manager: MemoryPressureManager, | |
| ) : ComponentCallbacks2 { | |
| @Volatile | |
| private var registered = false | |
| @Volatile | |
| private var application: Application? = null |
🤖 Prompt for AI Agents
In
landscapist-core/src/androidMain/kotlin/com/skydoves/landscapist/core/memory/AndroidMemoryPressureHandler.kt
around lines 28-34, the mutable fields `registered` and `application` are
accessed without synchronization causing race conditions when
register()/unregister() are called concurrently; fix by wrapping the register
and unregister logic in synchronized(this) blocks so reads/writes to
`registered` and `application` are atomic (check-and-set, assign application,
and flip registered flag inside the lock), ensure register is idempotent (no
duplicate register calls) and unregister clears `application` and `registered`
inside the same synchronized block, and if you must call
application.unregisterComponentCallbacks take care to either invoke it inside
the lock or capture the application reference inside the lock then call
unregisterComponentCallbacks outside to avoid holding the lock during platform
callbacks.
| public class Landscapist private constructor( | ||
| public val config: LandscapistConfig, | ||
| private val memoryCache: MemoryCache, | ||
| private val diskCache: DiskCache?, | ||
| private val fetcher: ImageFetcher, | ||
| private val decoder: ImageDecoder, | ||
| private val dispatcher: CoroutineDispatcher, | ||
| public val requestManager: RequestManager = RequestManager(), | ||
| public val memoryPressureManager: MemoryPressureManager = MemoryPressureManager(), | ||
| ) { | ||
|
|
||
| private val scope = CoroutineScope(SupervisorJob() + dispatcher) | ||
|
|
There was a problem hiding this comment.
CoroutineScope without lifecycle management may cause resource leaks.
The scope created on line 80 uses SupervisorJob() but is never cancelled. When a Landscapist instance is replaced via setInstance() or removed via removeInstance(), the old scope's coroutines may continue running indefinitely.
Consider adding a shutdown() or close() method to cancel the scope when the instance is being replaced or removed.
🔎 Proposed addition
public class Landscapist private constructor(
...
) {
private val scope = CoroutineScope(SupervisorJob() + dispatcher)
+ private val supervisorJob = SupervisorJob()
+ private val scope = CoroutineScope(supervisorJob + dispatcher)
+
+ /**
+ * Shuts down this Landscapist instance, cancelling all pending requests.
+ * Call this before discarding the instance to prevent resource leaks.
+ */
+ public fun shutdown() {
+ requestManager.cancelAll()
+ supervisorJob.cancel()
+ }Then call shutdown() from setInstance() and removeInstance() before replacing/removing the instance.
Committable suggestion skipped: line range outside the PR's diff.
| // Track active request | ||
| scope.async { | ||
| mutex.withLock { | ||
| activeRequests[id] = deferred | ||
| } | ||
| } | ||
|
|
||
| return deferred | ||
| } |
There was a problem hiding this comment.
Race condition: activeRequests update may complete after the request finishes.
The separate async block (lines 126-130) that adds to activeRequests races with the main decode operation. The request could complete and be removed from activeRequests before it's ever added, causing cancel(id) to miss active requests.
🔎 Suggested fix
Add to activeRequests synchronously before launching the async:
+ // Track active request immediately
+ mutex.withLock {
+ activeRequests[id] = deferred
+ }
+
val deferred = scope.async {
// Add to pending
mutex.withLock {
pendingRequests[id] = request
}
// ... rest of decode logic
}
- // Track active request
- scope.async {
- mutex.withLock {
- activeRequests[id] = deferred
- }
- }
return deferredNote: This requires restructuring since deferred isn't available before it's created. Consider using CompletableDeferred or restructuring the tracking logic.
Committable suggestion skipped: line range outside the PR's diff.
| private var globalScheduler: DecodeScheduler? = null | ||
|
|
||
| /** | ||
| * Gets the global decode scheduler, creating it if necessary. | ||
| */ | ||
| public fun global(): DecodeScheduler { | ||
| globalScheduler?.let { return it } | ||
|
|
||
| val newScheduler = DecodeScheduler() | ||
| globalScheduler = newScheduler | ||
| return newScheduler | ||
| } |
There was a problem hiding this comment.
Race condition in global() singleton initialization.
Two threads calling global() concurrently could both see globalScheduler as null, create two instances, and one would be lost. Use double-checked locking or an atomic reference.
🔎 Suggested fix using atomicfu
+import kotlinx.atomicfu.atomic
+
public companion object {
public const val DEFAULT_PARALLELISM: Int = 4
- private var globalScheduler: DecodeScheduler? = null
+ private val globalScheduler = atomic<DecodeScheduler?>(null)
public fun global(): DecodeScheduler {
- globalScheduler?.let { return it }
-
- val newScheduler = DecodeScheduler()
- globalScheduler = newScheduler
- return newScheduler
+ globalScheduler.value?.let { return it }
+ val newScheduler = DecodeScheduler()
+ return if (globalScheduler.compareAndSet(null, newScheduler)) {
+ newScheduler
+ } else {
+ globalScheduler.value!!
+ }
}
public fun initialize(parallelism: Int = DEFAULT_PARALLELISM) {
- globalScheduler = DecodeScheduler(parallelism)
+ globalScheduler.value = DecodeScheduler(parallelism)
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/scheduler/DecodeScheduler.kt
around lines 264 to 275, the global() singleton initialization has a race: two
threads can both see globalScheduler as null and create separate instances; make
initialization thread-safe by replacing the current check-with-assignment with a
concurrent-safe pattern — either use double-checked locking (check
globalScheduler, synchronize on a private lock, re-check and assign) or use an
atomic reference (atomicfu or java.util.concurrent.atomic.AtomicReference) and
perform a compare-and-set to install the new DecodeScheduler, returning the
existing instance if another thread wins; ensure the chosen approach is
multiplatform-safe (use atomicfu for KMM) and avoids creating and discarding
multiple instances under concurrency.
| public class ProgressiveLoadingPlugin( | ||
| private val initialBlurRadius: Float = 16f, | ||
| private val transitionDuration: Int = 300, | ||
| ) : ImagePlugin.LoadingStatePlugin { | ||
|
|
||
| @Composable | ||
| override fun compose( | ||
| modifier: Modifier, | ||
| imageOptions: ImageOptions, | ||
| executor: @Composable (IntSize) -> Unit, | ||
| ): ImagePlugin = apply { | ||
| // Request a tiny thumbnail for fast preview | ||
| val previewSize = IntSize(32, 32) | ||
| executor.invoke(previewSize) | ||
| } |
There was a problem hiding this comment.
Constructor parameters are unused.
initialBlurRadius and transitionDuration are stored but never used within the compose method. The plugin only invokes the executor with a fixed 32x32 preview size. This appears to be an incomplete implementation.
Either:
- Remove the unused parameters if the blur logic is meant to be handled externally
- Integrate the blur rendering logic within the
composemethod similar to howProgressiveImagedoes it
| LaunchedEffect(sourceFile) { | ||
| // Close any previously created decoder first | ||
| createdDecoder?.close() | ||
| createdDecoder = null | ||
|
|
||
| // Only create decoder from source file if no explicit decoder is provided | ||
| if (explicitDecoder == null && sourceFile != null) { | ||
| val newDecoder = sourceFile.let { file -> | ||
| if (file.exists() && file.canRead()) { | ||
| withContext(Dispatchers.IO) { | ||
| ImageRegionDecoder.create(file.absolutePath) | ||
| } | ||
| } else { | ||
| null | ||
| } | ||
| } | ||
| createdDecoder = newDecoder | ||
| } | ||
| } |
There was a problem hiding this comment.
LaunchedEffect key should include explicitDecoder dependency.
The effect checks explicitDecoder == null inside, but only uses sourceFile as the key. If explicitDecoder transitions from non-null to null (e.g., parent stops providing it), the effect won't re-run and no decoder will be created from sourceFile.
Also, createdDecoder?.close() on line 68 runs on the main thread. If close() performs blocking I/O, consider moving it inside withContext(Dispatchers.IO).
🔎 Proposed fix
- LaunchedEffect(sourceFile) {
- // Close any previously created decoder first
- createdDecoder?.close()
- createdDecoder = null
+ LaunchedEffect(sourceFile, explicitDecoder == null) {
+ // Close any previously created decoder on IO thread
+ createdDecoder?.let { decoder ->
+ withContext(Dispatchers.IO) {
+ decoder.close()
+ }
+ }
+ createdDecoder = null
// Only create decoder from source file if no explicit decoder is provided
if (explicitDecoder == null && sourceFile != null) {📝 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.
| LaunchedEffect(sourceFile) { | |
| // Close any previously created decoder first | |
| createdDecoder?.close() | |
| createdDecoder = null | |
| // Only create decoder from source file if no explicit decoder is provided | |
| if (explicitDecoder == null && sourceFile != null) { | |
| val newDecoder = sourceFile.let { file -> | |
| if (file.exists() && file.canRead()) { | |
| withContext(Dispatchers.IO) { | |
| ImageRegionDecoder.create(file.absolutePath) | |
| } | |
| } else { | |
| null | |
| } | |
| } | |
| createdDecoder = newDecoder | |
| } | |
| } | |
| LaunchedEffect(sourceFile, explicitDecoder == null) { | |
| // Close any previously created decoder on IO thread | |
| createdDecoder?.let { decoder -> | |
| withContext(Dispatchers.IO) { | |
| decoder.close() | |
| } | |
| } | |
| createdDecoder = null | |
| // Only create decoder from source file if no explicit decoder is provided | |
| if (explicitDecoder == null && sourceFile != null) { | |
| val newDecoder = sourceFile.let { file -> | |
| if (file.exists() && file.canRead()) { | |
| withContext(Dispatchers.IO) { | |
| ImageRegionDecoder.create(file.absolutePath) | |
| } | |
| } else { | |
| null | |
| } | |
| } | |
| createdDecoder = newDecoder | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/request/DisposableTest.kt (1)
112-117: Consider removing or clarifying the trivial assertion.The assertion
assertTrue(true)on line 116 is always true and doesn't validate any meaningful behavior. The actual test is whetherawait()completes without blocking indefinitely (which would cause a test timeout). Consider either removing this line or replacing it with a comment explaining that successful completion proves non-blocking behavior.🔎 Proposed fix
@Test fun `ImmediateDisposable await completes immediately`() = runTest { - // Should not block ImmediateDisposable.await() - assertTrue(true) + // Test passes if await() completes without blocking }landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/event/EventListenerTest.kt (1)
28-36: Consider removing the trivial assertion.The test correctly validates that the NONE listener doesn't throw. However,
assertTrue(true)adds no value—the test passes by virtue of reaching the end without throwing.🔎 Suggested simplification
fun `NONE listener does not throw`() { val listener = EventListener.NONE val event = ImageLoadEvent.Started(model = "test", timestamp = 0L) - // Should not throw listener.onEvent(event) - assertTrue(true) }landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/DownsamplingStrategyTest.kt (2)
32-49: Consider consolidating enum presence tests.The four separate tests checking for AUTO, NONE, FIT, and FILL could be consolidated into a single test that iterates over expected values, reducing verbosity.
🔎 Suggested consolidation
@Test - fun `DownsamplingStrategy contains AUTO`() { - assertTrue(DownsamplingStrategy.entries.contains(DownsamplingStrategy.AUTO)) - } - - @Test - fun `DownsamplingStrategy contains NONE`() { - assertTrue(DownsamplingStrategy.entries.contains(DownsamplingStrategy.NONE)) - } - - @Test - fun `DownsamplingStrategy contains FIT`() { - assertTrue(DownsamplingStrategy.entries.contains(DownsamplingStrategy.FIT)) - } - - @Test - fun `DownsamplingStrategy contains FILL`() { - assertTrue(DownsamplingStrategy.entries.contains(DownsamplingStrategy.FILL)) + fun `DownsamplingStrategy contains all expected values`() { + val expected = listOf( + DownsamplingStrategy.AUTO, + DownsamplingStrategy.NONE, + DownsamplingStrategy.FIT, + DownsamplingStrategy.FILL + ) + expected.forEach { value -> + assertTrue(DownsamplingStrategy.entries.contains(value)) + } }
68-69: Consider verifying complete enum ordering.The test only checks that AUTO has ordinal 0. You might want to validate the complete ordering of all enum values to ensure the sequence remains stable.
🔎 Suggested enhancement
@Test - fun `AUTO is first in enum`() { + fun `enum values have expected ordinals`() { assertEquals(0, DownsamplingStrategy.AUTO.ordinal) + assertEquals(1, DownsamplingStrategy.NONE.ordinal) + assertEquals(2, DownsamplingStrategy.FIT.ordinal) + assertEquals(3, DownsamplingStrategy.FILL.ordinal) }landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/decoder/AnimatedImageDetectorTest.kt (1)
145-205: Consider documenting placeholder values.The PNG helper functions use placeholder CRC values (all zeros), which is acceptable for detection logic testing since the detector only checks chunk signatures. Adding a brief comment explaining this choice would clarify intent for future maintainers.
Example documentation
private fun buildPngWithoutAnimation(): ByteArray { - // PNG signature + IHDR chunk + IDAT chunk (simplified) + // PNG signature + IHDR chunk + IDAT chunk (simplified) + // Note: CRC values are placeholders (0x00) since detection only checks chunk names return byteArrayOf(landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/request/RequestManagerTest.kt (1)
1-2: Consider updating the copyright year range.The copyright header shows "2020-2023" but this PR is from 2025. Consider updating to "2020-2025" for consistency.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/scheduler/PrioritizedRequestTest.kt (1)
22-184: Optional: Consider test helper functions for request creation.The test suite provides excellent coverage of PrioritizedRequest and DecodePriority behavior. To reduce duplication in request instantiation across multiple tests, consider extracting a helper function:
private fun createTestRequest( id: String = "test-id", priority: DecodePriority = DecodePriority.NORMAL, createdAt: Long = 0L, tag: String? = null, decoder: () -> Any = { } ) = PrioritizedRequest(id, priority, createdAt, tag, decoder)This would make individual tests more concise while preserving clarity. However, the current explicit approach is also valid for test independence and readability.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/DataSourceTest.kt (1)
24-73: Consider simplifying enum tests.These tests comprehensively verify Kotlin's built-in enum behavior (valueOf, name, ordinal, entries). While they ensure API stability and catch unexpected changes, most of this functionality is guaranteed by the language. Consider consolidating into fewer tests focused on the enum's actual contract:
- Combine the individual "contains X" tests (lines 31-49) into a single test that verifies all expected values exist
- The valueOf and name tests could potentially be parametrized or combined
However, if the goal is to maintain strict API guarantees or achieve 100% coverage for the core module, these tests serve that purpose.
Example consolidated approach
@Test - fun `DataSource has four values`() { - val values = DataSource.entries - - assertEquals(4, values.size) - } - - @Test - fun `DataSource contains MEMORY`() { - assertTrue(DataSource.entries.contains(DataSource.MEMORY)) - } - - @Test - fun `DataSource contains DISK`() { - assertTrue(DataSource.entries.contains(DataSource.DISK)) - } - - @Test - fun `DataSource contains NETWORK`() { - assertTrue(DataSource.entries.contains(DataSource.NETWORK)) - } - - @Test - fun `DataSource contains UNKNOWN`() { - assertTrue(DataSource.entries.contains(DataSource.UNKNOWN)) - } + fun `DataSource has expected values`() { + val expected = setOf( + DataSource.MEMORY, + DataSource.DISK, + DataSource.NETWORK, + DataSource.UNKNOWN + ) + + assertEquals(expected, DataSource.entries.toSet()) + }landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/CachePolicyTest.kt (1)
64-78: Consider consolidating built-in enum behavior tests.Similar to DataSourceTest, the valueOf and name tests verify Kotlin's built-in enum functionality. While thorough, these could be consolidated or simplified since the language guarantees this behavior. However, they serve a purpose for API stability verification.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/ImageResultTest.kt (1)
54-54: Consider using assertNotNull for clearer failure messages.The force unwrap
!!works but provides a less informative error message if rawData is unexpectedly null. Consider usingassertNotNullfor better debugging.Alternative approach
- assertTrue(rawData.contentEquals(success.rawData!!)) + assertNotNull(success.rawData) + assertTrue(rawData.contentEquals(success.rawData))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
landscapist-core/build.gradle.ktslandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/DownsamplingStrategyTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/ImageRequestBuilderTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/LandscapistConfigTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/cache/CacheKeyTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/cache/LruMemoryCacheTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/cache/TwoTierMemoryCacheTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/decoder/AnimatedImageDetectorTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/event/EventListenerTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/event/ImageLoadEventTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/CachePolicyTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/DataSourceTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/ImageResultTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/request/DisposableTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/request/RequestManagerTest.ktlandscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/scheduler/PrioritizedRequestTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- landscapist-core/build.gradle.kts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: API check
- GitHub Check: Spotless check
- GitHub Check: Build and Tests
🔇 Additional comments (24)
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/request/DisposableTest.kt (1)
29-93: LGTM! Comprehensive JobDisposable test coverage.The test suite thoroughly verifies JobDisposable behavior including id matching, disposal states for active/cancelled/completed jobs, explicit disposal via
dispose(), and awaiting completion. All tests properly clean up resources and use appropriate coroutine testing patterns.landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/LandscapistConfigTest.kt (1)
26-163: LGTM! Comprehensive test coverage for configuration APIs.The test suite thoroughly validates the configuration classes with appropriate coverage of:
- Default values for all configuration types
- Custom value preservation
- Nested configuration behavior
- Data class copy semantics
The tests are well-structured, clearly named, and use meaningful assertions.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/event/ImageLoadEventTest.kt (1)
23-252: LGTM! Excellent test coverage for the event hierarchy.The test suite provides comprehensive validation of the ImageLoadEvent API:
- Individual tests for all 12 event types verify correct property storage
- Collection test validates polymorphic behavior across the hierarchy
- Exhaustive when expression test ensures complete handling of all cases
- Default timestamp behavior is validated
The tests are well-organized and provide strong guarantees about the event system's correctness.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/event/EventListenerTest.kt (1)
38-207: LGTM! Comprehensive coverage of EventListener implementations.The test suite thoroughly validates:
- NONE listener and factory behavior
- LoggingEventListener with single and multiple events
- CompositeEventListener dispatch and factory composition, including edge cases
- MetricsEventListener state tracking for cache hits, durations, data sources, and reset behavior
The tests provide strong guarantees about the correctness of the event listener system.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/cache/CacheKeyTest.kt (1)
1-165: Excellent test coverage for CacheKey!The test suite comprehensively validates CacheKey behavior including:
- Hash consistency and uniqueness
- Suffix generation for dimensions and transformations
- Memory key composition
- Factory method variants
- Equality semantics
The tests are well-structured, focused, and use clear naming conventions.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/cache/TwoTierMemoryCacheTest.kt (1)
1-263: Strong test coverage for TwoTierMemoryCache!The test suite thoroughly validates two-tier cache behavior including strong/weak cache interactions, LRU eviction, size management, and configuration variants. The helper methods provide clean test data construction.
Note: The weak reference cleanup test (lines 230-242) only verifies the method doesn't crash, as forcing GC in tests isn't feasible. The comment appropriately documents this limitation.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/ImageRequestBuilderTest.kt (1)
1-241: Comprehensive builder test coverage!Excellent test suite that thoroughly validates:
- Default value initialization
- Individual builder methods
- Fluent chaining behavior
- Header immutability (lines 188-199)
- CachePolicy read/write semantics
- DecodePriority ordering
The tests demonstrate good practices including immutability verification and comprehensive builder pattern validation.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/cache/LruMemoryCacheTest.kt (1)
1-245: Excellent LRU cache test suite!The tests comprehensively cover LruMemoryCache behavior including:
- Basic CRUD operations
- Size and count tracking
- LRU eviction logic (lines 157-176 effectively validate that access patterns affect eviction)
- Value replacement with size updates
- Dynamic trimming and resizing
The helper methods provide clean, reusable test data construction. Well done!
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/decoder/AnimatedImageDetectorTest.kt (2)
24-141: Comprehensive test coverage!The test suite thoroughly covers animated image detection across GIF, APNG, and WebP formats, including both MIME type and signature-based detection. Edge cases like empty and small data are also tested.
207-247: WebP test data correctly structured.The WebP helper functions correctly construct RIFF/WEBP format with the animation flag (0x02) properly set in the VP8X chunk for animated WebP. The distinction between animated (VP8X with flag) and static (VP8 without VP8X) is accurately represented.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/request/RequestManagerTest.kt (3)
29-48: Excellent test coverage for initial state and registration.These tests correctly verify the manager starts empty and that registration produces a valid disposable with the expected ID format. The use of
runTestand proper job cleanup is appropriate.Note: Line 44 asserts on the specific ID format prefix ("request-"), which couples the test to an implementation detail. This is acceptable for unit tests but consider whether this assertion adds sufficient value versus the brittleness if ID generation changes.
50-127: Comprehensive cancellation test coverage.Excellent test coverage of all cancellation scenarios:
- By ID (positive and negative cases)
- By tag (selective cancellation verified)
- By model
- All active requests
Each test correctly verifies both return values and side effects (job cancellation state), and includes proper resource cleanup.
129-211: Well-designed query and lifecycle tests.These tests provide excellent coverage of:
- Active request queries (by model, all IDs, by tag)
- Automatic cleanup on job completion using
Job().complete()andjoin()- Request ID uniqueness across 100 registrations
The use of
Job()factory (which returnsCompletableJob) and callingcomplete()followed byjoin()is the correct approach for testing the automatic removal mechanism.landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/scheduler/PrioritizedRequestTest.kt (6)
24-43: Excellent priority hierarchy coverage.The DecodePriority tests clearly validate the priority ordering and boundary values. The slight overlap between the first two tests enhances readability and makes each test's intent explicit.
45-72: Property storage tests are solid.These tests correctly verify that PrioritizedRequest stores all constructor parameters, including the nullable tag field. Not executing the decoder lambda in these tests is appropriate since they focus on storage semantics.
74-128: Comparison logic thoroughly validated.These tests clearly demonstrate the expected ordering behavior: priority takes precedence, with FIFO ordering for equal priorities. The IMMEDIATE priority test nicely confirms that timestamp is irrelevant for the highest priority level.
130-146: CompareTo semantics correctly verified.This test appropriately confirms that requests with identical priority and timestamp compare as equal (return 0) regardless of decoder instance, which is correct for priority-queue ordering. The test focuses on compareTo rather than structural equality, which aligns with the Comparable contract.
148-165: Comprehensive sorting validation.This integration test excellently validates the complete ordering behavior, including the FIFO guarantee for requests with equal priority. The explicit assertions and inline comment (line 161) make the expected behavior crystal clear.
167-183: Copy semantics properly tested.The test correctly verifies that copy() creates a new instance with the updated priority while preserving other properties.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/CachePolicyTest.kt (2)
32-62: Excellent coverage of CachePolicy semantics.These tests thoroughly verify the custom
readEnabledandwriteEnabledproperties for each policy variant, which is the core behavior of this enum. The test cases correctly validate the expected read/write permissions matrix.
80-96: Good use of higher-level matrix validation.These matrix tests provide valuable verification by filtering and confirming the complete set of policies with each capability. This catches potential issues if the enum is extended or modified in the future.
landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/model/ImageResultTest.kt (3)
36-58: Comprehensive property verification.This test thoroughly validates that all Success properties are correctly stored and accessible, including edge cases like rawData byte array comparison. Well done.
99-183: Excellent equality and hashCode coverage.These tests thoroughly verify the custom equals implementation with multiple edge cases:
- Identical rawData byte arrays (structural equality)
- Different rawData content
- Null rawData handling
- Mixed null/non-null scenarios
- hashCode consistency with equals contract
This is exemplary testing for data classes with array properties.
239-255: Good practice for sealed class exhaustiveness.This test verifies that all sealed subtypes are covered in when expressions, which is valuable for catching issues if new subtypes are added in the future. The test structure ensures compilation fails if a new subtype is added without updating the when expression.
1 participant
Introduce landscapist-core and landscapist-image modules.
landscapist-core
The foundation layer that handles the actual image loading pipeline:
landscapist
The Compose UI layer built on top of core:
landscapist-image
The unified image composable that combines everything:
Summary by CodeRabbit
New Features
API
Build
Tests
✏️ Tip: You can customize this high-level summary in your review settings.