search

COSC481W-2024Winter / JARVIS

J.A.R.V.I.S
2 stars 3 forks source link

Feature: Pitch, Language, Volume Completed #72

Closed dayshsweet closed 7 months ago

dayshsweet commented 7 months ago

addresses #59

This pull request is the completed version on my PBI.

there were many bugs in the previous pull requests, and they have been addressed.

ghost commented 7 months ago

completing second review:

checking tests. volume_tests.dart does pass but still produces a warning about going off screen.

=== begin testing app functionality.

main screen now displays three buttons properly image

settings page has been completely replaced with language, pitch and volume. image

volume is still tied to system volume image There is a button to hide that it is tied to system sound. @haohuazheng3 is this okay? I'm not sure if you want volume divorced from system volume. The current volume is tied to system volume, even if it is hidden by 'show system UI'.

switching to spanish does not change the contents (email, news, weather) to spanish, but changes the pronunciation (it does do numbers in the language. This may be out of scope.

the response from the stt and tts are good "como te llamas?" image

Chinese is still not working image

pitch works fine

image

The theming is not cohesive on the settings page, and there a few minor bugs: **TO BE FIXED:

  1. I would advise removing chinese as it doesn't work
  2. removing the "show system UI:true (Show/Hide UI)"

for what I think is out of scope would be:

  1. separating system volume and in app volume.
  2. fixing the themeing.
  3. actually changing the weather/news/emails to be spanish.

so I would just fix the first three bugs I mentioned. @haohuazheng3 if you have anything to add please do so.

ghost commented 7 months ago

current status is good. There isn't chinese, which the professor has indicated she would like to see. There is no issue at present merging this into main (as far as I can see) as it doesn't break anything else. If you run into errors implementing chinese, revert to ecd353b and we will use that.

haohuazheng3 commented 7 months ago

Overall, there are no issues. As Emily mentioned, we just need to add one more Chinese portion and this pull request is approved.