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

Refactor/new implementation #40

Merged
merged 14 commits into from
Dec 26, 2023
Merged

Refactor/new implementation #40

merged 14 commits into from
Dec 26, 2023

Conversation

kingdcreations
Copy link
Collaborator

close #38

Copy link
Owner

@riderodd riderodd left a comment

Choose a reason for hiding this comment

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

Nice work @kingdcreations , can you update the Readme with a warning about this breaking change and I take care of the iOS part ?

@riderodd
Copy link
Owner

riderodd commented Dec 6, 2023

@kingdcreations : how do you specify an array of available strings for the recognition now? (the old grammar parameter)

@kingdcreations
Copy link
Collaborator Author

@kingdcreations : how do you specify an array of available strings for the recognition now? (the old grammar parameter)

No changes !

Nice work @kingdcreations , can you update the Readme with a warning about this breaking change and I take care of the iOS part ?

I updated it with new functionalities, but I'd still like to add those:

  • Timeout handling, I want to add an argument to start() but as there is no function overloading in js, should i set start() args to an options object like:
start({
 grammar: ["left", "right", "[unk]"],
 timeout: 10000
})
  • String promises instead of { data: String } actual one

What do you think ?

@riderodd
Copy link
Owner

riderodd commented Dec 7, 2023 via email

@kingdcreations
Copy link
Collaborator Author

All done ! However react native 0.73 is here, should i update ?

@riderodd
Copy link
Owner

All done ! However react native 0.73 is here, should i update ?

No let's keep that for later

Copy link
Owner

@riderodd riderodd left a comment

Choose a reason for hiding this comment

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

Amazing work thanks !
I'm a bit confused about all the events, but that's the way Vosk works so... If you have any suggestion to make it clearer feel free to make a suggestion !
As soon as everything is resolved I'll work on the iOS side.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@riderodd riderodd left a comment

Choose a reason for hiding this comment

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

Nice thanks !

@riderodd
Copy link
Owner

@kingdcreations : would you review my last commits ?

ios/Vosk.swift Outdated

/// Set grammar to use
@objc(setGrammar:withResolver:withRejecter:)
func setGrammar(words: [String]?, resolve:RCTPromiseResolveBlock, reject:RCTPromiseRejectBlock) -> Void {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No cleaning/ restart needed ? Even after multiple calls/ recognition ? No leak ?

Copy link
Owner

Choose a reason for hiding this comment

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

Grammar is stored into an internal class variable, for sure it won't work unless you call stop() and start() again. How did you do on Android ? I may have implemented a wrong pattern ^^

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I clean the recognizer, call the native setGrammar method, and restart, wdy think ?

@ReactMethod
  fun setGrammar(grammar: ReadableArray? = null, promise: Promise) {
    if (recognizer == null || speechService == null) {
      promise.reject(IOException("Recognizer is not started yet"))
    } else {
      try {
        speechService!!.stop()
        speechService!!.shutdown()

        if (grammar != null)
          recognizer!!.setGrammar(makeGrammar(grammar))
        else
          recognizer!!.setGrammar("[]")

        speechService = SpeechService(recognizer, sampleRate)
        if (speechService!!.startListening(this))
          return promise.resolve("Recognizer successfully started")

      } catch (e: IOException) {
        promise.reject(e)
      }
    }
  }

Copy link
Owner

Choose a reason for hiding this comment

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

@kingdcreations : ok I did the same implementation on my side, but it made me think about the timeout argument that id not preserved, isn't it ? I mean, I call start with timeout: 5, then setGrammar(), and my timeout disappear no ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, what should i do ?

Copy link
Owner

Choose a reason for hiding this comment

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

In a first time, removing setGrammar seem fine no ? Devs can just call stop() and start() again with grammar in parameters, and they keep control of the recognition state on their UI. Isn't it better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, lets do that

ios/Vosk.swift Outdated Show resolved Hide resolved
@riderodd riderodd merged commit 3ad4e27 into main Dec 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.

Better implementation
2 participants