Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve speech profile widget handling #871

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Improve speech profile widget handling #871

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7ff072
improve speech profile card handling
mdmohsin7 Sep 17, 2024
10ce9df
minor onboarding navigation fix
mdmohsin7 Sep 17, 2024
9166b51
move hasSpeakerProfile a bit more up for faster loading in UI
mdmohsin7 Sep 17, 2024
9e5581c
Replace friend with Omi in speech profile setup page
mdmohsin7 Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/lib/pages/capture/_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ class CapturePageState extends State<CapturePage> with AutomaticKeepAliveClientM
child: Stack(
children: [
ListView(children: [
speechProfileWidget(context),
SpeechProfileCardWidget(),
...getConnectionStateWidgets(
context,
provider.hasTranscripts,
Expand Down
111 changes: 61 additions & 50 deletions app/lib/pages/capture/widgets/widgets.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:friend_private/pages/capture/connect.dart';
import 'package:friend_private/pages/speech_profile/page.dart';
import 'package:friend_private/providers/connectivity_provider.dart';
import 'package:friend_private/providers/device_provider.dart';
import 'package:friend_private/providers/home_provider.dart';
import 'package:friend_private/utils/analytics/mixpanel.dart';
import 'package:friend_private/utils/enums.dart';
import 'package:friend_private/utils/other/temp.dart';
Expand Down Expand Up @@ -207,61 +208,71 @@ _getNoFriendConnectedYet(BuildContext context) {
);
}

speechProfileWidget(BuildContext context) {
return !SharedPreferencesUtil().hasSpeakerProfile
? 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: [
Expanded(
child: Row(
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),
),
),
],
);
},
);
}
Comment on lines +211 to +275
Copy link

Choose a reason for hiding this comment

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

Image description Entelligence.AI

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.

}

getTranscriptWidget(
Expand Down
2 changes: 1 addition & 1 deletion app/lib/pages/memories/page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class _MemoriesPageState extends State<MemoriesPage> with AutomaticKeepAliveClie
child: CustomScrollView(
slivers: [
const SliverToBoxAdapter(child: SizedBox(height: 32)),
SliverToBoxAdapter(child: speechProfileWidget(context)),
const SliverToBoxAdapter(child: SpeechProfileCardWidget()),
SliverToBoxAdapter(child: getMemoryCaptureWidget()),
if (memoryProvider.memoriesWithDates.isEmpty && !memoryProvider.isLoadingMemories)
const SliverToBoxAdapter(
Expand Down
2 changes: 1 addition & 1 deletion app/lib/pages/onboarding/wrapper.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

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

Image description Entelligence.AI

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);

} else {
_goNext();
}
Expand Down
18 changes: 17 additions & 1 deletion app/lib/providers/home_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ class HomeProvider extends ChangeNotifier {
final FocusNode chatFieldFocusNode = FocusNode();
bool isMemoryFieldFocused = false;
bool isChatFieldFocused = false;
bool hasSpeakerProfile = false;
bool isLoading = false;

HomeProvider() {
memoryFieldFocusNode.addListener(_onFocusChange);
Expand All @@ -27,10 +29,24 @@ class HomeProvider extends ChangeNotifier {
notifyListeners();
}

void setIsLoading(bool loading) {
isLoading = loading;
notifyListeners();
}

void setSpeakerProfile(bool? value) {
hasSpeakerProfile = value ?? SharedPreferencesUtil().hasSpeakerProfile;
notifyListeners();
Comment on lines +37 to +39
Copy link

Choose a reason for hiding this comment

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

Image description Entelligence.AI

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);
Comment on lines 42 to +49
Copy link

Choose a reason for hiding this comment

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

Image description Entelligence.AI

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);
+    }

notifyListeners();
}

Expand Down
Loading