android icon indicating copy to clipboard operation
android copied to clipboard

Thumbnail support

Open JustFanta01 opened this issue 1 year ago • 6 comments

Hello guys 👋 This is our proposal for implementing thumbnail support [/issues/41] Me, @WheelyMcBones and @taglioIsCoding have made the following changes.

We have implemented a uniform solution that works both for local and the remote clouds, this solution exploits the DiskLruCache in the CryptoImplDecorator and in the CryptoImplVaultFormat(Pre)7. We have decided these two location because we have access to all necessary informations: the decrypted image and the cloud type.

In cache we save a thumbnail when someone reads an image file and retrieve it from the same cache during the listing process. Thumbnails are stored as decrypted files in cache and, unlike other files in the /decrypted folder, these are persistent until the cache is deleted. We added the attribute ".thumbnail" in the CryptoFile pointing to the file in the disk cache and the CloudFileModel wraps it around. We also added the Preference in the Settings for when it is supposed to generate the thumbnails. Finally we got rid of the full duplication of the image by elaborating the thumbnail in stream with an ad-hoc Thread pool.

JustFanta01 avatar May 08 '24 14:05 JustFanta01

Walkthrough

Adds configurable thumbnail generation UI and strings, implements per-cloud-type thumbnail LRU caching and on-demand thumbnail generation in the data layer, and integrates thumbnail association and download/processing workflows into file-browsing presenter and UI. UI changes include new preference entries, scroll-triggered thumbnail association for visible nodes, methods to replace images with a download icon, adapter support for showing thumbnail bitmaps, image-preview OOM handling, and a new thumbnail accessor on CloudFileModel. Manifest updated to request a larger heap.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Thumbnail support" directly and accurately summarizes the primary objective of this pull request. The changeset introduces comprehensive thumbnail generation and display functionality across multiple components, including presenter logic, UI fragment/activity updates, adapter modifications, and core thumbnail generation in the CryptoImplDecorator. A teammate reviewing the git history would immediately understand that this PR implements thumbnail support for both local and remote clouds. The title is concise and specific without being overly technical or containing noise.
Description Check ✅ Passed The pull request description clearly relates to the changeset by explaining the implementation approach for thumbnail support. It identifies the team members involved, describes the technical strategy of using DiskLruCache in specific locations to access decrypted image data and cloud type information, explains how thumbnails are cached persistently, documents the new CryptoFile ".thumbnail" attribute and CloudFileModel wrapper, mentions the user preference in Settings, and notes the thread pool optimization to avoid full image duplication. The description provides meaningful context about the changes rather than being vague or generic.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar May 08 '24 14:05 coderabbitai[bot]

@JustFanta01 Not a review, but a general suggestion: ~For caching you use https://github.com/JakeWharton/DiskLruCache, which is unmaintained since 8 yrs.~ Consider using https://github.com/ben-manes/caffeine instead, which is a modern and quite good caching library.

Edit: my bad, this was the wrong dependency.

infeo avatar May 08 '24 14:05 infeo

Thank you so much for this contribution :heart:, will have a closer look to it on Monday!

Consider using https://github.com/ben-manes/caffeine instead, which is a modern and quite good caching library.

@infeo can you please explain in detail why we should switch from DiskLruCache to Caffeine?

SailReal avatar May 11 '24 18:05 SailReal

@SailReal I withdraw my suggestion^^ First, i thought this was an outdated, unmaintained dependency, but i was wrong. Second, the project already uses this dependency and then it is good practice to use what's already there. And third, the dependency targets Android, so i guess it is also "optimized" for the OS in some way.

Regarding Caffeine: It uses a different algorithm with a statistically higher hit rate. See also https://github.com/ben-manes/caffeine/wiki/Efficiency.

infeo avatar May 13 '24 08:05 infeo

