Suggestion about using raw, assets, and file path for audio about jcplayer HOT 30 CLOSED

It sounds good.

I have suggestions though:

Instead of doing this:

urls.add(new FileAndOrigin("http://www.villopim.com.br/android/Music_01.mp3", Origin.URL));
urls.add(new FileAndOrigin("http://www.villopim.com.br/android/Music_02.mp3", Origin.URL));
urls.add(new FileAndOrigin(this.getFilesDir() + "/" + "13.mid", Origin.FILE_PATH));
urls.add(new FileAndOrigin("49.v4.mid", Origin.ASSET));
urls.add(new FileAndOrigin("56.mid", Origin.ASSET));
urls.add(new FileAndOrigin(String.valueOf(R.raw.a_203), Origin.RAW));
urls.add(new FileAndOrigin(String.valueOf(R.raw.a_34), Origin.RAW));

I suggest it to be written with Factory pattern and change the name of FileAndOrigin to JcFile,
and it will become like this:

public class JcFile /*FileAndOrigin*/ {
   private String filePath;
   private Origin origin;
   ...
   // getter, setter, and constructor omitted.

  
  public static JcFile createWithRaw(@RawRes int rawId){
    // here is the JcFile creation stuff
    return new JcFile(String.valueOf(R.raw.a_203), Origin.RAW); // So user wont choose origin you will set it here
  }
  public static JcFile createWithAsset(Context con, String assetName){
    // here is the JcFile creation stuff
  }
  // ... and so on
}

Usage:

urls.add(JcFile.createWithRaw(R.raw.a_34));
urls.add(JcFile.createWithUrl("http://www.villopim.com.br/android/Music_01.mp3"));
urls.add(JcFile.createWithAsset(context,"49.v5.mid");
urls.add(JcFile.createWithFile(this.getFilesDir() + "/" + "13.mid"));

This will be much better and less mistaken than letting user choose the origin every time he enters the path.

Thank you

from jcplayer.

Ok guys, I made the changes. I think that it stayed good 😃 Please, pull from the master branch 👍 @joielechong @iballan

from jcplayer.

@iballan: It turns out that static method not use more memory eventhough the object is instantiated many times as in http://softwareengineering.stackexchange.com/questions/306377/does-making-a-method-static-save-memory-on-a-class-youll-have-many-instances-of
But I still think that we need to separate a pojo from its factory. How do you think? What is your opinion @jeancsanchez ?

from jcplayer.

@iballan: That's marvellous! Your suggestion is the best way! Never thought it before. I'll change my fork according to your suggestion. And let's wait for @jeancsanchez decision :).

from jcplayer.

Awesome! Thanks @joielechong and @iballan for suggestions. I will implement and test both suggestions. The more precise implementation will stay on the next library's release. Feel free for send a PR too 👍 :)

from jcplayer.

@joielechong Indeed, I dont use another class usually (like JcFileFactory) in order to let the api be easier to use. Since That functions will not be used by developers, it will be used by end user, i prefer to keep it at the same class (at JcFile). it will be easier for the end user to use the library.
This is my opinion, you may still use the factory class and it will be nice both ways :)

from jcplayer.

@joielechong of course, createFromRaw() and so on... will be better

from jcplayer.

@joielechong I thought just throw an exception when the url is invalid. Then, the dev just need to fix the url. I think that exception must be trown on the dev code and fixed by him, leaving the developer to decide what make when it happen. What do you think?

from jcplayer.

@iballan: Is it better using static method in JcFile for factory pattern instead using one class as factory like JcFileFactory? My major concern here is about memory usage. when we create a JcFile object, each of object will contain the static methods. So we waste a memory space each time JcFile is instanted. Afaik.

@jeancsanchez: is there any coding rules to follow for JcPlayer project? I'll make a pr in a couple of days.

from jcplayer.

@iballan: I agree with you. The api should focus to user instead of developer. For the method name, isn't it better if we use createFromRaw() instead createWithRaw ()? What's your suggestion?

from jcplayer.

What you think about the user just set the string path and the lib check and set the type of the file?
For example:

player.addAudio("Yes! I am a RAW file.", "R.raw.mFile");

@iballan @joielechong

from jcplayer.

@jeancsanchez I guess you want to make a uniformity for adding audio file, am I right? I think it will be better if we stick to one API for creating the audio file. So, instead of

player.addAudio("Yes! I am a RAW file.", "R.raw.mFile");

