Make sync scheduling more predictable #641
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors synchronization flow management by removing the syncStarted signal and making sync state transitions more deterministic through direct connections. The changes eliminate asynchronous timing issues in the sync lifecycle by ensuring state updates occur immediately and in the correct order.
- Removes the
syncStartedsignal and related logging/handling inFolderMan - Changes signal connections from queued to direct connections to ensure proper state ordering
- Moves
showSyncResultPopup()call to execute after state is set inslotSyncFinished
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gui/scheduling/syncscheduler.cpp | Updates connection type to combine DirectConnection with SingleShotConnection; removes extra space in log message |
| src/gui/folderman.h | Removes FolderMan class documentation and unused slot declarations |
| src/gui/folderman.cpp | Removes slotFolderSyncStarted and slotFolderSyncFinished implementations and their signal connections |
| src/gui/folder.h | Removes syncStarted signal and unused _scheduleSelfTimer member |
| src/gui/folder.cpp | Removes unused includes; changes sync initialization to avoid signal emission; updates connection to DirectConnection; changes sync invocation from queued to direct; reorders popup call; removes deferred signal emission |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _timeSinceLastSyncDone.start(); | ||
|
|
||
| setSyncState(definition.paused ? SyncResult::Paused : SyncResult::Queued); | ||
| // set the sync state without emiting a signal |
There was a problem hiding this comment.
Corrected spelling of 'emiting' to 'emitting'.
| // set the sync state without emiting a signal | |
| // set the sync state without emitting a signal |
|
|
||
| if (syncStatus != SyncResult::Undefined) { | ||
| setSyncState(syncStatus); | ||
| showSyncResultPopup(); |
There was a problem hiding this comment.
Moving showSyncResultPopup() after setSyncState() changes the order of operations. Since showSyncResultPopup() may access _syncResult and this method is now called via Qt::DirectConnection, ensure that any UI updates in showSyncResultPopup() are thread-safe, as the sync engine may be running on a different thread.
No description provided.