markfguerra / GLWallpaperService

Please submit issues and pull requests to the main repository
https://github.com/GLWallpaperService/GLWallpaperService
Apache License 2.0
218 stars 114 forks source link

Remove and replace redundant interfaces #26

Closed jayschwa closed 11 years ago

jayschwa commented 11 years ago

Many of the interfaces are exact duplicates of ones from GLSurfaceView and, as far as I can tell, add no value. While integrating the latest code into the downstream Rajawali framework, I found these interfaces to be a nuisance. Our classes, which already implement the GLSurfaceView interfaces, were not considered valid to the GLWallpaperService.

GLWallpaperService has been modified to accept the GLSurfaceView interfaces. The local duplicates have been removed. This will break backward compatibility for projects that use the local variant, but the fix is easy: replace the "GLWallpaperService." prefix with a "GLSurfaceView." prefix or tweak the import statements.

Alternate Implementation

25 is a variation of this pull request that maintains backward compatibility and merely deprecates the duplicate interfaces. However, it is more verbose and "ugly". Only one or the other should be merged.

markfguerra commented 11 years ago

Sorry, I haven't had time yet to look at your pull requests in detail. I work two jobs so my workload is crazy on a good day.

However, I haven't forgotten about these contributions. I'll mention these pull requests on the mailing list, and hopefully people will begin to comment and get the ball rolling.

jayschwa commented 11 years ago

That's understandable. No rush. There's a possibility Rajawali may stop using this implementation anyway. If the alternate one bears fruit, I'll be sure to share the findings with this project.

On a slightly more "meta" note, if you need additional maintainers on this project due to your own workload, I'll throw my hat into the ring. Contact me via e-mail (see profile page) if you wish to discuss further.

Thanks.

ratana commented 11 years ago

I recommend this pull request; it's cleaner and impact on existing projects will be minimal.

markfguerra commented 11 years ago

Hey there!

So I was looking at this issue and I think we can solve this one painlessly. Someone else once had a similar issue to yours (#18), where they wished to refer to our flavor of Renderer as if it were android.opengl.GLSurfaceView.Renderer.

Since the method signatures were the same, the solution we chose was to make our Renderer interface extend that of GLSurfaceView. This result was that you could refer your Renderer whichever way you like and they were treated the same way by the compiler. No one downstream had to change their code around either, so everybody won.

I believe using this approach on the remaining interfaces would solve your issues in a similar way. Would you agree?

markfguerra commented 11 years ago

I pushed a new branch with the code changes I suggested above, regarding extending the GLSurfaceView interfaces. https://github.com/markfguerra/GLWallpaperService/commits/issue26-extend-interfaces (Branch deleted - click here: 85946a9a) Check it out and let me know if this works for you.

jayschwa commented 11 years ago

That is what the alternate #25 does. The interfaces are made subclasses of the GLSurfaceView ones and marked as deprecated (with the recommendation that the GLSurfaceView ones be used instead).

markfguerra commented 11 years ago

Oops, looks like you're right, you also extended the GLSurfaceView interfaces in #25. My mistake, I missed that part.

I'm open to getting rid of these interfaces, they seem to be causing trouble for you and others without adding any value. A quick google search seemed to indicate that no publicly available source code is extending or implementing the additional interfaces.

In the interest of not making bold moves with our public interface without any warning, I think #25 is a better approach right now. We can deprecate the interfaces and at some point in the future remove them. Thanksfully the impact should be pretty low when we finally do remove them; downstream users won't have to make too many code changes to accomodate the new interface choices, as Adam pointed out.

markfguerra commented 11 years ago

I merged in pull request #25. Thanks again for all the hard work.

markfguerra commented 11 years ago

I pushed a new tag archive/remove-interfaces to hold on to these commits for posterity