Closed cinder92 closed 11 months ago
As far as android goes, i can help with the following:
1- I have developed RN 0.6+ modules before, and can help with this part.
2- Updating to AndroidX is somewhat trivial, I also can help with that aswell.
3- We have relatively large test group when it comes to android, so testing on android wont be a problem.
4- as for Code improvement, in my opinion this is a very necessary step, the code (on my part) is currently very spaghetti like and needs to be rewritten in a more organised manner.
5- new features need to be added, and I think we can add support for more than just music files as it is not very complicated to do on android, I was thinking of warping the entire AndroidMediaStore
.
@Drazail amazing, no worries, i can be testing on iOS, i have a very little experience on this, so i think it wont be a problem, but maybe take a long time to develop, so, we need to make on same channel as per code and new features, maybe we should list what modules we need an listen to the community
@cinder92
yes, I do agree that its still too soon to talk about the MediaStore thing.
given the feedback we received through previous issues, I gathered these:
1- some methods needs restructuring.
2- pagination: instead of using events we should handle pagination by either index range ( database model ) or batchNumber
and batchSize
( REST model ).
3- sorting.
4- we need to handle cover pictures in a manner which is more performant ( avoid extracting images after the first scan) and also is invisible to the android's mediaScanner
( so they wont show up in user's gallery).
5- export all the constants to avoid any confusion.
We should try to keep the old API compatible with the new one, however, some of the above will undoubtedly introduce some breaking changes.
I do not know how many of the above can be achieved in iOS.
I think we should create a new, clean branch for development of these new features.
given the above, I started refactoring RNAndroidAudioStore
as follows:
getAll:
(options:
{
boolean blurred;
boolean cover,
Uri coverFolder,
int minimumSongDuration,
int batchSize,
int batchNumber,
String sortBy,
boolean sortOrder
}
) ->{ length: int, results: TrackObject[] }
getSongsByPath:
(options:
{
Uri path,
boolean blurred,
boolean cover,
Uri coverFolder,
int minimumSongDuration,
int batchSize,
int batchNumber,
String sortBy,
boolean sortOrder
}
) -> { length: int, results: TrackObject[] }
getAlbums:
(options:
{
String artist,
String song,
int batchSize,
int batchNumber,
String sortBy,
boolean sortOrder
}
) -> { length: int, results: AlbumObject[] }
getArtists:
(options:
{
String album,
String song,
int batchSize,
int batchNumber,
String sortBy,
boolean sortOrder
}
) -> { length: int, results: ArtistObject[] }
getSongs:
(options:
{
String artist,
String album,
int batchSize,
int batchNumber,
String sortBy,
boolean sortOrder
}
) -> { length: int, results: TrackObject[] }
search:
(options:
{
String searchParam
int batchSize,
int batchNumber,
String sortBy,
boolean sortOrder
}
) -> { length: int,
tracks: TrackObject[],
artists: ArtistObject[],
albums: AlbumObject[] }
const Constants = {
SortBy:{
Artist: 'ARTIST',
Album: 'ALBUM',
Title: 'TITLE'
},
SortOrder:{
Ascending: 'ASC',
Descending: 'DESC'
}
}
4- we need to handle cover pictures in a manner which is more performant ( avoid extracting images after the first scan) and also is invisible to the android's mediaScanner ( so they wont show up in user's gallery).
@Drazail I'm hiding cover images from gallery by pointing the coverFolder
property to a directory with a dot at the beginning of its name (like /storage/Documents/.covers/). Maybe this can be made default for coverFolder
?
Also, I had a question regarding the performance issues when covers are extracted. Is it normal native behaviour or is it a React Native bridge overhead issue?
@farshed
About the cover folder, that's exactly the behaviour I went for in v3dev branch of my fork.
Currently we extract the cover ( or query android's database if the entry is present), pipe the data to a file and return the path. Then ( on the js side of things) the image component grabs the file as a source and show it to the user. As you can see, this is not a very efficient way of doing things, doesn't have anything to do with the ReactNative though as the only thing passed over the bridge is the path and not the actual base64 data.
There is a solution I am working on, which goes something like this:
Instead of saving the files and returning the path, we could, in theory, implement a native ui component ( by extending Image from react-native), and pass the binary data directly to that. The final API should look something like this: `
` This should increase the performance significantly as well as eliminating the need for saving the covers locally.
@cinder92 @farshed
I have implemented search
, getAll
, getSongsByPath
and CoverImage
here.
CoverImage
inherits from Image
so all Image
props should work on CoverImage
as well.cover
set to true
, will take care of that in future updates.if you could check it out and give some feedback it would be great.
@cinder92 @farshed
I think the android part of the module is ready for release:
https://github.com/Drazail/react-native-get-music-files/tree/v3dev
@Drazail
That's awesome. Let me know if there's anything I can do to facilitate the release.
@farshed
you can start playing around with the module and find if there are any bugs I've missed. ;-)
@cinder92
I was thinking about spilling different parts of the module into different scopes to keep the platform agnostic APIs similar to previous versions as well as making development of platform specific features easier.
maybe something like this:
"dependencies": {
"@react-native-get-music-files/common": "^3.0.0",
"@react-native-get-music-files/android": "^3.0.0",
"@react-native-get-music-files/ios": "^3.0.0",
"@react-native-get-music-files/windows": "^3.0.0",
"@react-native-get-music-files/expo": "^3.0.0"
}
/common
will include platform agnostic methods for instance the getAll
method/android
, /ios
and /windows
will include platform specific methods, such as getSongByPath
for android/expo
will include expo friendly methodsplease let me know what you think about this idea.
@Drazail MusicFiles
is null
when imported as import MusicFiles from "react-native-get-music-files-v3dev-test";
RNAndroidAudioStore
works successfully in the same project. Is there a different procedure for linking react-native-get-music-files-v3dev-test
?
@Drazail nvm. I just rebuilt the project and it works now.
@Drazail
It seems to be showing one cover for many songs, even though all of them have their own. I'm using CoverImage
and fetching data via getAll()
with cover = false
.
Also how do you plan to handle the absence of albumArt?
Edit: Some more information about the bug that might help you track down the cause. The repeating cover in the screenshot is actually the cover for the very first track in the list (when sorted by Title in Ascending order). The list has ~400 tracks and roughly 40 - 50% of them seem to have this problem.
@farshed
about absence of album art, it should behave the same as an Image component with invalid source URI, ie: passing a placeholder image should do the trick.
about the bug you mentioned, are you using flatList? also can you check if the bug persists for images not in the list? does getAll return correct URIs? and please share the code snippet for this list if possible.
@Drazail
Yes, I'm using a FlatList.
I'm sorry I don't get what you mean by "images not in the list".
Yes, getAll
returns the correct URIs since all the songs can be played correctly. The problem is only with the covers.
renderItem method of the FlatList
const RenderTrack = React.memo(
(props) => {
const [isModalVisible, setOptionsModal] = useState(false);
const { item, currentTrack, playlistRemoveOption } = props;
return (
<MainWrapper>
<CoverImage src={item.url} width={50} height={50} />
<TextWrapper>
<Title numberOfLines={1} current={item.id === currentTrack.id}>
{item.title}
</Title>
<Artist numberOfLines={1}>{item.artist}</Artist>
</TextWrapper>
<StyledIcon {...optionsIcon} />
<OptionsModal
selectedTrack={item}
isVisible={isModalVisible}
onPressCancel={() => setOptionsModal(false)}
playlistRemoveOption={playlistRemoveOption}
/>
</MainWrapper>
);
}
);
Another thing I noticed is that the FlatList is very choppy when using CoverImage
. For context, I'm comparing it with the one that renders the data returned by RNAndroidAudioStore.getAll
(which is significantly smoother). I'm testing on a real device using the release build.
@farshed
I meant does the Image component work properly outside of the flatList?
The choppiness I guess is due to the extraction taking place while flatList trying to load the components ( which is typically during scroll). let me move that process to a new thread and see if that helps.
@Drazail
The result is same outside the FlatList i.e. it displays the same cover that it displays while being inside the list.
Another thing that might be of importance is that CoverImage
doesn't seem to re-render when src
prop changes.
@farshed yep, seems to be a bug, working on it.
Meanwhile please let me know if you encountered any other bugs with the rest of the module. ( specially the pagination in getAll)
Also can you test without memoization ?
@farshed
Ive published a new version over at npm, can you check if the bugs are still present?
please note that some props have been renamed, you should write something like this:
<CoverImage
source={'/storage/emulated/0/Download/Hurt - Johnny Cash.mp3'}
placeHolder={
'https://cdn2.iconfinder.com/data/icons/Qetto___icons_by_ampeross-d4njobq/256/library-music.png'
}
width={120}
height={120}
/>
@Drazail
The FlatList is no longer choppy and performance of the CoverImage
now seems to be on par with the Image
component 👌
CoverImage
still doesn't show the appropriate cover when source
prop is changed dynamically. For example, when I jump to the next track the whole screen re-renders with the updated information (title, artist) but not the CoverImage
component. I've to pop the screen and open it again for CoverImage
to show the correct cover. This doesn't make sense because children are supposed to render again when their parent re-renders. Btw, I'm testing this with unmemoized components.
Another problem (which wasn't present in the previous version afair) is that certain list items in the FlatList don't show the cover at all, not even the placeholder. They're just empty. This behaviour is random and if I restart the app OR scroll somewhere else and then scroll back to that list item, the previously affected item now shows the cover but some other item doesn't. Almost 10% of the items display this behaviour at a given time.
Hope I was able the explain the issues well enough.
@farshed 2- noted, I was under the impression that when you extend Image, all the default behaviors gets copied over, which clearly isn't the case. 3- I think it happens to large lists. when thread limitations kick in, some threads seem to get interrupted. will try to fix it asap.
@farshed unfortunately as much as I tried, the coverImage component doesn't seem to work. The logs indicates that the source attribute of the ImageView is being set uppon updating props, but the UI isn't being updated. meanwhile, As a workaround, I limited the thread being used by this package to 1 and made it so if the song has already been scanned, the operation should take much less time.
if you got time, please test with cover : true
and check if everything is working as intended.
Ill try to figure out the CoverImage component as soon as possible.
ps. npm : https://www.npmjs.com/package/react-native-get-music-files-v3dev-test
So this works right now for android 10?
@Drazail can i download react-native-get-music-files-v3dev-test and use it in my project?
@Drazail can i download react-native-get-music-files-v3dev-test and use it in my project?
as long as you only target android, it should work fine.
In android 10 not working anymore.
@ajoykarmakar, I'm using it and it works
Play store now require atleast 29 compileSdkVersion. Before 28 also works for mine also. Please try this one, and let me know is it working for you or not.
android/build.gradle
ext {
buildToolsVersion = "29.0.3"
minSdkVersion = 17
compileSdkVersion = 29
targetSdkVersion = 29
supportLibVersion = "29.0.0"
}
By any chance, is it still working ?
@yaitskeshav We do use this library in production, only android though. As far as iOS goes, we decided to drop most of the "local file player" support long ago, so no idea about the iOS side of things.
I used it in my project but the problem is the cover Art doesn't get loaded. It perhaps returns the uri of coverart but when i use it in Image source, it doesn't works at all. Can't open the path returned by the getAll [ cover ] using react native fs module. If it's working for you then please help me with the exact repo which i should clone to get things working.
@yaitskeshav
You can che my fork here. However it is quite a few commits behind what we use in production, so there maybe some compatibility issues with newer android versions.
Closing this in pro of https://github.com/cinder92/react-native-get-music-files/pull/108
This issue is opened in order to update this module for newest RN versions we need to
1 .- Know how to create native modules for RN 0.60+ 2 .- Update packages for Android X 3 .- Much test on iOS 13 4 .- Code improvement 5 .- Test on many devices (i have Android 6 and 9, iOS 13)
Please let me know your suggestions
@Haggaish @flarocca @anthonytietjen @rashidul-hasan @Drazail