Improve speech profile widget handling #871
Conversation
|
WalkthroughThis update introduces a new widget, Changes
Uplevel your code reviews with Entelligence.AI ProEntelligence.AI ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (5)
- app/lib/pages/capture/_page.dart (1)
- app/lib/pages/capture/widgets/widgets.dart (2)
- app/lib/pages/memories/page.dart (1)
- app/lib/pages/onboarding/wrapper.dart (1)
- app/lib/providers/home_provider.dart (2)
Review comments generated (9)
- Review: 4
- LGTM: 5
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -127,7 +127,7 @@ class _OnboardingWrapperState extends State<OnboardingWrapper> with TickerProvid | |||
| SpeechProfileWidget( | |||
| goNext: () { | |||
| if (context.read<SpeechProfileProvider>().memory == null) { | |||
| _controller!.animateTo(_controller!.index + 2); | |||
| routeToPage(context, const HomePageWrapper(), replace: true); | |||
There was a problem hiding this comment.
The change from an animation call to a route navigation call could potentially disrupt the user experience. If the SpeechProfileProvider().memory is null, the user is now taken directly to the HomePageWrapper, skipping any remaining onboarding steps. This might be confusing for users who are expecting a smooth onboarding process. Please ensure that this abrupt transition is intended and does not negatively impact the user experience.
- routeToPage(context, const HomePageWrapper(), replace: true);
+ _controller!.animateTo(_controller!.index + 2);| class SpeechProfileCardWidget extends StatelessWidget { | ||
| const SpeechProfileCardWidget({super.key}); | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return Consumer<HomeProvider>( | ||
| builder: (context, provider, child) { | ||
| if (provider.isLoading) return const SizedBox(); | ||
| return provider.hasSpeakerProfile | ||
| ? const SizedBox() | ||
| : Stack( | ||
| children: [ | ||
| GestureDetector( | ||
| onTap: () async { | ||
| MixpanelManager().pageOpened('Speech Profile Memories'); | ||
| bool hasSpeakerProfile = SharedPreferencesUtil().hasSpeakerProfile; | ||
| await routeToPage(context, const SpeechProfilePage()); | ||
| if (hasSpeakerProfile != SharedPreferencesUtil().hasSpeakerProfile) { | ||
| if (context.mounted) { | ||
| // TODO: is the websocket restarting once the user comes back? | ||
| context.read<DeviceProvider>().restartWebSocket(); | ||
| } | ||
| } | ||
| }, | ||
| child: Container( | ||
| decoration: BoxDecoration( | ||
| color: Colors.grey.shade900, | ||
| borderRadius: const BorderRadius.all(Radius.circular(12)), | ||
| ), | ||
| margin: const EdgeInsets.fromLTRB(16, 0, 16, 16), | ||
| padding: const EdgeInsets.all(16), | ||
| child: const Row( | ||
| mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
| children: [ | ||
| Icon(Icons.multitrack_audio), | ||
| SizedBox(width: 16), | ||
| Text( | ||
| 'Teach Omi your voice', | ||
| style: TextStyle(color: Colors.white, fontSize: 16), | ||
| Expanded( | ||
| child: Row( | ||
| children: [ | ||
| Icon(Icons.multitrack_audio), | ||
| SizedBox(width: 16), | ||
| Text( | ||
| 'Teach Omi your voice', | ||
| style: TextStyle(color: Colors.white, fontSize: 16), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| Icon(Icons.arrow_forward_ios) | ||
| ], | ||
| ), | ||
| ), | ||
| Icon(Icons.arrow_forward_ios) | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| Positioned( | ||
| top: 12, | ||
| right: 24, | ||
| child: Container( | ||
| width: 12, | ||
| height: 12, | ||
| decoration: const BoxDecoration(color: Colors.red, shape: BoxShape.circle), | ||
| ), | ||
| ), | ||
| ], | ||
| ) | ||
| : const SizedBox(height: 0); | ||
| ), | ||
| Positioned( | ||
| top: 12, | ||
| right: 24, | ||
| child: Container( | ||
| width: 12, | ||
| height: 12, | ||
| decoration: const BoxDecoration(color: Colors.red, shape: BoxShape.circle), | ||
| ), | ||
| ), | ||
| ], | ||
| ); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The refactoring from a function to a widget is a good step towards better modularity. However, there's a potential issue with the SharedPreferencesUtil().hasSpeakerProfile check. This check is performed twice: once before navigating to the SpeechProfilePage and once after returning from it. If the value of hasSpeakerProfile changes while on the SpeechProfilePage, the second check will not reflect the updated value because SharedPreferencesUtil() might not have the latest data immediately after the change.
To ensure that you always have the most recent value, consider using an async getter method for hasSpeakerProfile in SharedPreferencesUtil and await its result.
- bool hasSpeakerProfile = SharedPreferencesUtil().hasSpeakerProfile;
+ bool hasSpeakerProfile = await SharedPreferencesUtil().getHasSpeakerProfile();And similarly,
- if (hasSpeakerProfile != SharedPreferencesUtil().hasSpeakerProfile) {
+ if (hasSpeakerProfile != await SharedPreferencesUtil().getHasSpeakerProfile()) {This way, you're sure to get the most recent value from shared preferences.
| void setSpeakerProfile(bool? value) { | ||
| hasSpeakerProfile = value ?? SharedPreferencesUtil().hasSpeakerProfile; | ||
| notifyListeners(); |
There was a problem hiding this comment.
The setSpeakerProfile method is using a default value from SharedPreferencesUtil().hasSpeakerProfile if the provided value is null. This could lead to unexpected behavior if the shared preferences value hasn't been initialized yet. It would be better to explicitly handle the null case.
- hasSpeakerProfile = value ?? SharedPreferencesUtil().hasSpeakerProfile;
+ if (value != null) {
+ hasSpeakerProfile = value;
+ }| Future setupHasSpeakerProfile() async { | ||
| SharedPreferencesUtil().hasSpeakerProfile = await userHasSpeakerProfile(); | ||
| setIsLoading(true); | ||
| var res = await userHasSpeakerProfile(); | ||
| setSpeakerProfile(res); | ||
| SharedPreferencesUtil().hasSpeakerProfile = res; | ||
| debugPrint('_setupHasSpeakerProfile: ${SharedPreferencesUtil().hasSpeakerProfile}'); | ||
| MixpanelManager().setUserProperty('Speaker Profile', SharedPreferencesUtil().hasSpeakerProfile); | ||
| setIsLoading(false); |
There was a problem hiding this comment.
The setupHasSpeakerProfile method is setting isLoading to true at the start and false at the end, but there's no error handling in case userHasSpeakerProfile() throws an exception. This could leave isLoading set to true indefinitely. Consider adding a try-catch block to ensure isLoading is set to false even when an error occurs.
- setIsLoading(true);
- var res = await userHasSpeakerProfile();
- setSpeakerProfile(res);
- SharedPreferencesUtil().hasSpeakerProfile = res;
- debugPrint('_setupHasSpeakerProfile: ${SharedPreferencesUtil().hasSpeakerProfile}');
- MixpanelManager().setUserProperty('Speaker Profile', SharedPreferencesUtil().hasSpeakerProfile);
- setIsLoading(false);
+ try {
+ setIsLoading(true);
+ var res = await userHasSpeakerProfile();
+ setSpeakerProfile(res);
+ SharedPreferencesUtil().hasSpeakerProfile = res;
+ debugPrint('_setupHasSpeakerProfile: ${SharedPreferencesUtil().hasSpeakerProfile}');
+ MixpanelManager().setUserProperty('Speaker Profile', SharedPreferencesUtil().hasSpeakerProfile);
+ } catch (e) {
+ // Handle or log the error as needed
+ } finally {
+ setIsLoading(false);
+ }
There was a problem hiding this comment.
Files selected (1)
- app/lib/pages/home/page.dart (2)
Review comments generated (2)
- Review: 2
- LGTM: 0
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -53,6 +53,7 @@ class _HomePageWrapperState extends State<HomePageWrapper> { | |||
| context.read<DeviceProvider>().periodicConnect('coming from HomePageWrapper'); | |||
| await context.read<mp.MemoryProvider>().getInitialMemories(); | |||
| context.read<PluginProvider>().setSelectedChatPluginId(null); | |||
| await context.read<HomeProvider>().setupHasSpeakerProfile(); | |||
There was a problem hiding this comment.
The setupHasSpeakerProfile function is now being called in the initState method. This is a good change as it ensures that the speaker profile setup is done as soon as the widget is initialized. However, please ensure that any potential exceptions thrown by setupHasSpeakerProfile are properly handled to prevent the app from crashing during initialization.
await context.read<HomeProvider>().setupHasSpeakerProfile();
+ .catchError((error) {
+ // Handle error appropriately
+ });| if (mounted) { | ||
| await context.read<HomeProvider>().setUserPeople(); | ||
| } | ||
| await context.read<HomeProvider>().setUserPeople(); |
There was a problem hiding this comment.
The call to setUserPeople has been moved outside of the nested if (mounted) check and is now directly under the first if (mounted) check. This is a good change as it simplifies the code and reduces unnecessary nesting. However, similar to the previous comment, please ensure that any potential exceptions thrown by setUserPeople are properly handled.
await context.read<HomeProvider>().setUserPeople();
+ .catchError((error) {
+ // Handle error appropriately
+ });
There was a problem hiding this comment.
Files selected (1)
- app/lib/pages/speech_profile/page.dart (1)
Review comments generated (1)
- Review: 0
- LGTM: 1
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
ScreenRecording_09-17-2024.10-57-33_1.1.mp4
Summary by Entelligence.AI
SpeechProfileCardWidget, a new widget for managing speech profiles, enhancing the user interface and experience.HomePageWrapper.HomePageWrapperandHomePageclasses, ensuring smoother setup of speaker profile and user people.