Created attachment 1068 [details]
the patch

the patch

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

Created attachment 1086 [details]
extracted the classes

Created attachment 1087 [details]
fixed warnings (2nd patch)

Created attachment 1088 [details]
protected (3rd patch)

Created attachment 1089 [details]
getLayout (4th patch)

Created attachment 1090 [details]
fixed warnings (5th patch)

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

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

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

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

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.

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!

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

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.

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.

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.

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?

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.

(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

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.

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()".