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

Create interfaces for Audio and Input on Android backend #5985

Merged
merged 8 commits into from
Apr 21, 2020

Conversation

obigu
Copy link
Contributor

@obigu obigu commented Mar 29, 2020

The original motivation for this PR was to realise how difficult was to replace the Android Audio backend by a different one (libGDX Oboe in this case (which is great by the way)).

This PR is a demo of a possible approach to improve extendability, decoupling and code quality on functional modules (like Audio or Input to start with) on each libGDX backend.

LibGDX currently has interfaces to be used in common code such as Audio but lacks backend specific interfaces. For example, AndroidApplication currently has an instance of the Android backend implementation of Audio called AndroidAudio which is instantiated on initialize(). This approach has 2 major problems:

  1. It's really hard for client projects to replace the Audio implementation for a different one without extending AndroidApplication itself.
  2. The coupling on specific implementations allows bad practices spreading across the codebase and making libGDX progressively less extendable, flexible and clean (I've already realized this just by refactoring 2 modules on the Android backend).

With the suggested approach it's as simple as overriding the createXXXX() method on the desired backend and return your desired implementation.

There may be other more sophisticated approaches but this one ensures backwards compatibility and allows a progressive refactor (module by module and backend by backend).

Please share your thoughts.

@cypherdare
Copy link
Member

Haven’t looked at your implementation yet but this would be helpful for customizing how we interact with input and adding our own specific features, such as turning off certain sensors at times to save battery.

@obigu
Copy link
Contributor Author

obigu commented Apr 4, 2020

@cypherdare Yes, in general it would make it simpler to customize our own implementations of the different modules and would also probably encourage users to share their own custom modules with the community (for example this #5989 would make a lot of sense as an alternative AndroidAudio implementation).

@wheelergames
Copy link
Contributor

This would be great to get done, so I can use my my Android looping music fix without having to put it in the codebase for everyone

@noblemaster
Copy link
Member

I think this is a good idea! The changes look fine. I guess it could be merged? Other opinions?

@crykn
Copy link
Member

crykn commented Apr 20, 2020

I am very much in favour of this refactor, but for consistency's sake, I think this should be changed in all backends before being merged.

@noblemaster
Copy link
Member

Yes, changing it in all backends makes sense!

@cypherdare
Copy link
Member

In my opinion, other backends should get the same features when possible, but it should never gate progress on a specific backend to wait for an implementation for the rest.

@wheelergames
Copy link
Contributor

wheelergames commented Apr 20, 2020

Partly because I'm biased and want to use my android fix, but I don't think we should wait for all backends to be done at once.

Personally I believe it's generally better to release smaller and more often.

I don't think we should actually release into the wild just one backend, but I do believe it's a good idea to merge one backend change into master or wherever the nightlies/SNAPSHOTs are released from. That way there could be a large enough crowd of people to help test it, but it's only a SNAPSHOT and therefore if it breaks, it's not breaking for live people, only for the people who like living on the edge!

Just my thoughts

@noblemaster
Copy link
Member

I guess I can go either way. Is this solid enough to be approved? Any reservations for approval?

@wheelergames
Copy link
Contributor

I don't think @obigu is happy for it to be merged, and it's his PR so I'd definitely listen to him first. Especially as I think the original idea was just a proof of concept (work in progress)

@noblemaster
Copy link
Member

Fine with me. @obigu if you think it's ready, can you add your changes to the CHANGES text file to and we merge it?

@obigu
Copy link
Contributor Author

obigu commented Apr 20, 2020

I agree with @cypherdare and @wheelergames. I think it's better doing it with few modules (let's keep Audio and Input for now) and backend by backend and let it mature in SNAPSHOT for a while before the next release to see how it goes. Don't worry for inconsistency, backwards compatibility is not broken and the moment this PR is merged I will have the rest of backend PRs ready in a matter of days.

@noblemaster Before merging I'd like to get @Tom-Ski approval/feedback to make sure we are all on the same boat and make a final sanity check plus add some javadocs (I will do this in a while).

…ener, OnGenericMotionListener interfaces.

Added javadocs and some cleanup.
@Tom-Ski
Copy link
Member

Tom-Ski commented Apr 20, 2020

Think its good, and agree with comments as this is standalone enough to just be merged without waiting on other backends.

@obigu what about AndroidTouchHandler? It can't be re-used, so maybe internalise that into the class?

@cypherdare
Copy link
Member

It should also be safe to remove the onTap, postTap, onDrop, and postDrop methods. These appear to be obsolete ways of accepting inputs from AndroidWallpaperService. AndroidWallpaperService no longer calls these, put rather calls AndroidInput.onTouch() and AndroidWallpaperListener.iconDropped() instead.

I'll be sure to test the snapshot with my live wallpapers after this is merged.

@obigu
Copy link
Contributor Author

obigu commented Apr 20, 2020

@Tom-Ski Yes, I agree we should get rid of the AndroidTouchHandler interface and replace it by the only implementation (which is heavily coupled to AndroidInputImpl mainly due to the way we process events synchronously).

@obigu obigu changed the title [WIP] Create interfaces for Audio and Input on Android backend Create interfaces for Audio and Input on Android backend Apr 21, 2020
@obigu
Copy link
Contributor Author

obigu commented Apr 21, 2020

@noblemaster If there are no more concerns I think it's good to merge

@noblemaster noblemaster merged commit 800c493 into libgdx:master Apr 21, 2020
@noblemaster
Copy link
Member

Thanks :-D

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

6 participants