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.

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

Speech Profile Page Improvements #971

Merged
merged 11 commits into from
Oct 8, 2024
55 changes: 34 additions & 21 deletions app/lib/pages/onboarding/speech_profile_widget.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import 'dart:async';
import 'package:flutter/material.dart';
import 'package:flutter_provider_utilities/flutter_provider_utilities.dart';
import 'package:friend_private/backend/preferences.dart';
import 'package:friend_private/pages/speech_profile/percentage_bar_progress.dart';
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 import of percentage_bar_progress.dart is new. Ensure that this file exists and is accessible from the current file's location. If it doesn't exist or isn't accessible, it will cause a compilation error.

+ import 'package:friend_private/pages/speech_profile/percentage_bar_progress.dart';

import 'package:friend_private/providers/capture_provider.dart';
import 'package:friend_private/providers/speech_profile_provider.dart';
import 'package:friend_private/widgets/dialog.dart';
Expand Down Expand Up @@ -138,6 +139,23 @@ class _SpeechProfileWidgetState extends State<SpeechProfileWidget> with TickerPr
),
barrierDismissible: false,
);
} else if (error == "NO_SPEECH") {
showDialog(
context: context,
builder: (c) => getDialog(
context,
() {
Navigator.pop(context);
// Navigator.pop(context);
},
() {},
'Are you there?',
'We could not detect any speech. Please make sure to speak for at least 10 seconds and not more than 3 minutes.',
okButtonText: 'Ok',
singleButton: true,
),
barrierDismissible: false,
);
Comment on lines +142 to +158
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 error handling for "NO_SPEECH" has been improved. The new dialog provides a clearer message to the user about what went wrong and how to fix it. However, consider using a constant or enum for the error string "NO_SPEECH" to avoid potential typos and improve maintainability.

- } else if (error == "NO_SPEECH") {
+ } else if (error == SpeechErrors.NO_SPEECH) {

}
},
child: Container(
Expand All @@ -148,10 +166,10 @@ class _SpeechProfileWidgetState extends State<SpeechProfileWidget> with TickerPr
height: 10,
),
Padding(
padding: const EdgeInsets.fromLTRB(40, 20, 40, 20),
padding: EdgeInsets.fromLTRB(40, !provider.startedRecording ? 20 : 0, 40, 20),
child: !provider.startedRecording
? const Text(
'Now, Omi needs to learn your voice to be able to recognise you.',
'Omi needs to learn your voice to recognize you',
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 padding and text have been adjusted. The padding now depends on whether recording has started or not. The text has also been slightly modified. These changes seem to be aimed at improving the UI and UX.

- padding: const EdgeInsets.fromLTRB(40, 20, 40, 20),
+ padding: EdgeInsets.fromLTRB(40, !provider.startedRecording ? 20 : 0, 40, 20),
- 'Now, Omi needs to learn your voice to be able to recognise you.',
+ 'Omi needs to learn your voice to recognize you',

textAlign: TextAlign.center,
style: TextStyle(
color: Colors.white,
Expand Down Expand Up @@ -210,6 +228,7 @@ class _SpeechProfileWidgetState extends State<SpeechProfileWidget> with TickerPr
: Column(
mainAxisAlignment: MainAxisAlignment.end,
children: [
const Text("Note: This only works in English", style: TextStyle(color: Colors.white)),
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

A note about language support has been added. This is a good addition for user awareness.

+ const Text("Note: This only works in English", style: TextStyle(color: Colors.white)),

const SizedBox(height: 20),
Container(
padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 0),
Expand Down Expand Up @@ -295,30 +314,24 @@ class _SpeechProfileWidgetState extends State<SpeechProfileWidget> with TickerPr
: Column(
mainAxisSize: MainAxisSize.min,
children: [
const SizedBox(height: 30),
const SizedBox(height: 20),
provider.percentageCompleted > 0
? const SizedBox()
: const Text(
"Introduce\nyourself",
style: TextStyle(color: Colors.white, fontSize: 24, height: 1.4),
textAlign: TextAlign.center,
),
// const SizedBox(height: 30),

// const SizedBox(height: 18),
ProgressBarWithPercentage(progressValue: provider.percentageCompleted),
const SizedBox(height: 12),
Text(
provider.message,
style: TextStyle(color: Colors.grey.shade300, fontSize: 14, height: 1.4),
textAlign: TextAlign.center,
),
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 progress bar has been replaced with ProgressBarWithPercentage which provides a more detailed progress display. This is a good improvement for user experience. However, the hardcoded string "Introduce\nyourself" should be moved to a constants file or handled through localization.

const SizedBox(height: 18),
Padding(
padding: const EdgeInsets.symmetric(horizontal: 32),
child: Stack(
children: [
LinearProgressIndicator(
value: provider.percentageCompleted,
backgroundColor: Colors.grey.shade300,
valueColor: const AlwaysStoppedAnimation<Color>(Colors.deepPurple),
),
],
),
),
const SizedBox(height: 12),
Text(
'${(provider.percentageCompleted * 100).toInt()}%',
style: const TextStyle(color: Colors.white),
),
],
),
),
Expand Down
23 changes: 13 additions & 10 deletions app/lib/pages/speech_profile/page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import 'package:gradient_borders/box_borders/gradient_box_border.dart';
import 'package:intercom_flutter/intercom_flutter.dart';
import 'package:provider/provider.dart';

import 'percentage_bar_progress.dart';
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 import statement for percentage_bar_progress.dart has been added. This is necessary to use the new ProgressBarWithPercentage widget in this file.

+ import 'percentage_bar_progress.dart';


Comment on lines +19 to +20
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 import statement for percentage_bar_progress.dart has been added. This is necessary to use the new ProgressBarWithPercentage widget in this file.

+ import 'package:intercom_flutter/intercom_flutter.dart';
+ import 'package:provider/provider.dart';
+ 
+ import 'percentage_bar_progress.dart';

class SpeechProfilePage extends StatefulWidget {
final bool onbording;

Expand Down Expand Up @@ -114,7 +116,7 @@ class _SpeechProfilePageState extends State<SpeechProfilePage> with TickerProvid
Navigator.pop(context);
},
() {},
'Invalid recording detected',
'Multiple speakers detected',
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 error message title has been updated to be more specific about the error. This change improves user experience by providing clearer feedback.

- 'Invalid recording detected',
+ 'Multiple speakers detected',

'It seems like there are multiple speakers in the recording. Please make sure you are in a quiet location and try again.',
okButtonText: 'Try Again',
singleButton: true,
Expand Down Expand Up @@ -290,7 +292,7 @@ class _SpeechProfilePageState extends State<SpeechProfilePage> with TickerProvid
Align(
alignment: Alignment.bottomCenter,
child: Padding(
padding: const EdgeInsets.fromLTRB(24, 0, 24, 48),
padding: const EdgeInsets.fromLTRB(0, 0, 0, 48),
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 padding on the left and right of the bottom widget has been removed. This could affect the layout and appearance of the page. Ensure that this change aligns with the intended design.

- padding: const EdgeInsets.fromLTRB(24, 0, 24, 48),
+ padding: const EdgeInsets.fromLTRB(0, 0, 0, 48),

child: !provider.startedRecording
? Column(
mainAxisAlignment: MainAxisAlignment.end,
Expand Down Expand Up @@ -419,23 +421,24 @@ class _SpeechProfilePageState extends State<SpeechProfilePage> with TickerProvid
),
const SizedBox(height: 24),
Padding(
padding: const EdgeInsets.symmetric(horizontal: 32),
padding: const EdgeInsets.symmetric(horizontal: 0),
child: Stack(
children: [
// LinearProgressIndicator(
// backgroundColor: Colors.grey[300],
// valueColor: AlwaysStoppedAnimation<Color>(Colors.grey.withOpacity(0.3)),
// ),
LinearProgressIndicator(
value: provider.percentageCompleted,
backgroundColor:
Colors.grey.shade300, // Make sure background is transparent
valueColor: const AlwaysStoppedAnimation<Color>(Colors.deepPurple),
),
// LinearProgressIndicator(
// value: provider.percentageCompleted,
// backgroundColor:
// Colors.grey.shade300, // Make sure background is transparent
// valueColor: const AlwaysStoppedAnimation<Color>(Colors.deepPurple),
// ),
ProgressBarWithPercentage(progressValue: provider.percentageCompleted),
],
),
),
const SizedBox(height: 12),
const SizedBox(height: 18),
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 LinearProgressIndicator has been replaced with a custom ProgressBarWithPercentage widget. The padding around the progress bar has also been removed, and the space below the progress bar has been increased. These changes will alter the visual representation of the progress bar and its surrounding space.

- padding: const EdgeInsets.symmetric(horizontal: 32),
- LinearProgressIndicator(
-   value: provider.percentageCompleted,
-   backgroundColor: Colors.grey.shade300,
-   valueColor: const AlwaysStoppedAnimation<Color>(Colors.deepPurple),
- ),
- const SizedBox(height: 12),

+ padding: const EdgeInsets.symmetric(horizontal: 0),
+ ProgressBarWithPercentage(progressValue: provider.percentageCompleted),
+ const SizedBox(height: 18),

Text('${(provider.percentageCompleted * 100).toInt()}%',
style: const TextStyle(color: Colors.white)),
],
Expand Down
131 changes: 131 additions & 0 deletions app/lib/pages/speech_profile/percentage_bar_progress.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import 'package:flutter/material.dart';

class ProgressBarWithPercentage extends StatefulWidget {
final double progressValue;

const ProgressBarWithPercentage({super.key, required this.progressValue});
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 super.key in the constructor of ProgressBarWithPercentage is not valid. If you want to pass a key to the superclass, it should be done like this: Key key. Here's how to fix it:

- const ProgressBarWithPercentage({super.key, required this.progressValue});
+ const ProgressBarWithPercentage({Key? key, required this.progressValue}) : super(key: key);

@override
_ProgressBarWithPercentageState createState() => _ProgressBarWithPercentageState();
}

class _ProgressBarWithPercentageState extends State<ProgressBarWithPercentage> {
@override
Widget build(BuildContext context) {
return SizedBox(
height: 60,
width: 400,
child: Column(
crossAxisAlignment: CrossAxisAlignment.center,
mainAxisAlignment: MainAxisAlignment.end,
mainAxisSize: MainAxisSize.min,
children: [
SizedBox(
height: 46,
child: Stack(
children: [
Positioned(
left: widget.progressValue * (400 - 122),
child: ProgressBubble(
content: '${(widget.progressValue * 100).toInt()}%',
),
),
],
),
),
SizedBox(
width: 300,
height: 8,
child: ClipRRect(
borderRadius: const BorderRadius.all(Radius.circular(10)),
child: LinearProgressIndicator(
value: widget.progressValue,
backgroundColor: Colors.grey.shade300,
valueColor: const AlwaysStoppedAnimation<Color>(Colors.deepPurple),
),
),
),
],
),
);
}
}

class TrianglePainter extends CustomPainter {
final Color color;
final Color shadowColor;
final double triangleHeight;
final double triangleBaseWidth;

TrianglePainter({
required this.color,
required this.shadowColor,
this.triangleHeight = 10.0, // Default height
this.triangleBaseWidth = 10.0, // Default base width
});

@override
void paint(Canvas canvas, Size size) {
final paint = Paint()
..color = color
..style = PaintingStyle.fill;

final trianglePath = Path()
// Move to the left point of the base of the triangle
..moveTo(size.width / 2 - triangleBaseWidth / 2, 0)
// Draw a line to the right point of the base
..lineTo(size.width / 2 + triangleBaseWidth / 2, 0)
// Draw a line to the tip of the triangle (height)
..lineTo(size.width / 2, triangleHeight)
..close();

// Draw triangle
canvas.drawPath(trianglePath, paint);
}

@override
bool shouldRepaint(covariant CustomPainter oldDelegate) => false;
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 shouldRepaint method in TrianglePainter always returns false. This means that the triangle will never be repainted even if its properties change. If you want the triangle to repaint when its color or dimensions change, you should compare these properties with those of the old delegate. Here's how to fix it:

- bool shouldRepaint(covariant CustomPainter oldDelegate) => false;
+ bool shouldRepaint(covariant TrianglePainter oldDelegate) {
+   return oldDelegate.color != color ||
+          oldDelegate.shadowColor != shadowColor ||
+          oldDelegate.triangleHeight != triangleHeight ||
+          oldDelegate.triangleBaseWidth != triangleBaseWidth;
+ }

}
Comment on lines +56 to +90
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

In the TrianglePainter class, the shouldRepaint method is returning a constant value of false. This means that the triangle will never be repainted even if the properties change. If you want to optimize and avoid unnecessary repaints, you should compare the old delegate's properties with the new ones.

  @override
-89:   bool shouldRepaint(covariant CustomPainter oldDelegate) => false;
+89:   bool shouldRepaint(covariant TrianglePainter oldDelegate) {
+90:     return oldDelegate.color != color ||
+91:            oldDelegate.shadowColor != shadowColor ||
+92:            oldDelegate.triangleHeight != triangleHeight ||
+93:            oldDelegate.triangleBaseWidth != triangleBaseWidth;
+94:   }


class ProgressBubble extends StatelessWidget {
final String content;
final double triangleHeight;

const ProgressBubble({
super.key,
required this.content,
this.triangleHeight = 10,
});

@override
Widget build(BuildContext context) {
return Column(
mainAxisSize: MainAxisSize.min,
children: [
Container(
// margin: const EdgeInsets.symmetric(horizontal: 16),
decoration: BoxDecoration(
color: Colors.white,
borderRadius: BorderRadius.circular(12),
),
child: Padding(
padding: const EdgeInsets.all(10),
child: Text(
content,
style: const TextStyle(
fontSize: 14,
color: Colors.grey,
),
),
),
),
CustomPaint(
painter: TrianglePainter(
color: Colors.white,
shadowColor: Colors.grey.withOpacity(0.5),
),
size: Size(10, triangleHeight),
),
],
);
}
}
6 changes: 3 additions & 3 deletions app/lib/providers/speech_profile_provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class SpeechProfileProvider extends ChangeNotifier
BtDevice? device;

final targetWordsCount = 70;
final maxDuration = 90;
final maxDuration = 150;
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 maxDuration has been increased from 90 to 150. Ensure that this change is reflected in all parts of the application where maxDuration is used, and that it doesn't negatively impact other functionalities or performance.

- final maxDuration = 90;
+ final maxDuration = 150;

StreamSubscription<OnConnectionStateChangedEvent>? connectionStateListener;
List<TranscriptSegment> segments = [];
double? streamStartedAtSecond;
Expand Down Expand Up @@ -145,8 +145,8 @@ class SpeechProfileProvider extends ChangeNotifier
if (uploadingProfile || profileCompleted) return;

int duration = segments.isEmpty ? 0 : segments.last.end.toInt();
if (duration < 5 || duration > 120) {
notifyError('INVALID_RECORDING');
if (duration < 10 || duration > 155) {
notifyError('No_SPEECH');
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 validation logic for duration has been updated to reflect the new minimum and maximum durations (10 and 155 respectively). This change is consistent with the update to maxDuration. However, the error message 'No_SPEECH' might be misleading if the duration is greater than 155. Consider using a more descriptive error message.

- notifyError('No_SPEECH');
+ notifyError(duration < 10 ? 'SPEECH_TOO_SHORT' : 'SPEECH_TOO_LONG');

}

String text = segments.map((e) => e.text).join(' ').trim();
Expand Down
Loading