-
Notifications
You must be signed in to change notification settings - Fork 16
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
2916: Add tts on android #2950
base: main
Are you sure you want to change the base?
2916: Add tts on android #2950
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works pretty well so far! A couple of points about the functionality:
- If I change the volume while it's playing, it starts re-reading the current fragment. If possible, I think it should keep reading at the same spot.
- In using, I thought the volume bar was for updating the speed of reading. Maybe that's just me but maybe not, we should see if other people have that problem.
- For Persian (and any other languages where it doesn't work), I think we should hide the button to read the text aloud, and close the overlay if it's open when someone switches languages.
- In emulated iOS, no sound comes out and I get this warning:
Excessive number of pending callbacks: 501. Some pending callbacks that might have leaked by never being called from native code: {"1000":{},"1001":{},"1002":{},"1003":{},"1004":{},"1005":{},"1006":{},"1007":{},"1008":{},"1009":{},"1010":{},"1011":{},"1012":{},"1013":{},"1014":{},"1015":{},"1016":{},"1017":{},"1018":{},"1019":{},"1020":{},"1021":{},"1022":{},"1023":{},"1024":{},"1025":{},"1026":{},"1027":{},"1028":{},"1029":{},"1030":{},"1031":{},"1032":{},"1033":{},"1034":{},"1035":{},"1036":{},"1037":{},"1038":{},"1039":{},"1040":{},"1041":{},"1042":{},"1043":{},"1044":{},"1045":{},"1046":{},"1047":{},"1048":{},"1049":{},"...(truncated keys)...":451}
But I guess you did say that it's not yet tested for iOS :D
As discussed, I only half-checked the PR, let me know when it's ready for review again :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i couldn't test it since its crashing for me on emulator when i click play on Android 13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me on android. good work.
Since its a draft some improvements are missing i guess
Shouldn't here stand the title of what will be read?
Even add shadows/elevation for the container and the play button
The icons should switch i guess since the direction arrows are wrong
I think it would be better to create a fixed area for this player instead of overlapping content, but this may be also done in an additional tasks, since it will need some effort.
Code Climate has analyzed commit 042e2ac and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 75.9% (50% is the threshold). This pull request will bring the total coverage in the repository to 74.2%. View more on Code Climate. |
Maybe I should define |
Hmm, putting in the example There is also a very minor bug that I found: when opening the TTS player, the line with the last update jumps down, and back up when you close it. I attached a video of the behavior. Simulator.Screen.Recording.-.iPhone.16.-.2024-10-28.at.15.34.08.mp4 |
This behavior is intended to leave space for the player to prevent it from covering the content. |
That's a good idea but leads to jumping in this case. The padding should probably be added under the last update stamp instead of before. |
I unfortunately still don't get any sound out. But the iOS can be fixed in a separate issue, I think you can for now work on getting the PR ready for reviewing for Android :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, looks quite good already and I like the implementation. I just think that the TtsPlayer
component should be split up to separate concerns and increase readability.
assets/icons/sound.svg
Outdated
@@ -0,0 +1 @@ | |||
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" fill="none" viewBox="0 0 24 24"><path fill="currentColor" d="M9.878 19.125c-.188 0-.365-.04-.53-.12-.167-.081-.342-.21-.524-.387l-3.428-3.251a.29.29 0 0 0-.201-.072h-2.31c-.541 0-.957-.148-1.247-.443-.29-.295-.434-.735-.434-1.32V10.54c0-.58.144-1.017.434-1.312.29-.3.706-.45 1.247-.45h2.31a.29.29 0 0 0 .201-.073l3.428-3.218c.204-.199.38-.341.531-.427.15-.086.32-.129.507-.129.274 0 .496.094.668.282a.938.938 0 0 1 .257.668v12.344a.874.874 0 0 1-.257.644.867.867 0 0 1-.652.257zm3.774-3.677a.605.605 0 0 1-.273-.435.729.729 0 0 1 .152-.523c.231-.327.41-.703.54-1.127a4.65 4.65 0 0 0 .193-1.343c0-.467-.065-.915-.193-1.344a3.538 3.538 0 0 0-.54-1.127.688.688 0 0 1-.16-.515.637.637 0 0 1 .281-.442c.14-.097.293-.13.459-.097a.636.636 0 0 1 .41.282c.311.407.553.896.724 1.464.177.569.266 1.162.266 1.779a5.95 5.95 0 0 1-.266 1.778 4.563 4.563 0 0 1-.724 1.464.636.636 0 0 1-.41.282.627.627 0 0 1-.459-.096zm2.994 2.1a.565.565 0 0 1-.25-.403.726.726 0 0 1 .137-.506 7.6 7.6 0 0 0 1.014-2.133c.247-.8.37-1.628.37-2.486a8.55 8.55 0 0 0-.362-2.487 7.338 7.338 0 0 0-1.022-2.132.695.695 0 0 1-.145-.5.597.597 0 0 1 .258-.41.61.61 0 0 1 .474-.104.636.636 0 0 1 .41.281c.521.698.923 1.516 1.208 2.455a9.94 9.94 0 0 1 .426 2.897c0 .992-.145 1.955-.434 2.888-.285.934-.684 1.755-1.2 2.463a.636.636 0 0 1-.41.281.658.658 0 0 1-.474-.104zm3.017 2.132a.568.568 0 0 1-.257-.418.699.699 0 0 1 .136-.5 12.144 12.144 0 0 0 1.778-4.257c.167-.798.25-1.627.25-2.485 0-.864-.083-1.693-.25-2.487-.165-.8-.402-1.553-.707-2.261-.3-.714-.657-1.379-1.07-1.996a.726.726 0 0 1-.137-.507.563.563 0 0 1 .257-.41.556.556 0 0 1 .467-.097.626.626 0 0 1 .418.298c.462.681.861 1.42 1.2 2.213.337.789.597 1.623.78 2.503.182.88.273 1.794.273 2.744 0 .95-.09 1.864-.273 2.744-.177.88-.435 1.716-.773 2.51a12.15 12.15 0 0 1-1.207 2.205.663.663 0 0 1-.418.298.556.556 0 0 1-.467-.097z" style="stroke-width:1.09867"/></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Are these all icons from a design on figma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you noticed this icon in Figma has size 28x28 so I had to change it to 24x24 and clean it up...
some Icons like the play button is not separated (black circle, play icon) so I had to improvise 😅.
I use Inkscape and svgomg for cleaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cool to request cleaned up ones from UI/UX instead as they have some weird or most likely unnecessary syntax/clipping rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
build-configs/common/theme/colors.ts
Outdated
themeContrast: string | ||
grayBackgroundColor: string | ||
slightlyDarkGray: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Do we really need three new colors? I'd prefer to reuse existing ones (at least where applicable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…useEffect with StartReading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
Change in average Code Health of affected files: +0.08 (9.70 -> 9.78)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested it with english and this works really good, nice job and thank you for taking on this big thing!
Nevertheless, I think your code still needs some polishing and cleaning up. Especially the concerns should be clear and listeners and useEffects should only be used if they are really necessary (in my opinion this should only be the case to initialize everything and for the tts-finish
listener to continue reading. If you have any questions about my comments or need some help, just let me know!
}) | ||
if (sentences.length === 0) { | ||
setSentenceIndex(0) | ||
setIsExpanded(false) | ||
} | ||
} else { | ||
setIsPlaying(false) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ I think responsibilities are currently a little unclear. This method should never have to set isPlaying to false again. Instead, it should only this method should set isPlaying to true at all (and only if there is actually something to play).
In my opinion, there should be the following methods/callback handlers:
- play
- pause
- stop (and/or close)
- playNext
- playPrevious
Each one of those should handle setting the correct state and edge cases (e.g. in the case of goForward, play should not be called if there is no next sentence).
So play
would look something like:
const play = (index = sentenceIndex) => {
Tts.stop() // or custom stop() if necessary
const sentence = sentences[index]
if (sentence) {
setIsPlaying(true)
Tts.speak(...)
}
}
while playNext
would look something like this:
const newIndex = index + 1
if (nextIndex < sentences.length) {
setSentenceIndex(nextIndex)
play(nextIndex)
}
and similarly for the other functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for overcomplicating things 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! I think you are doing a great job :)
const startReading = (index = sentenceIndex) => { | ||
if (!unsupportedLanguage && sentences.length > 0) { | ||
// Persian not supported | ||
Tts.setDefaultLanguage(languageCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 This belongs to initializeTts
, or is it necessary to call this again every single time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed if I changed the language while staying in the same component lifecycle I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good already, thanks for taking care of all those comments! A little more to go but we are getting there :)
@@ -83,6 +85,7 @@ const Header = ({ | |||
// Save route/canGoBack to state to prevent it from changing during navigating which would lead to flickering of the title and back button | |||
const [previousRoute] = useState(navigation.getState().routes[navigation.getState().routes.length - 2]) | |||
const [canGoBack] = useState(navigation.canGoBack()) | |||
const { enabled: isTtsActive, setVisible: setTtsPlayerVisible } = useTtsPlayer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙃 Please keep the naming consistent
const { enabled: isTtsActive, setVisible: setTtsPlayerVisible } = useTtsPlayer() | |
const { enabled: isTtsEnabled, setVisible: setTtsPlayerVisible } = useTtsPlayer() |
setSentences: (sentences: string[]) => void | ||
} | ||
|
||
export const ttsContext = createContext<TtsContextType>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 So far we are using upper case names for contexts, please change accoringly
export const ttsContext = createContext<TtsContextType>({ | |
export const TtsContext = createContext<TtsContextType>({ |
setSentenceIndex(0) | ||
} | ||
|
||
const initializeTts = useCallback((): void => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, you are right. Lets keep this as it is :)
useEffect(() => { | ||
if (sentences.length <= 1) { | ||
stop() | ||
} | ||
}, [sentences]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 This useEffect
should be removed by only passing a modified setSentences
down that both sets the sentences and stops playing.
handleBackward={playPrevious} | ||
handleForward={playNext} | ||
handleClose={close} | ||
pauseReading={pause} | ||
startPlaying={play} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧
handleBackward={playPrevious} | |
handleForward={playNext} | |
handleClose={close} | |
pauseReading={pause} | |
startPlaying={play} | |
playPrevious={playPrevious} | |
playNext={playNext} | |
close={close} | |
pause={pause} | |
play={play} |
setIsPlaying={setIsPlaying} | ||
sentenceIndex={sentenceIndex} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Both setIsPlaying
and sentenceIndex
should not be necessary anymore, could you adjust TtsPlayer
accordingly?
) | ||
|
||
const playPrevious = (index = sentenceIndex) => { | ||
const prevIndex = Math.max(0, index - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ The max
should not be needed anymore, right?
const prevIndex = Math.max(0, index - 1) | |
const previousIndex = index - 1 |
}) | ||
if (sentences.length === 0) { | ||
setSentenceIndex(0) | ||
setIsExpanded(false) | ||
} | ||
} else { | ||
setIsPlaying(false) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! I think you are doing a great job :)
const [isPlaying, setIsPlaying] = useState(false) | ||
const [sentenceIndex, setSentenceIndex] = useState(0) | ||
const [visible, setVisible] = useState(false) | ||
const [enabled, setEnabled] = useState(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ There is no state needed for this as it does need to be manually set (enabled is just dependent on other states/constant values). See comments #2950 (comment) and #2950 (comment)
This should be done here and only enabled
should be passed down in the context and exposed by useTtsPlayer
.
useEffect(() => { | ||
if (sentences) { | ||
tts.setSentences(sentences) | ||
} | ||
tts.setEnabled(enabled) | ||
return () => { | ||
tts.setSentences([]) | ||
tts.setEnabled(false) | ||
} | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [enabled, sentences, languageCode]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 There is no need to manually set enabled
, it should only be defined by other values.
useEffect(() => { | |
if (sentences) { | |
tts.setSentences(sentences) | |
} | |
tts.setEnabled(enabled) | |
return () => { | |
tts.setSentences([]) | |
tts.setEnabled(false) | |
} | |
// eslint-disable-next-line react-hooks/exhaustive-deps | |
}, [enabled, sentences, languageCode]) | |
useEffect(() => { | |
setSentences(sentences) | |
return () => setSentences([]) | |
}, [setSentences, sentences]) |
One more thing: Personally I'd be in favor of always showing the header item |
Short description
Adding text-to-speech functionality.
Proposed changes
TtsPlayer.tsx
: deals with html content and it passes it toreact-native-tts
as sentences. used debounce function from lodash to handle rapid volume change. I added an expandPlayer state just to keep player expanded when changes from sentence to sentence. This component located at app level where it wraps around the NavigationContainer to be able to be floating at the bottom of the screen while scrolling.useTtsPlayer.ts
: Is a hook used to pass data (content, title, visibility, volume) from across the app toTtsPlayer.tsx
usinguseContext
.Categories.tsx, News.tsx, Events.tsx each one is using the
useTtsPlayer.ts
hook to pass content and title.Created a component
Slider.tsx
instead of adding more libraires and used the ones we have:react-native-reanimated
&react-native-gesture-handler
to create slider that can be used anywhere other than the volume.Added ReadAloud HeaderButtonTitle to be selected from the kebab menu to show the player. This button will show up only in Integreat/IntegreatTestCms and at third level pages.
The TtsPlayer will stop when the app is in the background also if you back out from a 3rd level page.
Notes:
tts.speak()
? due toreact-native-tts
doesn't have a functional pause also passing word by word will sound more robotic.react-native-tts
.Side effects
Testing
Resolved issues
Fixes: #2916