Also there is a bug if you use an svg-file, then BitmapFactory.decodeStream(thumbnailReader, null, options) returns null, thumbnailBitmap is then null (I think we shouldn't even call ThumbnailUtils.extractThumbnail if it is null) and then it crashes when we write to it in thumbnailWriter.write(buff.array(), 0, buff.remaining()) because we didn't even create the file we want to write into

org.cryptomator.domain.exception.FatalBackendException: java.io.IOException: Pipe closed
    	at org.cryptomator.data.cloud.crypto.CryptoImplDecorator.read(CryptoImplDecorator.kt:422)
    	at org.cryptomator.data.cloud.crypto.CryptoCloudContentRepository.read(CryptoCloudContentRepository.kt:95)
    	at org.cryptomator.data.cloud.crypto.CryptoCloudContentRepository.read(CryptoCloudContentRepository.kt:21)
    	at org.cryptomator.data.repository.DispatchingCloudContentRepository.read(DispatchingCloudContentRepository.kt:160)
    	at org.cryptomator.domain.usecases.cloud.DownloadFiles.execute(DownloadFiles.java:32)
    	at org.cryptomator.domain.usecases.cloud.DownloadFilesUseCase$Launcher$2.subscribe(DownloadFilesUseCase.java:99)
    	at io.reactivex.internal.operators.flowable.FlowableFromPublisher.subscribeActual(FlowableFromPublisher.java:29)
    	at io.reactivex.Flowable.subscribe(Flowable.java:14935)
    	at io.reactivex.Flowable.subscribe(Flowable.java:14882)
    	at io.reactivex.internal.operators.flowable.FlowableSubscribeOn$SubscribeOnSubscriber.run(FlowableSubscribeOn.java:82)
    	at io.reactivex.internal.schedulers.ExecutorScheduler$ExecutorWorker$BooleanRunnable.run(ExecutorScheduler.java:288)
    	at io.reactivex.internal.schedulers.ExecutorScheduler$ExecutorWorker.run(ExecutorScheduler.java:253)
    	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
    	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
    	at java.lang.Thread.run(Thread.java:920)
    Caused by: java.io.IOException: Pipe closed
    	at java.io.PipedInputStream.checkStateForReceive(PipedInputStream.java:263)
    	at java.io.PipedInputStream.awaitSpace(PipedInputStream.java:271)
    	at java.io.PipedInputStream.receive(PipedInputStream.java:234)
    	at java.io.PipedOutputStream.write(PipedOutputStream.java:149)
    	at org.cryptomator.data.cloud.crypto.CryptoImplDecorator.read(CryptoImplDecorator.kt:398)
    	at org.cryptomator.data.cloud.crypto.CryptoCloudContentRepository.read(CryptoCloudContentRepository.kt:95) 
    	at org.cryptomator.data.cloud.crypto.CryptoCloudContentRepository.read(CryptoCloudContentRepository.kt:21) 
    	at org.cryptomator.data.repository.DispatchingCloudContentRepository.read(DispatchingCloudContentRepository.kt:160) 
    	at org.cryptomator.domain.usecases.cloud.DownloadFiles.execute(DownloadFiles.java:32) 
    	at org.cryptomator.domain.usecases.cloud.DownloadFilesUseCase$Launcher$2.subscribe(DownloadFilesUseCase.java:99) 
    	at io.reactivex.internal.operators.flowable.FlowableFromPublisher.subscribeActual(FlowableFromPublisher.java:29) 
    	at io.reactivex.Flowable.subscribe(Flowable.java:14935) 
    	at io.reactivex.Flowable.subscribe(Flowable.java:14882) 
    	at io.reactivex.internal.operators.flowable.FlowableSubscribeOn$SubscribeOnSubscriber.run(FlowableSubscribeOn.java:82) 
    	at io.reactivex.internal.schedulers.ExecutorScheduler$ExecutorWorker$BooleanRunnable.run(ExecutorScheduler.java:288) 
    	at io.reactivex.internal.schedulers.ExecutorScheduler$ExecutorWorker.run(ExecutorScheduler.java:253) 
    	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) 
    	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) 
    	at java.lang.Thread.run(Thread.java:920) 
    
ErrorCode: F1D9:OBF0

In this case, the file can not be opened anymore.

Please also test it with further other file types.

SailReal avatar May 14 '24 09:05 SailReal

