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

[Feature] Allow specifying the alarm volume using steps/stops #276

Closed
wants to merge 0 commits into from

Conversation

orkun1675
Copy link
Contributor

@orkun1675 orkun1675 commented Nov 1, 2024

Replaced by #282


This PR allows alarm users to speicfy a staircase function to control the alarm volume.

It also fixes #267 by adding this line. For some reason AVAudioPlayer ignores volume changes unless the initial volume is set to zero.

PS: Overall, AVAudioPlayer has been a pain to deal with, setVolume has bugs, and it also doesn't report the real .volume value while a fade is in progress.

I've also taken the liberty to fix lint warnings in both iOS and Android native code.

I've tested using iOS and Android emulators + iOS physical device. I would appreciate if you could test this as well before merging. Thanks!

@gdelataillade
Copy link
Owner

Thanks for the PR! I have one remark: it might be a bit much to have three separate parameters just for handling volume fade. Maybe we could create a new model, like I did with NotificationSettings, to group related parameters under the same theme. For example, the volume parameter and the new volumeEnforced parameter could be part of a model called VolumeSettings. I realize it would take some extra work to refactor, but what do you think of this approach?

@orkun1675
Copy link
Contributor Author

orkun1675 commented Nov 3, 2024

Hi @gdelataillade I totally agree the current API is not ideal. It also relies on the caller providing two lists of equal length + the first list having strictly inceasing float values. It would be much better to communicate these requirements with an object oriented API.

However, I would prefer to decouple these improvements from this feature PR. It would be much easier to implement this API refactoring after migrating to pigeon.

Maybe we can merge this PR as is, but also file a bug to refactor, WDYT?

@gdelataillade
Copy link
Owner

Got it, so your plan is:

  1. Merge this PR
  2. Migrate to pigeon
  3. Refactor to an object-oriented API

A couple of thoughts: Wouldn’t adding these parameters now be confusing for users? Also, migrating to Pigeon could take a while, and I’m not sure it’s needed since the current setup already has models and native data handling working well. I haven’t used Pigeon myself, so I’m not sure how well it would fit here. Are you comfortable handling the migration if we go that route?

@orkun1675
Copy link
Contributor Author

orkun1675 commented Nov 7, 2024

I'll see if I can carve out sometime to look into Pigeon migration. It's OK by me to keep this PR pending in the meantime.

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.

iOS: Alarm volume does not fade
2 participants