Add image caching to SmallImageCard #319

ishita-gambhir-adobe · 2025-01-07T10:33:31Z

Description

Previously the images were always downloaded from the url, added cache functionality. Now, the image will be fetched from cache if present, otherwise it will be downloaded and cached for future use.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2025-01-07T10:48:14Z

Codecov Report

Attention: Patch coverage is 56.92308% with 28 lines in your changes missing coverage. Please review.

Project coverage is 82.23%. Comparing base (f1c6246) to head (e5398ed).

Files with missing lines	Patch %	Lines
...keting/mobile/messaging/ContentCardImageManager.kt	56.25%	22 Missing and 6 partials ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##             feature/content-cards     #319      +/-   ##
===========================================================
- Coverage                    82.78%   82.23%   -0.56%     
  Complexity                     800      800              
===========================================================
  Files                           78       79       +1     
  Lines                         3701     3765      +64     
  Branches                       572      583      +11     
===========================================================
+ Hits                          3064     3096      +32     
- Misses                         459      483      +24     
- Partials                       178      186       +8

Flag	Coverage Δ
functional-tests	`30.17% <0.00%> (-0.61%)`	⬇️
unit-tests	`81.91% <56.92%> (-0.56%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rymorale · 2025-01-08T16:46:00Z

...messaging/src/main/java/com/adobe/marketing/mobile/aepcomposeui/components/SmallImageCard.kt

-            UIUtils.downloadImage(imageUrl) {
+            val contentCardImageManager = ContentCardImageManager()
+
+            contentCardImageManager.getContentCardImageBitmap(imageUrl) {


nit: can we define the getContentCardImageBitmap function in a companion object within ContentCardImageManager so there is no need to create a ContentCardImageManager object just to call getContentCardImageBitmap?

rymorale · 2025-01-08T16:47:06Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/ContentCardImageManager.kt

+import java.io.InputStream
+import java.nio.ByteBuffer
+
+class ContentCardImageManager() {


nit: the () can be removed after the class name

rymorale · 2025-01-08T16:56:10Z

...messaging/src/main/java/com/adobe/marketing/mobile/aepcomposeui/components/SmallImageCard.kt

                it.onSuccess { bitmap ->
                    imageBitmap = bitmap
                    isLoading = false
                }
                it.onFailure {
-                    // TODO once we have a default image, we can use that here
+                    // todo - confirm default image bitmap to be used here
+                    // imageBitmap = contentCardManager.getDefaultImageBitmap()


this function can also be added to the companion object so it can be called statically.

rymorale · 2025-01-08T17:03:44Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/ContentCardImageManager.kt

+                byteArray.inputStream() // Create InputStream from byte array
+            }
+
+            val cacheEntry = CacheEntry(imageInputStream, CacheExpiry.never(), null)


we should set a cache expiry here as content cards will be refreshed often. @navratan-soni and @PravinPK what do you guys think is the ideal expiry time we should set? i'm thinking a 7 day expiry would be fine.

rymorale · 2025-01-08T17:05:57Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/ContentCardImageManager.kt

+                Log.warning(
+                    MessagingConstants.LOG_TAG,
+                    SELF_TAG,
+                    "getImageBitmapFromCache: Unable to read cached data into a bitmap due to error: $e"


nit: we usually use a - between the function name tag and the log message

rymorale · 2025-01-08T17:06:21Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/ContentCardImageManager.kt

+            Log.warning(
+                MessagingConstants.LOG_TAG,
+                SELF_TAG,
+                "getImageBitmapFromCache: Unable to read cached data as the inputStream is null"


nit: we usually use a - between the function name tag and the log message

rymorale · 2025-01-08T17:07:16Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/ContentCardImageManager.kt

+            cacheService?.set(cacheName, imageName, cacheEntry)
+
+            return true
+        } catch (ex: java.lang.Exception) {


there is no need to provide the full Exception package path here:

} catch (e: Exception) {

rymorale · 2025-01-08T17:09:04Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/ContentCardImageManager.kt

+            Log.warning(
+                MessagingConstants.LOG_TAG,
+                SELF_TAG,
+                "cacheImage - An unexpected error occurred while caching the downloaded image: \n %s",


let's use the kotlin string interpolation format:

Log.warning( MessagingConstants.LOG_TAG, SELF_TAG, "cacheImage - An unexpected error occurred while caching the downloaded image: $e"

rymorale · 2025-01-08T17:10:23Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/MessagingConstants.java

@@ -21,6 +21,8 @@ public final class MessagingConstants {
    static final String CACHE_BASE_DIR = "messaging";
    static final String PROPOSITIONS_CACHE_SUBDIRECTORY = "propositions";
    static final String IMAGES_CACHE_SUBDIRECTORY = "images";
+    static final String CONTENT_CARD_CACHE_SUBDIRECTORY = "contentCardImages";
+    static final String CONTENT_CARD_TEST_CACHE_SUBDIRECTORY = "contentCardTestImages";


let's add these constants to MessagingTestConstants instead of here

rymorale · 2025-01-08T17:12:16Z

...messaging/src/test/java/com/adobe/marketing/mobile/messaging/ContentCardImageManagerTests.kt

+    }
+
+    @Test
+    fun get_image_first_time_when_not_in_cache() {


nit: can we name the tests with more readable names like try getting an image when it is not in the cache? back ticks can be used to add spaces to test method names: https://kotlinlang.org/docs/coding-conventions.html#names-for-test-methods

navratan-soni · 2025-01-09T12:50:17Z

code/messaging/src/main/java/com/adobe/marketing/mobile/messaging/ContentCardImageManager.kt

+import java.io.InputStream
+import java.nio.ByteBuffer
+
+class ContentCardImageManager {


Whole class can be made as an object instead of class, and we can remove the companion object.

navratan-soni

Two points here.

If all the methods are in companion object, can we remove the companion object and just create the whole class as Object.
My preferred approach would be to create instance of this Manager class, which takes the input of cache name in constructor itself. That way we will not need to pass the name of cache in all the methods.

rymorale · 2025-01-09T17:20:54Z

the Verify UI testing screenshots / verify-screenshots error can be resolved later

ishita-gambhir-adobe added 2 commits December 22, 2024 12:10

cache content card images

9a03f03

integrate caching in smallImageCard

d30ad46

ishita-gambhir-adobe requested a review from navratan-soni January 7, 2025 10:33

ishita-gambhir-adobe added 3 commits January 8, 2025 21:29

add unit tests

506465f

spotless apply

7437646

Merge branch 'feature/content-cards' into imageCache

5971097

rymorale reviewed Jan 8, 2025

View reviewed changes

ishita-gambhir-adobe added 5 commits January 9, 2025 12:37

address PR comments

46a2b9c

fix failing tests

03b24bb

run spotless apply

a013405

add additional UTs

aabb1aa

spotless apply

e5398ed

navratan-soni reviewed Jan 9, 2025

View reviewed changes

navratan-soni requested changes Jan 9, 2025

View reviewed changes

convert contentCardImageManager to object

d7e48ae

ishita-gambhir-adobe requested review from rymorale and navratan-soni January 9, 2025 16:19

navratan-soni approved these changes Jan 9, 2025

View reviewed changes

rymorale approved these changes Jan 9, 2025

View reviewed changes

ishita-gambhir-adobe merged commit ff6d1b5 into adobe:feature/content-cards Jan 9, 2025
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add image caching to SmallImageCard #319

Add image caching to SmallImageCard #319

ishita-gambhir-adobe commented Jan 7, 2025

codecov bot commented Jan 7, 2025 •

edited

Loading

rymorale Jan 8, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025

rymorale Jan 8, 2025

navratan-soni Jan 9, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025

ishita-gambhir-adobe Jan 9, 2025

rymorale Jan 8, 2025 •

edited

Loading

ishita-gambhir-adobe Jan 9, 2025

navratan-soni Jan 9, 2025

navratan-soni left a comment

rymorale commented Jan 9, 2025

Add image caching to SmallImageCard #319

Add image caching to SmallImageCard #319

Conversation

ishita-gambhir-adobe commented Jan 7, 2025

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Jan 7, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymorale Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navratan-soni left a comment

Choose a reason for hiding this comment

rymorale commented Jan 9, 2025

codecov bot commented Jan 7, 2025 •

edited

Loading

rymorale Jan 8, 2025 •

edited

Loading