-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Silence detection sample #130
base: main
Are you sure you want to change the base?
Silence detection sample #130
Conversation
I think the separate page works fine, thank you! |
@ArturWyszomirski thank you so much for this effort! I really like the concept. I am wondering if we can consider refactoring this implementation down to something that might be a little easier to use. This is fully intended as a discussion and not me telling you want should be done so please feel free to push back if you don't agree :). I am thinking of potentially dropping the Enhance
|
@bijington your proposal looks super cool! The syntax is so much readable that even my wife is able to get it :P Yet, I am not sure if I got your idea right, so below I have linked my implementation where I created a static class And, please, don't ever hesitate to make any reviews, remarks and suggestion :) I am very grateful for every kind of feedback on my work, especially the one which levers up my skills and way of thinking about designing software. |
@ArturWyszomirski thank you, that is nice to hear! I assume your wife is non-technical? If so then I guess the point kind of works, the aim is to make some readable and therefore developers don't have to think about what the code does as it should be descriptive itself. I have created this PR: ArturWyszomirski#1 To show what I was thinking. This does a bit more than your PR originally intended but it leaves it open for other ways to potentially control how a recording is stopped. What do you think? |
@bijington i think: WOW! I am truly impressed by your solution! However, I was still concerned about canceling ongoing stop rules when one of them will be fulfilled (e.g. user push stop button and fire stop immediately rule when there is already awaited silence detection). So I came up with the idea that if one stop rule would be fulfilled, then all other should be cancelled, so API consumer won't have to bother about cancelling tasks (but, if he decide to do it on his own, the cancellation token is still exposed in EnforceStop parameters). Let me know what do you think about it: bijington#1 |
Thank you! Oh sorry I realise that I didn't really update the sample app other than to allow it to compile. I have opened bijington#2 to show the changes that I think need to be made in order to satisfy your scenario. I'll be honest I haven't tested this but my reasoning is that the Does this make sense? I could well be missing something |
@bijington em, I must have not been clear enough and you might missed some key parts of my pr: https://github.com/bijington/Plugin.Maui.Audio/pull/1/files What I tried to achieve here is to follow to idea of making API user's life easier, so that he won't have to bother with task cancellation at all. I put the logic responsible for cancellation of all ongoing stop tasks inside the AudioRecorder class. If one reason for stop is fulfilled all other stop tasks will automatically be cancelled. In a result the viewmodel part could be as simple as that: async Task StartStopRecordToggleAsync()
{
if (!IsRecording)
{
await audioRecorder.StartAsync();
audioSource = await audioRecorder.StopAsync(When.SilenceIsDetected(SilenceTreshold, SilenceDuration));
}
else
{
audioSource = await audioRecorder.StopAsync(When.Immediately());
}
} |
Ah I understand your point now. I know we didn't do this overly in the past but I believe it is best practice to allow developers to supply a cancellation token when providing an asynchronous method to give them the power to cancel it themselves. I like your idea but I think it lends itself better to a fire and forget model? By this I mean with your example you couldn't await the first Stop call otherwise this command method won't execute a second time (at least not by default if using something like RelayCommand) from the MVVM toolkit. This might be a bit of an edge case scenario though. I'm wondering if there is some middle ground here although I'd be keen to hear what @jfversluis thinks on this topic? |
I left the cancellation token as a parameter of EnforceStop, so if someone will decide to use their own cancellation token, it is still possible to do. The automated cancellation of all stop tasks is fired only if cancellation token is left as default: if (cancellationToken == default)
{
cancellationToken = stopCancelTokenSource.Token;
}
bool isStopRuleFulfilled = await (stopRule ?? When.Immediately()).EnforceStop(this, cancellationToken);
if (isStopRuleFulfilled)
{
stopCancelTokenSource.Cancel();
stopCancelTokenSource = new();
} I did some tests using community toolkit's RelayCommand and I didn't spot any issues besides the fact that it is not possible to make directly the toggle type button because the button stays inactive until the RelayCommand's method finishes its tasks. However I use this workaround to make start/stop button enabled all the time: async Task StartStopRecordToggleAsync()
{
if (!IsRecording)
{
MainThread.BeginInvokeOnMainThread(async () =>
{
await audioRecorder.StartAsync();
audioSource = await audioRecorder.StopAsync(When.SilenceIsDetected(SilenceTreshold, SilenceDuration));
}
}
else
{
audioSource = await audioRecorder.StopAsync(When.Immediately());
}
} |
For clarity I've made a separate page for silence detection sample, but I can also merge it into existing audio recorder page if you prefer.