Add ability to change speaker names; Need to test #1248
Conversation
WalkthroughThe changes introduce new methods for managing speaker names in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MemoryDetailProvider
participant SharedPreferencesUtil
participant MemoryDetailPage
participant TranscriptWidget
User->>MemoryDetailPage: Edit Speaker Name
MemoryDetailPage->>MemoryDetailProvider: updateSpeakerName(speakerId, newName, updateAll)
MemoryDetailProvider->>SharedPreferencesUtil: saveSpeakerName(speakerId, newName)
SharedPreferencesUtil-->>MemoryDetailProvider: Success/Failure
MemoryDetailProvider->>User: Display Success/Failure Message
User->>TranscriptWidget: View Updated Transcript
TranscriptWidget->>SharedPreferencesUtil: getSpeakerName(speakerId)
SharedPreferencesUtil-->>TranscriptWidget: Return Speaker Name
TranscriptWidget->>User: Display Transcript with Speaker Name
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
app/lib/widgets/transcript.dart (1)
85-85: Consider moving speaker name logic to a dedicated provider.The direct usage of SharedPreferencesUtil in the widget layer violates separation of concerns. Consider implementing a dedicated SpeakerProvider to handle speaker-related operations.
Example provider pattern:
class SpeakerProvider extends ChangeNotifier { final Map<int, String> _speakerNames = {}; String getSpeakerName(int speakerId) { if (!_speakerNames.containsKey(speakerId)) { _speakerNames[speakerId] = SharedPreferencesUtil().getSpeakerName(speakerId) ?? 'Speaker $speakerId'; } return _speakerNames[speakerId]!; } Future<void> updateSpeakerName(int speakerId, String name) async { await SharedPreferencesUtil().setSpeakerName(speakerId, name); _speakerNames[speakerId] = name; notifyListeners(); } }app/lib/pages/memory_detail/memory_detail_provider.dart (2)
262-262: Add method documentation.Consider adding documentation to explain the method's purpose, parameters, and behavior.
+ /// Updates the speaker name for transcript segments. + /// + /// Parameters: + /// - [speakerId]: The ID of the speaker to update + /// - [newName]: The new name for the speaker + /// - [updateAll]: If true, updates all segments with matching speakerId + /// + /// Throws an exception if the update fails. Future<void> updateSpeakerName(int speakerId, String newName, bool updateAll) async {
270-270: Extract speaker name format to a constant.The speaker name format is hardcoded in multiple places. Consider extracting it to a constant for better maintainability.
+ static const _speakerNameFormat = 'SPEAKER_%02d'; + Future<void> updateSpeakerName(int speakerId, String newName, bool updateAll) async { // ... - segment.speaker = 'SPEAKER_${speakerId.toString().padLeft(2, '0')}'; + segment.speaker = sprintf(_speakerNameFormat, [speakerId]); // ... - 'SPEAKER_${speakerId.toString().padLeft(2, '0')}'; + sprintf(_speakerNameFormat, [speakerId]);Don't forget to add the required import:
import 'package:sprintf/sprintf.dart';Also applies to: 278-278
app/lib/backend/preferences.dart (2)
427-434: Implementation looks good but needs input validation.The methods are well-structured and consistent with the class's patterns. However, consider adding input validation for robustness.
Consider these improvements:
Future<bool> saveSpeakerName(int speakerId, String name) async { + if (speakerId < 0) { + throw ArgumentError('Speaker ID must be non-negative'); + } + if (name.trim().isEmpty) { + throw ArgumentError('Speaker name cannot be empty'); + } return await saveString('speaker_name_$speakerId', name); } String? getSpeakerName(int speakerId) { + if (speakerId < 0) { + throw ArgumentError('Speaker ID must be non-negative'); + } return getString('speaker_name_$speakerId'); }Also consider adding these utility methods:
/// Removes the name associated with the given speaker ID Future<bool> removeSpeakerName(int speakerId) async { if (speakerId < 0) { throw ArgumentError('Speaker ID must be non-negative'); } return await remove('speaker_name_$speakerId'); } /// Returns a Map of all speaker IDs to their names Map<int, String> getAllSpeakerNames() { final Map<int, String> speakers = {}; final pattern = RegExp(r'^speaker_name_(\d+)$'); _preferences?.getKeys() .where((key) => pattern.hasMatch(key)) .forEach((key) { final match = pattern.firstMatch(key); final id = int.parse(match!.group(1)!); final name = getString(key); if (name != null) { speakers[id] = name; } }); return speakers; }
427-434: Add documentation for the new methods.Consider adding dartdoc comments to improve API clarity.
+ /// Saves a name for the specified speaker ID. + /// + /// [speakerId] must be non-negative. + /// [name] must not be empty or only whitespace. + /// + /// Returns true if the operation was successful, false otherwise. + /// + /// Throws [ArgumentError] if [speakerId] is negative or [name] is empty. Future<bool> saveSpeakerName(int speakerId, String name) async { + /// Retrieves the name associated with the specified speaker ID. + /// + /// [speakerId] must be non-negative. + /// + /// Returns the speaker's name, or null if no name is set. + /// + /// Throws [ArgumentError] if [speakerId] is negative. String? getSpeakerName(int speakerId) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/lib/backend/preferences.dart(1 hunks)app/lib/pages/memory_detail/memory_detail_provider.dart(1 hunks)app/lib/pages/memory_detail/page.dart(1 hunks)app/lib/widgets/transcript.dart(1 hunks)
🔇 Additional comments (4)
app/lib/widgets/transcript.dart (1)
85-85: Verify SharedPreferencesUtil implementation.
Need to ensure that getSpeakerName is properly implemented and handles edge cases.
✅ Verification successful
SharedPreferencesUtil implementation is properly handling edge cases
The implementation of getSpeakerName and its underlying getString method is correctly implemented with proper null safety:
getSpeakerNameuses a consistent key formatspeaker_name_$speakerId- It properly returns
nullwhen no value is found - The transcript widget handles the null case by falling back to 'Speaker ${data.speakerId}'
- The SharedPreferencesUtil class follows the singleton pattern and properly initializes preferences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getSpeakerName implementation and its tests
echo "Searching for getSpeakerName implementation:"
rg -A 5 "getSpeakerName.*speakerId.*"
echo "\nSearching for related tests:"
fd -e dart -e test | xargs rg "getSpeakerName.*test"
Length of output: 1045
Script:
#!/bin/bash
# Let's check the full implementation of SharedPreferencesUtil and its getString method
echo "Checking SharedPreferencesUtil implementation:"
rg -A 10 "class SharedPreferencesUtil" app/lib/backend/preferences.dart
echo "\nChecking getString implementation:"
rg -A 5 "getString\(" app/lib/backend/preferences.dart
echo "\nChecking if there are any tests for SharedPreferencesUtil:"
fd test | xargs rg "SharedPreferencesUtil"
Length of output: 6364
app/lib/pages/memory_detail/memory_detail_provider.dart (1)
262-288: Consider adding concurrency protection.
The method modifies shared state without protection against concurrent modifications. Consider adding synchronization or using a mutex.
Let's verify if there are other places in the codebase that might modify the transcript segments concurrently:
app/lib/backend/preferences.dart (1)
427-434: Verify error handling in components using these methods.
Let's verify how errors are handled in components using these methods.
✅ Verification successful
Let me generate additional scripts to check the error handling patterns more thoroughly.
Based on the results from both script executions, I can now generate the final response:
Error handling is properly implemented for speaker name operations
saveSpeakerNameis wrapped in try-catch blocks in MemoryDetailProvider with appropriate error message displaygetSpeakerNamehas null-safety handling in TranscriptWidget with fallback to default speaker name- The underlying
saveStringoperation returns a Future indicating success/failure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in components using speaker name methods
# Look for usage of saveSpeakerName and error handling
echo "Checking saveSpeakerName usage and error handling:"
rg -A 5 "saveSpeakerName" --type dart
# Look for usage of getSpeakerName and null handling
echo "Checking getSpeakerName usage and null handling:"
rg -A 5 "getSpeakerName" --type dart
# Look for try-catch blocks around these calls
echo "Checking error handling patterns:"
rg -B 5 -A 5 "try.*catch.*saveSpeakerName" --type dart
Length of output: 2707
Script:
#!/bin/bash
# Check for try-catch blocks in MemoryDetailProvider
echo "Checking error handling in MemoryDetailProvider:"
rg -B 10 -A 5 "catch.*showError.*'Failed to update speaker name'" --type dart
# Check implementation of saveString to understand underlying error handling
echo "Checking saveString implementation:"
ast-grep --pattern 'Future<bool> saveString($_, $_) {
$$$
}'
# Check if there are any other error handling patterns around speaker operations
echo "Checking speaker-related error handling:"
rg -B 5 -A 5 "speaker.*try|try.*speaker" --type dart
Length of output: 2484
app/lib/pages/memory_detail/page.dart (1)
469-487: Verify the integration with speaker profile updates.
Since this feature modifies speaker names, ensure it properly integrates with the speech profile functionality used elsewhere in the file.
✅ Verification successful
Based on the search results, I can now verify the integration with speaker profile updates. The code shows that the speaker name update functionality is properly integrated with the rest of the codebase:
-
The speaker name update is handled by
updateSpeakerNamemethod inmemory_detail_provider.dartwhich:- Updates the speaker name in SharedPreferences storage
- Updates all segments with matching speakerId if updateAll is true
- Properly updates the UI through notifyListeners()
-
The updated speaker names are correctly used in:
- Transcript display (
widgets/transcript.dart) - Memory detail page (
pages/memory_detail/page.dart)
- Transcript display (
-
The speaker ID system is consistently used across:
- TranscriptSegment model
- Memory processing
- UI components
The speaker name update functionality is properly integrated and maintains consistency with the rest of the codebase's speaker identification system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for speech profile related code that might need updates
# Test: Search for speech profile related code
echo "Searching for speech profile related code..."
rg -A 5 "speech.*profile|speaker.*profile"
# Test: Search for speaker name usage
echo "Searching for speaker name usage..."
rg -A 5 "speaker.*name|speakerId"
Length of output: 99596
| @@ -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}', | |||
There was a problem hiding this comment.
🛠️ 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.
| memory.transcriptSegments[selectedSegmentIndex].speaker = | ||
| 'SPEAKER_${speakerId.toString().padLeft(2, '0')}'; |
There was a problem hiding this comment.
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 initializationCommittable suggestion skipped: line range outside the PR's diff.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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); | |
| } | |
| } |
| 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, | ||
| ); | ||
| }, | ||
| ), | ||
| ); | ||
| }, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the speaker name editing UX.
The implementation could benefit from the following improvements:
- Show the current speaker name instead of just the ID in the title
- Add loading state handling during name updates
- 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.
|
Hi @sanjayamirthraj, we had this feature back then (implemented and later removed by Joan). Are you directly uncommenting that feature? If not, then doing that would makes sense I guess (since that was already working) |
|
Oh I did not see that commented; I reimplemented the function. I’m not sure if the feature is commented out or removed completely |
|
omi/app/lib/pages/memory_detail/page.dart Line 338 in 021fa1e You can check this out |
@mdmohsin7 @smian1 If one of yall can test to make sure everything looks good/make small UI changes that would be great
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Enhancements