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

Add ability to change speaker names; Need to test #1248

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+56 −1
Open

Add ability to change speaker names; Need to test #1248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions app/lib/backend/preferences.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,4 +424,12 @@ class SharedPreferencesUtil {
Future<bool> clear() async {
return await _preferences?.clear() ?? false;
}

Future<bool> saveSpeakerName(int speakerId, String name) async {
return await saveString('speaker_name_$speakerId', name);
}

String? getSpeakerName(int speakerId) {
return getString('speaker_name_$speakerId');
}
}
28 changes: 28 additions & 0 deletions app/lib/pages/memory_detail/memory_detail_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,4 +258,32 @@ class MemoryDetailProvider extends ChangeNotifier with MessageNotifierMixin {
assignMemoryTranscriptSegment(memoryId, segmentIdx);
notifyListeners();
}

Future<void> updateSpeakerName(int speakerId, String newName, bool updateAll) async {
toggleEditSegmentLoading(true);

try {
if (updateAll) {
// Update all segments with matching speakerId
for (var segment in memory.transcriptSegments) {
if (segment.speakerId == speakerId) {
segment.speaker = 'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
// Store the display name mapping in SharedPreferences
await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
}
}
} else {
// Update single segment
memory.transcriptSegments[selectedSegmentIndex].speaker =
'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
Comment on lines +277 to +278
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix undefined variable usage.

The code references selectedSegmentIndex which appears to be undefined. This could cause runtime errors.

Add the missing property to the class:

+  int selectedSegmentIndex = 0;  // Add appropriate initialization

Committable suggestion skipped: line range outside the PR's diff.

await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
}

notifyListeners();
} catch (e) {
showError('Failed to update speaker name');
} finally {
toggleEditSegmentLoading(false);
}
}
Comment on lines +262 to +288
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation and improve error handling.

