Skip to content
New issue

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

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

Already on GitHub? # to your account

Performance of isAttachmentInLocalStore #1613

Closed
2 tasks done
adrianvintu opened this issue Nov 16, 2023 · 5 comments
Closed
2 tasks done

Performance of isAttachmentInLocalStore #1613

adrianvintu opened this issue Nov 16, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@adrianvintu
Copy link

Checklist

  • I could not find a solution in the documentation, the existing issues or discussions.
  • I already asked for help in the chat

In which Project did the bug appear?

Famedly App

If you selected "Other" as Project, please enter in which project the bug occurred.

No response

On which platform did the bug appear?

Android

SDK Version

matrix: ^0.22.7

Describe the problem caused by this bug

I am using if (await _event.isAttachmentInLocalStore(getThumbnail: true)) for each chat bubble, to show a download icon/button or not, in that chat bubble.

To my surprise, isAttachmentInLocalStore actually loads the file into memory uint8list = await database.getFile(mxcUrl); making the method similar to downloadAndDecryptAttachment.

The performance is terrible.

For example, I have 10 chat bubbles which are all file attachments - one is 19 MB. I can easily see the flickering of the download icon appearing and disappearing because of if (await _event.isAttachmentInLocalStore(getThumbnail: true)) which loads the whole file into memory.
Battery is also quickly drained.

  1. Can you make the method isAttachmentInLocalStore return the correct result without loading the whole file into memory?

  2. Secondly, when I upload a file from the device and run if (await _event.isAttachmentInLocalStore(getThumbnail: true)) I get... an exception: "This event hasn't any attachment or thumbnail.".
    Why is the code breaking and not handling uploads elegantly and correctly?

Steps To Reproduce

No response

Screenshots or Logs

No response

Security related

No response

@adrianvintu adrianvintu added the bug Something isn't working label Nov 16, 2023
@krille-chan
Copy link
Contributor

Thanks for the issue. If you only want to know if an event has something you can download, I would more suggest to look at the event type. This can even be done synchroniously.

The method you have mentioned seems to be not correct. However as far as I can see it is not in use in the sdk, or fluffychat or famedly and I don't see the point keeping it at all. To know if the user can download something, we should look at the event type and to download we have a download method which also returns it from cache. I would suggest that we replace the method with a simple bool getter which leads to a database method bool isFileCached(String mxcUrl).

@adrianvintu
Copy link
Author

adrianvintu commented Nov 17, 2023

Than you for your feedback @krille-chan!

If you only want to know if an event has something you can download
->
I need to know it the file is cached or not, not if there is something we can downlod.

I would suggest that we replace the method with a simple bool getter which leads to a database method bool isFileCached(String mxcUrl).

This is kinda what I need! Something like event.isFileCached(no params). Can you implement that?

@krille-chan
Copy link
Contributor

This is kinda what I need! Something like event.isFileCached(no params). Can you implement that?

Actually I would more suggest that you implement this in your own app. You probably have overwritten the Database like this is done in FluffyChat to provide features on the Flutter platform: https://github.com/krille-chan/fluffychat/blob/main/lib/utils/matrix_sdk_extensions/flutter_hive_collections_database.dart

Then you also have implemented your own way to get and store files. Just add another method to check if a file exists and you are done.

@krille-chan
Copy link
Contributor

Even if you are using the new Matrix SDK Database which has some basic support for file storage, you can implement this by yourself by overwriting it in your app

@krille-chan krille-chan closed this as not planned Won't fix, can't repro, duplicate, stale Nov 20, 2023
@adrianvintu
Copy link
Author

adrianvintu commented Nov 20, 2023

Thank you, I have extended DatabaseApi with Future<bool> fileExists(Uri mxcUri) async

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants