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]: Add 'on_eos' event #8609

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

mak8kammerer
Copy link
Contributor

@mak8kammerer mak8kammerer commented Feb 11, 2024

Good evening!

I want to add on_eos event to Kivy audio system. It works the same way as in the video, but is only available with ffpyplayer and gstplayer providers.

Related issue: #2156.

P.S.: Could someone please advise me on how the tests for this PR should look? Or is it not necessary to write them for this case?

Maintainer merge checklist

  • Title is descriptive/clear for inclusion in release notes.
  • Applied a Component: xxx label.
  • Applied the api-deprecation or api-break label.
  • Applied the release-highlight label to be highlighted in release notes.
  • Added to the milestone version it was merged into.
  • Unittests are included in PR.
  • Properly documented, including versionadded, versionchanged as needed.

@mak8kammerer mak8kammerer marked this pull request as ready for review February 11, 2024 20:20
@Julian-O
Copy link
Contributor

As well as unittests, this needs some documentation. I've read the code, and I am still just guessing what EOS stands for.

@mak8kammerer mak8kammerer marked this pull request as draft April 1, 2024 16:09
@mak8kammerer
Copy link
Contributor Author

Tests are ready. If somebody has any suggestions, let me know. Tests are not my thing.

@mak8kammerer mak8kammerer marked this pull request as ready for review April 2, 2024 11:29
@gottadiveintopython
Copy link
Member

I'm not familiar with the audio providers so I have nothing to say about it, but you can use the kivy_clock pytest fixture instead of kivy.clock.Clock, which makes the unittests have less side-effects.

@mak8kammerer
Copy link
Contributor Author

mak8kammerer commented Apr 16, 2024

can use the kivy_clock pytest fixture instead of kivy.clock.Clock

Done. Thank you!
Ready to review.

@gottadiveintopython
Copy link
Member

Nice! You don't need the set_clock.

def test_on_eos_event(self, kivy_clock):
    ...

@mak8kammerer
Copy link
Contributor Author

Nice! You don't need the set_clock.

def test_on_eos_event(self, kivy_clock):
    ...

It won't work. Take a look at this StackOverflow question and this code snippet.

@gottadiveintopython
Copy link
Member

It won't work.

Sorry, I didn't know that.

@mak8kammerer
Copy link
Contributor Author

@misl6, could you please review my PR, as it seems that @Julian-O is offline for long time.

Sorry for pinging.

@gottadiveintopython
Copy link
Member

gottadiveintopython commented May 3, 2024

Seems like on_play and on_stop events work differently depending on audio providers when sound.loop = True.

  • sdl2 ... these events will be fired in each iteration
  • gstplayer ... nothing will be fired in each iteration

@mak8kammerer
Copy link
Contributor Author

Seems like on_play and on_stop events work differently depending on audio providers when sound.loop = True.

* sdl2 ... these events will be fired every iteration

* gstplayer ... nothing will be fired during the loop

This is not related with this PR. I think it should be clarified in a separate issue.

@gottadiveintopython
Copy link
Member

gottadiveintopython commented May 3, 2024

Yes, I'll open an issue. (Sorry I forgot to mention that the issue occurs on the master branch)
The reason I mentioned the issue is that, unless we define when on_play and on_stop events should occur, it's probably hard to document about the difference between on_eos and on_stop.

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

3 participants