-
-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for local phone stored images #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rohansohonee , thanks for the contribution and sorry for the delay in reviewing it.
There are a few changes I would like to request before merging it.
For performance reasons when there are many media items, the plugin currently loads artwork lazily when a media item actually becomes current, or alternatively there is an option shouldPreloadArtwork
which which loads all artwork for all media items asynchronously in the background. Your patch for loading local files effectively loads all artwork for all media items synchronously regardless of the shouldPreloadArtwork
option. I can understand this since loading from local files should be fast anyway and not a problem (maybe not if there are hundreds of media items), but if it is fast, then the same reasoning would mean that it would also not be a problem if you loaded artwork from local files in the same way that remote artwork is currently loaded, i.e. lazily on demand, unless the shouldPreloadArtwork
option is enabled. I think this would also simplify your code, so even bitmaps loaded locally would go in the cache and you wouldn't need to make your change in createMediaMetadata
, you could make it in loadArtBitmap
.
Also, as your code is currently written, if multiple media items share the same artUrl
, it will load the same local file multiple times creating multiple bitmaps in memory. If you put them in the cache, it would result in less memory usage.
Besides the above, I only had one other minor comment relating to coding standards. I haven't actually written up any coding standards for the project, but for Dart, I would follow Effective Dart and the /// comments as well as square brackets within them need to follow the Dart Doc spec so that they are picked up correctly by the dartdoc tool. For Java, the only change I would make is to insert a space in if(bitmap == null)
between the if
and the (
.
Hi @ryanheise Please check whether I have made the changes correctly. If not, please do let me know. If what I have done is wrong can you please help out and explain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks on the right track. Although, you're still calling BitmapFactory.decodeFile
before checking the cache, so it should be the other way around. Reverting to the original line of code in loadArtBitmap
:
Bitmap bitmap = artBitmapCache.get(artUri.toString());
I would just insert your code immediately after this line, like this:
if (bitmap == null)
bitmap = BitmapFactory.decodeFile(artUri);
Then you can delete the createBitmap
method.
One more thing is you seem to have a duplicate import for BitmapFactory
.
Hi @ryanheise Thank you for replying so fast and helping me. |
Looks good now, all of it amounts to just a 2 line change after simplifying. By the way, I'm curious (and I should have thought of this earlier), does it work if you use a
Could you have used:
|
Hi @ryanheise The path I am getting for the device stored images is:
Also on android developer page it is mentioned that public static Bitmap decodeFile (String pathName) Decode a file path into a bitmap. If the specified file name is null, or cannot be decoded into a bitmap, the function returns null.
A valid path name should be given I guess for it to work is all. |
Yes, the file scheme will not work with |
I am not sure on how to convert the file path to URI |
In Dart, if MediaItem mediaItem = MediaItem(
id: 'audio_1',
album: 'Sample Album',
title: 'Sample Title',
artist: 'Sample Artist',
artUri: '${Uri.file(path)}'); |
Hi @ryanheise It worked. No need for merging my changes. OMG sorry for all the trouble. |
Cool, thanks for helping test it, and it's good to know file URIs are working (since I had never tested it myself). |
How get filePath from flutter asset or android resources? |
Assets aren't files so they don't have a file path. However, you can copy a Flutter asset's data into a file with Dart code: await file.writeAsBytes((await rootBundle.load(assetPath)).buffer.asUint8List()); If you search the standard Dart libraries, you can find more information on the asset APIs. |
Thanks, I found similar solution yet11 дек. 2019 г. 11:40 пользователь ryanheise <notifications@github.com> написал:Assets aren't files so they don't have a file path. However, you can copy a Flutter asset's data into a file with Dart code:
await file.writeAsBytes((await rootBundle.load(assetPath)).buffer.asUint8List());
If you search the standard Dart libraries, you can find more information on the asset APIs.
—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or unsubscribe.
|
No description provided.