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

Audio service for Android (#39) #178

Merged
merged 25 commits into from
Aug 21, 2020
Merged

Audio service for Android (#39) #178

merged 25 commits into from
Aug 21, 2020

Conversation

AlmasB
Copy link
Contributor

@AlmasB AlmasB commented Aug 9, 2020

No description provided.

@AlmasB AlmasB requested a review from jperedadnr August 12, 2020 09:50
@jperedadnr
Copy link
Contributor

A DummyAudioService class is required, so API jar does include the impl package.

@AlmasB
Copy link
Contributor Author

AlmasB commented Aug 19, 2020

I've resolved remaining comments and extracted them here for clarity:

  1. I will add AUDIO to client plugin, once this is merged.
  2. setOnFinished() is now gone. Path API is used on JDK11 side.

As for media status, I think we still come back to the same issue of not having enough info. For example, when looping is false, PLAY will eventually internally change to STOP. Since there are no listeners, the state, from our perspective, is still PLAY and not STOP. We can't readily monitor this change with Sound. Perhaps, instead we should think about possible uses cases and then go from there?

I propose we don't add any status related API in the first version on the basis that we can't reliably obtain that information for both music and audio. After the first version is released, the community feedback and use cases should help guide the public API, which will bring us much closer to the desired API than if we were to consider it only theoretically.

  1. I propose we keep the synchronous mediaPlayer.prepare(). This means the following sequence of calls should always succeed:
Optional<Audio> audio = service.loadAudio(...);
audio.ifPresent(a -> a.play());

If the user wants to load the audio from a background thread, they can, for example, by using Thread directly, or javafx Task<>. So, just let the user have the control over the flow. Making load asynchronous means we have to also provide some kind of a status listener, adding extra steps to the call site.

After these are resolved, I'll start finalizing the PR.

@jperedadnr
Copy link
Contributor

I'm okay with your proposal. Audio status could be added later on if needed.

@AlmasB
Copy link
Contributor Author

AlmasB commented Aug 20, 2020

This PR is now ready for a full review and then merge. Sample test video is available for Android 10, showing use of public API and working functionality for both music and sound.

@jperedadnr jperedadnr changed the title Initial draft of audio service #39 Audio service for Android (#39) Aug 21, 2020
@jperedadnr jperedadnr requested a review from johanvos August 21, 2020 09:22
Copy link
Contributor

@johanvos johanvos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the longer term, it would be good if we can have this for iOS and Android in OpenJFX, as an implementation for the OpenJFX API's.
For now, Attach is a good place to test and experiment.

@jperedadnr jperedadnr merged commit 30a7150 into gluonhq:master Aug 21, 2020
@jperedadnr
Copy link
Contributor

@AlmasB Already filed this for you: gluonhq/gluonfx-maven-plugin#241

@AlmasB AlmasB mentioned this pull request Aug 21, 2020
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.

3 participants