2916: Add tts on android #2950
2916: Add tts on android #2950
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
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. |
|
It unfortunately also doesn't play any sound on real iOS, there is just this warning that pops up:
|
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 :) |
bahaaTuffaha
requested review from
steffenkleinle and
ztefanie
as code owners
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.
❓ Are these all icons from a design on figma?
There was a problem hiding this comment.
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.
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.
build-configs/common/theme/colors.ts
Outdated
| themeContrast: string | ||
| grayBackgroundColor: string | ||
| slightlyDarkGray: string |
There was a problem hiding this comment.
🔧 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.
Yes ... and these shades could make a difference in the look.
| } | ||
| }, [content, title, tts]) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
🔧 Please add all necessary dependencies instead of disabling linting rules. Linting rules should only be disabled as last resort if there is good reason for it.
There was a problem hiding this comment.
tts is an object that changes every time so its wrong to add it as dependency.. any suggestions?
There was a problem hiding this comment.
It shouldn't change on every render as you wrap it in a useMemo and all of its dependencies are from useState. However, the feeling that it changes too often might be because you also expose sentenceIndex which changes frequently. Since its not used anywhere, you can just remove that though.
There was a problem hiding this comment.
Tried to put setSentenceIndex in a callback function at TtsContainer but still showing:
Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.
| const TTS_STOP_DELAY = 100 // delay to make sure tts is stopped | ||
| await new Promise(resolve => { | ||
| setTimeout(resolve, TTS_STOP_DELAY) | ||
| }) |
There was a problem hiding this comment.
❓ Why is this needed? Please add the reasoning in the comment. Comments should address the why, not the how/what as the code should be self explanatory enough
There was a problem hiding this comment.
Sometimes Tts.stop() wont make the tts stop so I experimented with this way and it works so I left it as it is.
I will remove the comment.
…useEffect with StartReading
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
Short description
Adding text-to-speech functionality.
Proposed changes
TtsPlayer.tsx: deals with html content and it passes it toreact-native-ttsas 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.tsxusinguseContext.Categories.tsx, News.tsx, Events.tsx each one is using the
useTtsPlayer.tshook to pass content and title.Created a component
Slider.tsxinstead of adding more libraires and used the ones we have:react-native-reanimated&react-native-gesture-handlerto 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-ttsdoesn't have a functional pause also passing word by word will sound more robotic.react-native-tts.Side effects
Testing
Resolved issues
Fixes: #2916