Wondering when thumbnails will be available for the Android version. App is excellent but not being able to see file in a glance is kind of a fail.

alejandracios avatar Feb 25 '25 14:02 alejandracios

@SailReal CC: @JustFanta01

I would like to inform you ✨ I have discovered issues with this PR's branch, so I have forked JustFanta01's fork and am currently working on it here:

https://github.com/aiya000/cryptomator-android

The work I'm doing first is:

  • Large image files fail to generate thumbnails and display errors. Photos cannot be displayed

And I've submitted a PR to JustFanta01 for this:

https://github.com/JustFanta01/cryptomator-android/pull/8

Anticipating that there might be no response from JustFanta01 to this PR (#533) and my above PR, I contacted you early. Please forgive my rudeness!

Additionally, I plan to continue working on the tasks I've logged here:

https://github.com/aiya000/cryptomator-android/issues


So, if JustFanta01 seems to be busy, would you both be willing to let me take over this PR (#533)?

If this is possible, I would appreciate it if you could let me know the current state! Is it at a stage that requires refactoring?

Also, personally, I feel that thumbnail generation is always performed when accessing photos, even if they have already been generated. (Though I haven't investigated this accurately yet...)

Taking these into account, I would be happy to discuss how to proceed with both of you 🙌

aiya000 avatar Aug 21 '25 10:08 aiya000

NOTE: I'm also implmenting Grid View now.

aiya000 avatar Aug 21 '25 10:08 aiya000

@aiya000 It would be awesome if you could continue working on thumbnails and grid view :green_heart:

Please send us a quick message at [email protected] so that we can add you to our development channel in our chat app. We can then discuss the current state of this feature and how we proceed.

SailReal avatar Aug 21 '25 11:08 SailReal

Hi @aiya000 Responding on behalf of @JustFanta01 and @taglioIsCoding too.

First of all, thank you for your contribution! We are quite busy in this period, but we will do our best to review your PR in the next days.

Then, we would be glad to continue working on these open PRs together with you.

Thanks

cc: @SailReal

WheelyMcBones avatar Sep 02 '25 19:09 WheelyMcBones

@aiya000 It would be awesome if you could continue working on thumbnails and grid view 💚

Please send us a quick message at [email protected] so that we can add you to our development channel in our chat app. We can then discuss the current state of this feature and how we proceed.

@SailReal

Thank you :D I'll send a quick message!

aiya000 avatar Sep 05 '25 06:09 aiya000

Hi @aiya000 Responding on behalf of @JustFanta01 and @taglioIsCoding too.

First of all, thank you for your contribution! We are quite busy in this period, but we will do our best to review your PR in the next days.

Then, we would be glad to continue working on these open PRs together with you.

Thanks

cc: @SailReal

@WheelyMcBones

Thank you so much for replying! I'm glad :D.

Yes, I will do that. First, wait for the PR review I sent to @JustFanta01 , then send a mail that @SailReal said. I would be happy to discuss how to proceed!

aiya000 avatar Sep 05 '25 06:09 aiya000

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 19 '25 15:10 CLAassistant

@SailReal Good morning☀

I noticed a bot comment above saying that the CLA license consent confirmation had not been obtained, so I have now completed the consent. https://github.com/cryptomator/android/pull/533#issuecomment-3419748850

Also, I previously sent a quick message to (the email address above), but I haven't received a reply. Is it okay to leave it as it is?

I look forward to the next commits and the completed PR 🙌✨

aiya000 avatar Oct 20 '25 05:10 aiya000

Also, I previously sent a quick message to (the email address above), but I haven't received a reply.

@aiya000 We responded to your email on the same day that we received it (09/08/2025)

SailReal avatar Oct 20 '25 09:10 SailReal

Also, I previously sent a quick message to (the email address above), but I haven't received a reply.

@aiya000 We responded to your email on the same day that we received it (09/08/2025)

@SailReal Aaah!! That's true, I missed it... I'm sorry. It's a little late, but I've replied...!! thank you!

aiya000 avatar Oct 20 '25 10:10 aiya000