The method lacks input validation and has generic error handling. Consider adding parameter validation and specific error handling.

   Future<void> updateSpeakerName(int speakerId, String newName, bool updateAll) async {
+    if (speakerId < 0) {
+      throw ArgumentError('Speaker ID must be non-negative');
+    }
+    if (newName.trim().isEmpty) {
+      throw ArgumentError('Speaker name cannot be empty');
+    }
+
     toggleEditSegmentLoading(true);
     
     try {
       if (updateAll) {
         // Update all segments with matching speakerId
         for (var segment in memory.transcriptSegments) {
           if (segment.speakerId == speakerId) {
             segment.speaker = 'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
             // Store the display name mapping in SharedPreferences
             await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
           }
         }
       } else {
         // Update single segment
         memory.transcriptSegments[selectedSegmentIndex].speaker = 
             'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
         await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
       }
       
       notifyListeners();
-    } catch (e) {
-      showError('Failed to update speaker name');
+    } catch (e, stackTrace) {
+      showError('Failed to update speaker name: ${e.toString()}');
+      debugPrint('Speaker name update failed: $e\n$stackTrace');
     } finally {
       toggleEditSegmentLoading(false);
     }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Future<void> updateSpeakerName(int speakerId, String newName, bool updateAll) async {
toggleEditSegmentLoading(true);
try {
if (updateAll) {
// Update all segments with matching speakerId
for (var segment in memory.transcriptSegments) {
if (segment.speakerId == speakerId) {
segment.speaker = 'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
// Store the display name mapping in SharedPreferences
await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
}
}
} else {
// Update single segment
memory.transcriptSegments[selectedSegmentIndex].speaker =
'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
}
notifyListeners();
} catch (e) {
showError('Failed to update speaker name');
} finally {
toggleEditSegmentLoading(false);
}
}
Future<void> updateSpeakerName(int speakerId, String newName, bool updateAll) async {
if (speakerId < 0) {
throw ArgumentError('Speaker ID must be non-negative');
}
if (newName.trim().isEmpty) {
throw ArgumentError('Speaker name cannot be empty');
}
toggleEditSegmentLoading(true);
try {
if (updateAll) {
// Update all segments with matching speakerId
for (var segment in memory.transcriptSegments) {
if (segment.speakerId == speakerId) {
segment.speaker = 'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
// Store the display name mapping in SharedPreferences
await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
}
}
} else {
// Update single segment
memory.transcriptSegments[selectedSegmentIndex].speaker =
'SPEAKER_${speakerId.toString().padLeft(2, '0')}';
await SharedPreferencesUtil().saveSpeakerName(speakerId, newName);
}
notifyListeners();
} catch (e, stackTrace) {
showError('Failed to update speaker name: ${e.toString()}');
debugPrint('Speaker name update failed: $e\n$stackTrace');
} finally {
toggleEditSegmentLoading(false);
}
}

}
19 changes: 19 additions & 0 deletions app/lib/pages/memory_detail/page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,25 @@ class EditSegmentWidget extends StatelessWidget {
// )
// : const SizedBox(),
const SizedBox(height: 12),
ListTile(
title: Text('Edit Speaker ${provider.memory.transcriptSegments[segmentIdx].speakerId} Name'),
trailing: const Icon(Icons.edit),
onTap: () {
showDialog(
context: context,
builder: (context) => EditSpeakerNameDialog(
speakerId: provider.memory.transcriptSegments[segmentIdx].speakerId,
onUpdate: (name, updateAll) {
provider.updateSpeakerName(
provider.memory.transcriptSegments[segmentIdx].speakerId,
name,
updateAll,
);
},
),
);
},
),
Comment on lines +469 to +487
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving the speaker name editing UX.

The implementation could benefit from the following improvements:

  1. Show the current speaker name instead of just the ID in the title
  2. Add loading state handling during name updates
  3. Add error handling and user feedback for failed updates
   ListTile(
-    title: Text('Edit Speaker ${provider.memory.transcriptSegments[segmentIdx].speakerId} Name'),
+    title: Row(
+      children: [
+        Text('Edit Speaker Name: '),
+        Text(
+          provider.getSpeakerName(provider.memory.transcriptSegments[segmentIdx].speakerId) ?? 
+          'Speaker ${provider.memory.transcriptSegments[segmentIdx].speakerId}',
+          style: const TextStyle(fontWeight: FontWeight.bold),
+        ),
+      ],
+    ),
     trailing: const Icon(Icons.edit),
     onTap: () {
+      if (provider.editSegmentLoading) return;
       showDialog(
         context: context,
         builder: (context) => EditSpeakerNameDialog(
           speakerId: provider.memory.transcriptSegments[segmentIdx].speakerId,
           onUpdate: (name, updateAll) {
+            try {
+              provider.toggleEditSegmentLoading(true);
               provider.updateSpeakerName(
                 provider.memory.transcriptSegments[segmentIdx].speakerId,
                 name,
                 updateAll,
               );
+              ScaffoldMessenger.of(context).showSnackBar(
+                const SnackBar(content: Text('Speaker name updated successfully!')),
+              );
+            } catch (e) {
+              ScaffoldMessenger.of(context).showSnackBar(
+                const SnackBar(content: Text('Failed to update speaker name. Please try again.')),
+              );
+            } finally {
+              provider.toggleEditSegmentLoading(false);
+            }
           },
         ),
       );
     },
   ),

Committable suggestion skipped: line range outside the PR's diff.

CheckboxListTile(
title: const Text('Yours'),
value: provider.memory.transcriptSegments[segmentIdx].isUser,
Expand Down
2 changes: 1 addition & 1 deletion app/lib/widgets/transcript.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class _TranscriptWidgetState extends State<TranscriptWidget> {
: 'You'
: data.personId != null
? person?.name ?? 'Deleted Person'
: 'Speaker ${data.speakerId}',
: SharedPreferencesUtil().getSpeakerName(data.speakerId) ?? 'Speaker ${data.speakerId}',
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider caching speaker names to improve performance.

The direct call to SharedPreferencesUtil().getSpeakerName() on every build could impact performance, especially in lists with many items. Consider caching the speaker names at a higher level or using a state management solution.

-: SharedPreferencesUtil().getSpeakerName(data.speakerId) ?? 'Speaker ${data.speakerId}',
+: _getSpeakerNameFromCache(data.speakerId),

+String _getSpeakerNameFromCache(int speakerId) {
+  // TODO: Implement caching mechanism
+  return SharedPreferencesUtil().getSpeakerName(speakerId) ?? 'Speaker $speakerId';
+}

Committable suggestion skipped: line range outside the PR's diff.

style: const TextStyle(
// person != null ? speakerColors[person.colorIdx!] :
color: Colors.white,
Expand Down