search

gdelataillade / alarm

A Flutter plugin to easily manage alarms on iOS and Android
https://pub.dev/packages/alarm
MIT License
132 stars 86 forks source link

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

Closed orkun1675 closed 1 week ago

orkun1675 commented 2 weeks ago

Replaced by https://github.com/gdelataillade/alarm/pull/282

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

It also fixes https://github.com/gdelataillade/alarm/issues/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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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.