This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• the patch (java-android.diff, text/plain, 2013-03-12 10:12:58 +0000, 51379 bytes)
• extracted the classes (export.diff, text/plain, 2013-03-31 02:37:55 +0000, 28952 bytes)
• fixed warnings (2nd patch) (fix-warnings.diff, text/plain, 2013-03-31 02:38:51 +0000, 1893 bytes)
• protected (3rd patch) (protected.diff, text/plain, 2013-03-31 02:39:09 +0000, 4523 bytes)
• getLayout (4th patch) (get-layout.diff, text/plain, 2013-03-31 02:39:37 +0000, 1072 bytes)
• fixed warnings (5th patch) (warnings.diff, text/plain, 2013-03-31 02:39:57 +0000, 734 bytes)
• small todo comment about a (possible) leak (6th patch) (todo.diff, text/plain, 2013-03-31 02:40:24 +0000, 1036 bytes)
• extracted yet another class (7th patch) (extract-class.diff, text/plain, 2013-03-31 02:40:44 +0000, 3700 bytes)
• one patch to rule them all (sdl-android.diff, text/plain, 2013-04-17 11:09:22 +0000, 51573 bytes)

Reported in version: HG 2.0
Reported for operating system, platform: Android (All), All

Comments on the original bug report:

On 2013-03-12 10:12:23 +0000, Martin Gerhardy wrote:

extracted the android java classes into separate files

made some stuff protected and added a getter to the layout to be able to override it
fixed some warnings in the java code

putting this into separate files allows users to replace single files in their project without conflicting with future updates.

also making stuff protected helps in adding your own activity and just extend SDLActivity (which is the prefered way according to the readme)

putting the layout into a getter allows us to override this, too - e.g. to be able to add ads or something like that to the layout.

On 2013-03-12 10:12:58 +0000, Martin Gerhardy wrote:

Created attachment 1068
the patch

the patch

On 2013-03-30 23:53:14 +0000, Sam Lantinga wrote:

These changes look good, but the patch doesn't cleanly apply.

On 2013-03-31 02:37:55 +0000, Martin Gerhardy wrote:

Created attachment 1086
extracted the classes

On 2013-03-31 02:38:51 +0000, Martin Gerhardy wrote:

Created attachment 1087
fixed warnings (2nd patch)

On 2013-03-31 02:39:09 +0000, Martin Gerhardy wrote:

Created attachment 1088
protected (3rd patch)

On 2013-03-31 02:39:37 +0000, Martin Gerhardy wrote:

Created attachment 1089
getLayout (4th patch)

On 2013-03-31 02:39:57 +0000, Martin Gerhardy wrote:

Created attachment 1090
fixed warnings (5th patch)

On 2013-03-31 02:40:24 +0000, Martin Gerhardy wrote:

Created attachment 1091
small todo comment about a (possible) leak (6th patch)

On 2013-03-31 02:40:44 +0000, Martin Gerhardy wrote:

Created attachment 1092
extracted yet another class (7th patch)

On 2013-03-31 04:34:46 +0000, Martin Gerhardy wrote:

I've now splitted the patch into several smaller commits. They are also made against the latest hg revision

On 2013-03-31 05:16:06 +0000, Sam Lantinga wrote:

I applied these patches and copied the files into an existing project and get errors:

