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

Add setPauseFor method for late setup #403

Merged
merged 5 commits into from
Jun 26, 2023
Merged

Add setPauseFor method for late setup #403

merged 5 commits into from
Jun 26, 2023

Conversation

JekaNS
Copy link

@JekaNS JekaNS commented Jun 19, 2023

This enable to setup puseFor timeout after start listening.

Reason: in my app I try to implement continuous conversation. So when start listentening after previous reposne, user may think longer. It more UX friendly if start listening with longer pauseFor (e.g. 10 sec) and after first onResult, chage pauseFor to short timeout (e.g. 3 sec.)

Copy link
Contributor

@sowens-csd sowens-csd left a comment

Choose a reason for hiding this comment

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

It's an interesting idea, thanks for outlining the use case, that's helpful. Also, many thanks for including tests, nice to see.

I'm wondering if we could make its functionality clearer through naming. Maybe something like 'changePauseFor'? I think that would be the simplest change.

There are more complicated options like adding an optional callback to onResult or a return from onResult to make its intended use clearer. But I think that's probably overkill.

@JekaNS
Copy link
Author

JekaNS commented Jun 19, 2023

Sure. I changed the method name to "changePauseFor"

And yes, I think adding new callbacks doesn't make sense. It's easy to implement on APP side.
For example, always start listening with pauseFor: 10 sec. and on each onResult change it to 3 sec. That why i add check for reload _listenTimer only if pauseFor diferrent that actual. So on APP side i no need care about checking if it's first onResult call or not.

@JekaNS
Copy link
Author

JekaNS commented Jun 19, 2023

Oh. Wait. I found some problems.
Today I fix them and give you an explanation here.

…imer. Fix counting delta time for last speach event.
@JekaNS
Copy link
Author

JekaNS commented Jun 19, 2023

So...

There was a problem with calulating delta time from last speech event _lastSpeechEventAt
If try to change pauseFor when no speech activity, delta may be greather than new pauseFor.
For example, we start listening without pauseFor, then after 10 seconds change pauseFor to 5 sec, in this case listening stops immediatelly beacause delta time is already 10 sec.

I had to overwrite _lastSpeechEventAt when changing pauseFor. Looks like a workaround, because _lastSpeechEventAt semantically not for this purpose. May be you have better solution?

Cover this case in tests.

Also i fixed checking for listening status. Previous _listenTimer?.isActive it was a mistake

@sowens-csd
Copy link
Contributor

Yes, I see the problem. I agree that changing the _lastSpeechEventAt is a bit undesirable since it implies there was a speech event when there wasn't. Two possibilities occur to me. The first would be to accept that if you change the pauseFor to something less than the current pause it would immediately terminate the listen session. I like that because it's simple and I think that it's a reasonable outcome of calling changePauseFor in that situation. Thoughts?

The second idea would be to add a new optional parameter to the _setupListenAndPause method which would be the elapsedPause or something that could optionally replace the _elapsedSinceSpeechEvent time. changePauseFor could send a 0 to cause the calculation to start from that time.

@JekaNS
Copy link
Author

JekaNS commented Jun 20, 2023

The second idea seems to be more intuitive for developers who will use this feature.

I added an optional parameter {bool ignoreElapsedPause = false} to _setupListenAndPause and if it is true then _elapsedSinceSpeechEvent is not used when calculating the timer

Copy link
Contributor

@sowens-csd sowens-csd left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@sowens-csd sowens-csd merged commit 1134d7d into csdcorp:main Jun 26, 2023
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.

2 participants