we could use our previous Factory Pattern:
player.addAudio(JcFile.createFromRaw("Yes! I am a RAW file.", R.raw.mFile);

So user will not confuse when using the API. imho

For checking the file, my opinion is that we just tell the user about not valid file with the previous Exception. Don't interrupt play when file is not valid, just go to the next file. But there is major flaw for it, if all the file isn't valid the player will still continue trying to play the next file. So we need to check if all the file in the playlist error then when we retry to play the same list, we can stop the play and inform user.

let's wait for @iballan

from jcplayer.

@jeancsanchez: I've implemented our discussion here in my fork. But a problem arise. If we add invalid audio file (url, raw, assets, or file path) in JcPlayerView.initWithTitlePlaylist(), JcPlayer will crash. So, how do we handle this? Do we keep the audio file in playlist but in the player it will be skipped when playing? Or we don't add the file if it is invalid.

In the first choice, we need additional code to handle process to skip the invalid file. But this is a good solution because if user audio file in external sdcard and user want to play it when external sdcard is removed, JcPlayer will keep the file in the list so user can keep the playlist and happily playing the file when external sdcard is mounted.

In the second choice, we, the developer, don't need to add additional code. But minus the 'feature' in the first choice.

from jcplayer.

@jeancsanchez: I think that is the better ways to handle the error. How do we inform the developer then? Do we throw the error when adding the audio file or when the player play the file and send the error from JcPlayerService JCPlayerServiceListener interface?

I'll try to send you a pr tonight. Let's discuss the code then ;)

from jcplayer.

@joielechong I think the first option is more flexible for developers, but it is debatable :D

from jcplayer.

@jeancsanchez You make me confuse :P

The first choice is from full fledged Audio Player pov. That's a really nice feature, but it is your decision to choose, do we go that route or another one.

I'll follow your decision ;)

from jcplayer.

Btw, what java code style that you use? So I can stick with your code style to send the pr.

I use https://github.com/square/java-code-styles for my project.

from jcplayer.

@joielechong Sorry haha. I meant that just throw the error when adding the audio file and leaving the dev decide what to do, maybe is a solution more flexible. In others words, the playlist will be created after all urls are valid, else throw an exception. But in the case that you cited about user external sdcard situation, such solution will crash the application probably :/
In this case, I think the JcPlayerService must throw the same exception too.

About the code style, I use the default of the Android Studio.

from jcplayer.

@jeancsanchez Well, that's a good solution. We don't accept invalid path when adding to the playlist. But this solution doesn't work when the developer add the previously working playlist, for example if developer reload the playlist from persistence storage like preference or sqlite. So, in my opinion, we better to throw an error when playing the file. Developer can add invalid file when adding the list, but player will throw error when trying to play an invalid file.

With the current code, the player will not crash when the url is valid though the device not connected to the internet. So we can expand the code to handle invalid file too.

from jcplayer.

@joielechong Nice! You right :D

from jcplayer.

@jeancsanchez I've send a pr at issue #12. Please don't harsh to me :P

from jcplayer.

@jeancsanchez @joielechong great job guys,
i have one question here, after adding file, how to remove it or find it in the playlist?

from jcplayer.

@iballan I never thought about it. :(
I'll check the code and also waiting for @jeancsanchez answer.

from jcplayer.

@joielechong Great job!!! Sorry for take long time for answer it. I am going to merging your code soon, just making some adjusts before pushing to master. Thanks. 🎉 🎈 @iballan On the next push I will send a feature for this problem. Thank you for find out this deficiency.

from jcplayer.

Hello Sorry but i couldn't find the play method to edit the code ? could you please pinpoint the method in the example so i can edit the code i got ? thank you!

from jcplayer.

Hello Jean is it implemented in the compile ? if yes can you put how to use ?

from jcplayer.

@ziadkiwan Hello 😄 Not yet. We need to make some more tests, however, in next week I will make it available a new version for compile.

If you are needing it with emergency, I suggest you to clone the master branch and compile locally in your machine.

from jcplayer.

@jeancsanchez This is great... :D.

I'll make some tests with the code.

from jcplayer.

I've test the code, it's working with removing the audio.

There is one question:
if we removing the current played audio, the player will still play the audio. From my perspective, player should play the next audio. But if the audio is the only audio in the playlist, player should stop itself. How do we address the problem?

from jcplayer.

I'm closing this issue and making my question as a new issue.

from jcplayer.