-compile:
[javac] Compiling 6 source files to /Users/slouken/projects/Equate/android-project/bin/classes
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLSurface.java:141: cannot find symbol
[javac] symbol : class OnGenericMotionListener
[javac] location: class android.view.View
[javac] private static class SDLOnGenericMotionListener implements View.OnGenericMotionListener {
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLActivity.java:208: cannot find symbol
[javac] symbol : method getMotionRanges()
[javac] location: class android.view.InputDevice
[javac] List<InputDevice.MotionRange> rangesList = InputDevice.getDevice(deviceIds[i]).getMotionRanges();
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLActivity.java:211: cannot find symbol
[javac] symbol : method getSource()
[javac] location: class android.view.InputDevice.MotionRange
[javac] if ( (range.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0) {
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLActivity.java:211: cannot find symbol
[javac] symbol : variable SOURCE_CLASS_JOYSTICK
[javac] location: class android.view.InputDevice
[javac] if ( (range.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0) {
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLActivity.java:211: incomparable types: boolean and int
[javac] if ( (range.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0) {
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLActivity.java:212: cannot find symbol
[javac] symbol : method getAxis()
[javac] location: class android.view.InputDevice.MotionRange
[javac] axesList.add(range.getAxis());
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLActivity.java:217: cannot find symbol
[javac] symbol : variable AXIS_X
[javac] location: class android.view.MotionEvent
[javac] axesList.add(MotionEvent.AXIS_X);
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLActivity.java:218: cannot find symbol
[javac] symbol : variable AXIS_Y
[javac] location: class android.view.MotionEvent
[javac] axesList.add(MotionEvent.AXIS_Y);
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLSurface.java:49: cannot find symbol
[javac] symbol : method setOnGenericMotionListener(org.libsdl.app.SDLSurface.SDLOnGenericMotionListener)
[javac] location: class org.libsdl.app.SDLSurface
[javac] setOnGenericMotionListener(new SDLOnGenericMotionListener());
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLSurface.java:147: cannot find symbol
[javac] symbol : variable SOURCE_CLASS_JOYSTICK
[javac] location: class android.view.InputDevice
[javac] if ( (event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0) {
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLSurface.java:157: cannot find symbol
[javac] symbol : method getAxisValue(java.lang.Integer,int)
[javac] location: class android.view.MotionEvent
[javac] SDLActivity.onNativeJoy(id, axisIndex, event.getAxisValue(axes.get(axisIndex), actionPointerIndex));
[javac] ^
[javac] /Users/slouken/projects/Equate/android-project/src/org/libsdl/app/SDLSurface.java:142: method does not override or implement a method from a supertype
[javac] @OverRide
[javac] ^
[javac] Note: Some input files use or override a deprecated API.
[javac] Note: Recompile with -Xlint:deprecation for details.
[javac] 12 errors

On 2013-03-31 14:06:17 +0000, Martin Gerhardy wrote:

This does not happen for you with the latest hg rev? This looks more like an old sdk to me. SDL needs api level >= 12 since the joystick stuff went into the repo.

As i haven't changed any code I would really be surprised if my changes would have introduced this.

On 2013-04-02 02:12:32 +0000, Sam Lantinga wrote:

Sorry for the iteration here. I went ahead and reverted the joystick patch that bumped the API revision requirements, but now your patches don't apply cleanly.

Can you provide one last full patch that includes your changes?

Thanks!

On 2013-04-17 11:09:22 +0000, Martin Gerhardy wrote:

Created attachment 1113
one patch to rule them all

On 2013-04-24 08:58:55 +0000, Martin Gerhardy wrote:

I have some other patches in my pipeline that make stuff protected and added more methods that can get overridden by derived classes. But i based these patches upon this patch. Is this going to be applied? Or should I try to create them against the latest rev from mercurial? I'm asking, because there is again a change on the java classes that now already have to get merged into this patch. So if this is not going to be used I would base my further patches not upon this.

But just to be sure. There should be some refactoring in the java classes before SDL2 is released. Because currently it's not possible to extend your view(s) if you derive from SDLActivity. Which makes it almost impossible to use without modifications on the SDL code.

On 2013-04-24 11:43:30 +0000, Gabriel Jacobo wrote:

I hate to do this because I don't like when my patches get shot down :)

BUT! I don't think we should take this in, at least not before SDL 2.0 is released. I do see value in keeping everything SDL needs from the Java side in a single file, even if it gets a bit dirty/against Java etiquette.

I do agree that a few of the private variables need to become protected or even public to make it easier for developers to extend this classes.

On 2013-04-24 12:06:51 +0000, Martin Gerhardy wrote:

The problem with keeping everything in a single file is the merge overhead you get if you only wanna replace one class but not the others.

On 2013-04-24 12:10:18 +0000, Gabriel Jacobo wrote:

If you want to replace a class you inherit from it (provided it's possible, which due to some private variables we have now, it isn't always the case), or do you mean something else?

On 2013-04-24 14:36:10 +0000, Martin Gerhardy wrote:

Btw. what is the value you see in keeping it all in one file? And as you said - it's against the java style. No java developer would expect this stuff in a single file.

And with replacement I mean if you only wanna replace a single class but still want to keep the others it's hard to do this right now, because you can't just merge with upstream in an easy way.

Anyway - it's of course up to you. And if you make stuff protected here, please also add protected methods for e.g. the layout that is used and stuff like that. Please also organize the imports and fix some of the warnings in the code, it fills the problems view in eclipse for no reason.

On 2013-04-28 05:23:22 +0000, Philipp Wiesemann wrote:

(In reply to comment # 14)

Created attachment 1113 [details]
one patch to rule them all

This patch seems to replace all spaces in the Java source with tabs. The remaining SDL source seems to favour spaces. Although it is mostly C and this is Java[1] it may be more consistent to use spaces everywhere (in SDL).

I assume this was some kind of automatic formatting and I think in some areas it did not improve readability (e.g. commented source near onDestroy() or in audioInit()).

I also think that most of these classes are strongly coupled anyway and separating them in multiple files may make them harder to maintain. Though if there is a need to replace single classes completely (why?) then this argument does not hold.
Maybe SDLActivity can provide an interface to allow replacing parts of itself without having the need to be modified itself ("open closed principle", like the getLayout() the patch introduced).

[1] http://source.android.com/source/code-style.html#use-spaces-for-indentation

On 2013-04-29 05:01:48 +0000, Martin Gerhardy wrote:

thanks for applying some of the patches.

I've used the default formatter of a fresh eclipse installation for java (because I think that's what every java developer is using for developing an Android app)

I've done my best to keep future changes as small as possible with the new structure and files - but my main intention was (I do java development for bringing home the bacon) to fulfill the general java-coding guidelines.

I've closed the ticket now.

But i would still like to beg you to make all the private members of SDLActivity at least protected. Also sendCommand might be useful.

On 2013-05-01 04:11:09 +0000, Philipp Wiesemann wrote:

The following source seems to be a working possibility to wrap the Views of SDLActivity without changing its current interface:

public class MyGame extends SDLActivity {

@Override
public void setContentView(View view) {
    mine = new LinearLayout(this); // create new root to wrap SDL view
    text = new TextView(this); text.setText("wrapping views example");
    sdls = view; // save last View set by SDLActivity if needed at all
    mine.addView(text);
    mine.addView(sdls);
    super.setContentView(mine);
}

I have not tested this much so maybe there is a flaw I missed. Also you need to be careful when calling setContentView() multiple times (maybe add additional checks). But if it is working it should be a better solution than a public "mLayout" and similar fragile (if at all) to "getLayout()".