Skip to content
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

Enable SDL2 MP3 support for Android #889

Closed
wants to merge 2 commits into from

Conversation

AndreMiras
Copy link
Member

We had to copy the external/smpeg2-2.0.0/ folder
to the jni/ folder to fix linker errors.
See https://groups.google.com/forum/#!topic/kivy-users/N4isIjQ1Ja8

We had to copy the external/smpeg2-2.0.0/ folder
to the jni/ folder to fix linker errors.
See https://groups.google.com/forum/#!topic/kivy-users/N4isIjQ1Ja8
@AndreMiras
Copy link
Member Author

I wrote "SDK2" in the commit but I meant "SDL2".
It adds MP3 support to Android. It was a lot of investigations because I'm new to the Android toolchain and all, but it doesn't actually do much.
It seems the "external" library must be copied to the jni/ folder if we want it to be linked. This is what it does.

patches = ['toggle_modplug_mikmod_smpeg_ogg.patch']
patches = ['toggle_modplug_mikmod_ogg.patch']

def _external_dir(self):
Copy link

@Fak3 Fak3 Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you split it into 4 methods? It would be more clear if you put it right in prebuild_arch()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Fak3, thanks for your feedback.
I used "inline methods" because I find it more clear to read, but this is my point of view.
For example when I read the prebuild_arch() method. On first sight to the self._copy_smpeg2_to_jni() line, I can quickly say, OK, the prebuild_arch() just copies the smpeg2 lib to the jni directory. And if I want to see in more details how this was achieved, I can step deeper into the stack.
On the other hand, if I had written:

external_dir = join(self.get_build_dir(None), 'external')
smpeg2_external_dir = join(self._external_dir(), 'smpeg2-2.0.0')
jni_dir = self.get_jni_dir()
shprint(sh.cp, '-r', smpeg2_external_dir, jni_dir)
super(LibSDL2Mixer, self).prebuild_arch(arch)

I fell like it takes more time to understand what the method just does, event though it's a very simple method. It's a little bit like the import antigravity thing.
It's also a way to avoid commenting each single line knowing that comments are future lies.
But again this my point of view and if it's an issue I can do another pull request, merging all these lines into a single method with some comments.
Does the fix work for you by the way?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that a comment with 2 lines of code will be much easier to read than 3 methods.

# Copy the smpeg2 to the jni dir
smpeg2_dir = join(self.get_build_dir(None), 'external/smpeg2-2.0.0/')
shprint(sh.cp, '-r', smpeg2_dir, self.get_jni_dir())

I have not yet tried it.

@AndreMiras AndreMiras closed this Oct 6, 2018
@AndreMiras AndreMiras deleted the android_mp3_support branch October 6, 2018 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants