Skip to content

[flutter_tts] Refactor the C++ code #401

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

Merged
merged 6 commits into from
Jul 5, 2022

Conversation

JSUYA
Copy link
Member

@JSUYA JSUYA commented Jun 10, 2022

Part of the code refactoring project.

  • Extract method.
  • Hiding tizen api in plugin.cc
  • Minimize use of output parameters
  • Change method name(Init-> TtsCreate, DeInit -> TtsDestroy, ...)

@JSUYA JSUYA marked this pull request as draft June 10, 2022 08:34
@JSUYA JSUYA force-pushed the dev/flutter_tts_refactor branch 3 times, most recently from 03c793e to d0c8edf Compare June 17, 2022 09:03
@JSUYA JSUYA changed the title [WIP][flutter_tts] Refactor the C++ [flutter_tts] Refactor the C++ Jun 17, 2022
@JSUYA JSUYA requested review from bbrto21, HakkyuKim and swift-kim June 17, 2022 09:04
@JSUYA JSUYA marked this pull request as ready for review June 17, 2022 09:04
@swift-kim swift-kim changed the title [flutter_tts] Refactor the C++ [flutter_tts] Refactor the C++ code Jun 17, 2022
@HakkyuKim
Copy link
Contributor

HakkyuKim commented Jun 20, 2022

I'm not sure changing to exception throws is a good idea when the original code worked fine without them. If you are going to use exceptions, they must all be catched (it's easy to miss in callbacks) as @bbrto21 mentioned.

nit: remove "[WIP]" in commit message.

- Extract method.
- Hiding tizen api in plugin.cc
- Enhance error message handling (working)
- Minimize use of output parameters
- Change method name(Init-> TtsCreate, DeInit -> TtsDestroy, ...)
@JSUYA JSUYA force-pushed the dev/flutter_tts_refactor branch from d0c8edf to efa2de7 Compare June 21, 2022 07:14
@JSUYA
Copy link
Member Author

JSUYA commented Jun 21, 2022

@bbrto21 @HakkyuKim Thank you for review. I reverted the code related to the exception.

@JSUYA JSUYA force-pushed the dev/flutter_tts_refactor branch from efa2de7 to 7db8728 Compare June 22, 2022 05:28
@JSUYA
Copy link
Member Author

JSUYA commented Jun 29, 2022

@swift-kim Any opinions?

Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

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

Some nitpicks. Otherwise looks good to me.

- int -> int32_t
- remove debug log and [TTS] tag
- rename callback functions
- remove create and destroy function
- etc
@swift-kim
Copy link
Member

Calling getSpeechRateValidRange fails with a StateError on Tizen.

[ERROR:flutter/lib/ui/ui_dart_state.cc(198)] Unhandled Exception: Bad state: No element
#0      ListMixin.firstWhere (dart:collection/list.dart:167)
#1      FlutterTts.getSpeechRateValidRange (package:flutter_tts/flutter_tts.dart:320)
<asynchronous suspension>
#2      _MyAppState._speak (package:flutter_tts_tizen_example/main.dart:90)
<asynchronous suspension>

Maybe we can

  • set the value of platform to "android" (instead of "tizen"), or
  • just unsupport the method and automatically set tts_speed_ based on the platform's speed range (proportional to 0.0~1.0). For example, if the max speed is 10 and the given speech rate is 0.5, set the tts_speed_ to 5.

How do you think? The implementation can be done in a separate PR.

@JSUYA
Copy link
Member Author

JSUYA commented Jul 4, 2022

Calling getSpeechRateValidRange fails with a StateError on Tizen.

[ERROR:flutter/lib/ui/ui_dart_state.cc(198)] Unhandled Exception: Bad state: No element
#0      ListMixin.firstWhere (dart:collection/list.dart:167)
#1      FlutterTts.getSpeechRateValidRange (package:flutter_tts/flutter_tts.dart:320)
<asynchronous suspension>
#2      _MyAppState._speak (package:flutter_tts_tizen_example/main.dart:90)
<asynchronous suspension>

Maybe we can

  • set the value of platform to "android" (instead of "tizen"), or
  • just unsupport the method and automatically set tts_speed_ based on the platform's speed range (proportional to 0.0~1.0). For example, if the max speed is 10 and the given speech rate is 0.5, set the tts_speed_ to 5.

How do you think? The implementation can be done in a separate PR.

Could you please explain more how to reproduce this log?
As far as I know, the example in flutter_tts doesn't have the code to call getSpeechRateValidRange.
Do I need to test it separately? If the problem is not caused by this refactoring, I think it is better to handle it in another PR.

@swift-kim
Copy link
Member

@JSUYA You can add

await FlutterTts.getSpeechRateValidRange;

to anywhere in the example app (like initState or _speak). And yes, the error has not been caused by this PR, but I mentioned here because the first solution in the above requires only a single line of change. If you prefer the second solution, we can work on it in another PR.

…f tizen

The value of "platform" is temporarily set to "android" instead of
"tizen". Because it is not declared in TextToSpeechPlatform of
TextToSpeech example.
@JSUYA
Copy link
Member Author

JSUYA commented Jul 4, 2022

@JSUYA You can add

await FlutterTts.getSpeechRateValidRange;

to anywhere in the example app (like initState or _speak). And yes, the error has not been caused by this PR, but I mentioned here because the first solution in the above requires only a single line of change. If you prefer the second solution, we can work on it in another PR.

Thank you for your advice. I updated PR

Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

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

Thank you!

@JSUYA JSUYA merged commit b3628fb into flutter-tizen:master Jul 5, 2022
@JSUYA JSUYA deleted the dev/flutter_tts_refactor branch July 5, 2022 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants