gluonhq / attach

GNU General Public License v3.0
48 stars 26 forks source link

Fixed some issues and improved performance of AndroidAudioService #348

Closed salmonb closed 1 year ago

salmonb commented 1 year ago

hi,

I'm making this PR to propose the following changes to the AndroidAudioService:

I contacted Almas but he said he didn't have time to review these changes, and he was happy for me to submit the PR.

Regarding the performance improvements, they might not be noticeable in most games, but I'm working on a new version of SpaceFX with an increased number of ennemies, and I promise these changes make a noticeable difference.

Like I said, I plan to publish the WebFX demos on the Google Play & App Stores to show live examples of JavaFX apps compiled by Gluon. I'm working in particular on this new version of SpaceFX as an example of how performant the native apps compiled with Gluon can be (this is in this context that I'm making these changes). Let me know if you want access to this new version of SpaceFX to test this PR (works great now on iPads and Android tablets).

salmonb commented 1 year ago

Well spotted. I only tested with SpaceFX on application start before submitting the PR, and since the volume buttons were working at this stage, I was assuming it will work all the time like in iOS with no additional code required in the app. I hadn't noticed that the volume buttons stopped working as soon as the user interacted with the screen...

I find a bit annoying to have to code again in the app (or service) what the Android system can already handle by default. I think what happens is that when the users interacts with the screen, this makes the Gluon substrate MainActivity focused, and this activity consumes all key events, even those that could eventually be handled by the system. Is it because dispatchKeyEvent() always return true? https://github.com/gluonhq/substrate/blob/master/src/main/resources/native/android/android_project/app/src/main/java/com/gluonhq/helloandroid/MainActivity.java#L379

I would suggest to investigate if dispatchKeyEvent() could return false for the volume keys since they can be handled correctly by Android by default (and it seems this is how Gluon behaves with iOS). I think this would avoid these complications with the Android services and simplify the code.

But for now I rewrote the volume buttons management in the Android audio service. The difficulty was actually to capture the key events. I couldn't do what you suggested because the audio service has no view (as opposed to the video service). And I couldn't find a way to add a key listener in the context of the DalvikAudioService (maybe it's because I'm not familiar enough with the Android SDK). So what I did in the end is to use the JavaFX API in the AndroidAudioService by installing a key event filter on the Stage(s) to intercept those volume keys (this adds a dependency to javafx.graphics by the way), and then dispatch these events to the DalvikAudioService so it can handle them (I took the code from the video service at this point). It seems to work now.

Let me know if you think it's ok.

jperedadnr commented 1 year ago

Sorry you needed to do this and go all the way back from JavaFX to Android to get this working. Actually, you are totally right: Substrate shouldn't consume the volume events. I've tested this with your previous changes (adding just activity.setVolumeControlStream) and that works perfectly fine.

I've created this PR: https://github.com/gluonhq/substrate/pull/1209 for that. In case you want to give it a try?

salmonb commented 1 year ago

Great. I'm happy to test, but I don't know how to tell the GluonFX Maven plugin to use Gluon Substrate 0.0.57-SNAPSHOT instead of 0.0.56. Can you please give me some guidance?

jperedadnr commented 1 year ago

Just clone my PR, and build:

sh gradlew publishToMavenLocal

and then clone the GluonFX maven plugin (version 1.0.17-SNAPSHOT, and using Substrate 0.0.57-SNAPSHOT) and install:

mvn clean install

That's it, now your project can use 1.0.17-SNAPSHOT.

salmonb commented 1 year ago

Thanks, cloning the GluonFX maven plugin was my missing part indeed...

I reverted my last changes, and yes, I confirm that now Android is managing the volume control without any additional code from the AudioService (activity.setVolumeControlStream() is enough) 👍

Should I update this PR (revert my last changes)?

jperedadnr commented 1 year ago

Can you comment the same at https://github.com/gluonhq/substrate/pull/1209 ? (not sure if you can approve it). Once merged, you can revert your last changes, indeed.

salmonb commented 1 year ago

I updated the PR but considered your previous comments. Let me know if it's ok.

Will you remove the now unnecessary volume control code from the VideoService?

jperedadnr commented 1 year ago

(Edited my comment, it had the wrong link). We can check the video service in a follow-up PR, once both Substrate and this PR are merged, and tested.

salmonb commented 1 year ago

Ok, I commented your Gluon Substrate PR.

jperedadnr commented 1 year ago

I'll merge now. For those that want to use these changes in the pom:

<attach.version>4.0.17-SNAPSHOT</attach.version>
<gluonfx.maven.plugin.version>1.0.17-SNAPSHOT</gluonfx.maven.plugin.version>

 <repositories>
        <repository>
            <id>snapshot</id>
            <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
        </repository>
    </repositories>

<pluginRepositories>
        <pluginRepository>
            <id>snapshot</id>
            <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
        </pluginRepository>
    </pluginRepositories>