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

Increase the local sync's ready to flush seconds to 120 #1014

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Increase the local sync's ready to flush seconds to 120 #1014

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
2 changes: 1 addition & 1 deletion app/lib/pages/memories/sync_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class _SyncPageState extends State<SyncPage> {
),
memoryProvider.isSyncing
? Text(
'${memoryProvider.walsSyncedProgress}% synced',
'${(memoryProvider.walsSyncedProgress * 100).toInt()}% synced',
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 displaying the sync progress as a floating-point number to an integer is a good one for readability. However, it might lead to less precision in the progress display. If the syncing process is slow, users might not see the progress update for a while because the percentage will only increase when it reaches the next whole number. Consider keeping the floating-point precision up to 1 or 2 decimal places.

- '${(memoryProvider.walsSyncedProgress * 100).toInt()}% synced',
+ '${(memoryProvider.walsSyncedProgress * 100).toStringAsFixed(1)}% synced',

style: const TextStyle(color: Colors.white, fontSize: 16),
)
: const SizedBox.shrink(),
Expand Down
28 changes: 18 additions & 10 deletions app/lib/pages/memories/widgets/local_sync.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import 'package:flutter/material.dart';
import 'package:friend_private/pages/memories/page.dart';
import 'package:friend_private/pages/memories/sync_page.dart';
import 'package:friend_private/providers/capture_provider.dart';
import 'package:friend_private/providers/developer_mode_provider.dart';
import 'package:friend_private/providers/memory_provider.dart';
import 'package:friend_private/utils/other/string_utils.dart';
import 'package:friend_private/utils/other/temp.dart';
Expand All @@ -27,8 +26,22 @@ enum LocalSyncStatus {
class _LocalSyncWidgetState extends State<LocalSyncWidget> {
LocalSyncStatus? _status;
Timer? _missSecondsInEstTimer;
bool _missSecondsInEstTimerEnabled = false;
int _missSeconds = 0;

@override
void initState() {
super.initState();

_missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
if (_missSecondsInEstTimerEnabled) {
setState(() {
_missSeconds++;
});
}
});
}

Comment on lines +29 to +44
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 timer is being initialized in the initState method and it runs every second regardless of whether it's needed or not. This could lead to unnecessary CPU usage. It would be better to start the timer only when it's needed and cancel it when it's not.

-    _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
-      if (_missSecondsInEstTimerEnabled) {
-        setState(() {
-          _missSeconds++;
-        });
-      }
-    });
+    // Move timer initialization to a separate method
+    void startTimer() {
+      _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
+        if (_missSecondsInEstTimerEnabled) {
+          setState(() {
+            _missSeconds++;
+          });
+        }
+      });
+    }

@override
void dispose() {
_missSecondsInEstTimer?.cancel();
Expand All @@ -39,11 +52,11 @@ class _LocalSyncWidgetState extends State<LocalSyncWidget> {
Widget build(BuildContext context) {
return Consumer2<MemoryProvider, CaptureProvider>(builder: (context, provider, captureProvider, child) {
var previousStatus = _status;
if (provider.missingWalsInSeconds > 120) {
if (provider.missingWalsInSeconds >= 120) {
_status = LocalSyncStatus.flush;
} else if (!captureProvider.isWalSupported) {
_status = LocalSyncStatus.disabled;
} else if (provider.missingWalsInSeconds > 0) {
} else if (!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) {
Comment on lines +55 to +59
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 condition for setting _status to LocalSyncStatus.flush has been changed from provider.missingWalsInSeconds > 120 to provider.missingWalsInSeconds >= 120. This change aligns with the PR title, so it seems correct. However, please ensure that this change doesn't introduce any off-by-one errors in your logic.

_status = LocalSyncStatus.inProgress;
} else {
_status = LocalSyncStatus.disabled;
Expand All @@ -59,14 +72,9 @@ class _LocalSyncWidgetState extends State<LocalSyncWidget> {
// timer
if ((_status == LocalSyncStatus.inProgress || _status == LocalSyncStatus.flush) &&
(!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady)) {
_missSecondsInEstTimer ??= Timer.periodic(const Duration(seconds: 1), (t) {
setState(() {
_missSeconds++;
});
});
_missSecondsInEstTimerEnabled = true;
} else {
_missSecondsInEstTimer?.cancel();
_missSecondsInEstTimer = null;
_missSecondsInEstTimerEnabled = false;
Comment on lines +75 to +77
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 timer control logic has been simplified by introducing the _missSecondsInEstTimerEnabled flag. This is a good approach as it makes the code easier to read and understand. However, remember to call startTimer() and cancelTimer() methods appropriately based on the _missSecondsInEstTimerEnabled flag value.

-        _missSecondsInEstTimerEnabled = true;
+        if (!_missSecondsInEstTimerEnabled) {
+          _missSecondsInEstTimerEnabled = true;
+          startTimer();
+        }
-        _missSecondsInEstTimerEnabled = false;
+        if (_missSecondsInEstTimerEnabled) {
+          _missSecondsInEstTimerEnabled = false;
+          cancelTimer();
+        }

}

// in progress
Expand Down
Loading