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

Fixes the local sync timer #1011

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Fixes the local sync timer #1011

merged 1 commit into from
Oct 9, 2024

Conversation

beastoin
Copy link
Collaborator

@beastoin beastoin commented Oct 9, 2024

Summary by Entelligence.AI

  • Refactor: Improved the behavior of the local sync timer in the Memories page. The update ensures more accurate status updates and better management of the timer during different scenarios, enhancing the user experience.
  • Style: Added a SliverToBoxAdapter with a SizedBox widget to create additional space in the UI layout for a cleaner look and feel.

@beastoin beastoin merged commit 8f60719 into main Oct 9, 2024
1 check passed
@beastoin beastoin deleted the jzecm_wal_local_sync_958 branch October 9, 2024 06:50
Copy link

github-actions bot commented Oct 9, 2024

Image description Entelligence.AI

Walkthrough

This change introduces a UI layout adjustment using SliverToBoxAdapter and SizedBox widgets, and refines the logic of local sync timer management in _LocalSyncWidgetState. The modifications aim to enhance the timer's behavior and status updates under various provider states.

Changes

Files Summary
app/lib/pages/memories/page.dart, app/lib/pages/memories/widgets/local_sync.dart Introduced a SliverToBoxAdapter with a SizedBox widget for UI spacing. Refactored the local sync timer logic in _LocalSyncWidgetState, improving the handling of status and timer based on provider and captureProvider states.

🎉 Code refined, logic aligned, 🕰️ timer's state now well-defined. 🌐 Syncing local, no more vocal, 🚀 performance is now focal. 🤖


Uplevel your code reviews with Entelligence.AI Pro

Entelligence.AI Pro

If you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Image description Entelligence.AI

Commits Files that changed from the base of the PR and between 574d2b7 and 657aecc commits.
Files selected (2)
  • app/lib/pages/memories/page.dart (1)
  • app/lib/pages/memories/widgets/local_sync.dart (1)
Files skipped from review due to trivial changes (1)
  • app/lib/pages/memories/page.dart
Review comments generated (1)
  • Review: 1
  • LGTM: 0

Tips

Chat with Image description Entelligence.AI Bot (@Entelligence.AI)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @Entelligence.AI in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

  • Add @Entelligence.AI: ignore anywhere in the PR description to pause further reviews from the bot.

Comment on lines +41 to +70
var previousStatus = _status;
if (provider.missingWalsInSeconds > 120) {
_status = LocalSyncStatus.flush;
_missSeconds = max(_missSeconds, provider.missingWalsInSeconds); // est. good for ux
} else if (!captureProvider.isWalSupported) {
_status = LocalSyncStatus.disabled;
_missSecondsInEstTimer?.cancel();
} else if ((!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) ||
provider.missingWalsInSeconds > 0) {
var previousStatus = _status;
} else if (provider.missingWalsInSeconds > 0) {
_status = LocalSyncStatus.inProgress;
} else {
_status = LocalSyncStatus.disabled;
}

// Change state to in progress
if (previousStatus != LocalSyncStatus.inProgress) {
_missSecondsInEstTimer?.cancel();
// miss seconds
if (_status == LocalSyncStatus.inProgress || _status == LocalSyncStatus.flush) {
if (previousStatus != _status) {
_missSeconds = provider.missingWalsInSeconds;
_missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
setState(() {
_missSeconds++;
});
});
}
}

// timer
if ((_status == LocalSyncStatus.inProgress || _status == LocalSyncStatus.flush) &&
(!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady)) {
_missSecondsInEstTimer ??= Timer.periodic(const Duration(seconds: 1), (t) {
setState(() {
_missSeconds++;
});
});
} else {
_missSecondsInEstTimer?.cancel();
_missSecondsInEstTimer = null;
}
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 of the timer and status logic seems to have introduced a potential issue. In the new code, _missSeconds is only updated when the status changes from another status to LocalSyncStatus.inProgress or LocalSyncStatus.flush. However, in the old code, _missSeconds was updated every time the build method was called and the status was LocalSyncStatus.inProgress or LocalSyncStatus.flush, regardless of whether the status had just changed.

This could lead to _missSeconds not being updated correctly if the build method is called multiple times without the status changing, which could happen due to various reasons such as other state changes in the widget triggering a rebuild.

Here's a suggested fix:

      if (_status == LocalSyncStatus.inProgress || _status == LocalSyncStatus.flush) {
        if (previousStatus != _status) {
           _missSeconds = provider.missingWalsInSeconds;
         }
       }

Replace with:

+ 53:       if (_status == LocalSyncStatus.inProgress || _status == LocalSyncStatus.flush) {
+ 54:           _missSeconds = provider.missingWalsInSeconds;
+ 55:        }

This will ensure that _missSeconds is always updated correctly when the status is LocalSyncStatus.inProgress or LocalSyncStatus.flush, even if the status hasn't just changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant