refactor: Refactored performSync to facilitate understanding the code.#134
Open
goncaloMgomes wants to merge 1 commit intojpirnay:masterfrom
Open
refactor: Refactored performSync to facilitate understanding the code.#134goncaloMgomes wants to merge 1 commit intojpirnay:masterfrom
goncaloMgomes wants to merge 1 commit intojpirnay:masterfrom
Conversation
Refactored fetch remote progress.
📝 WalkthroughWalkthroughThis PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/activities/reader/KOReaderSyncActivity.cpp (3)
122-135: Inconsistent indentation / brace placement in theswitch.The case bodies and closing braces are misaligned, which makes the block harder to scan. Purely cosmetic, but worth tidying up while you're here.
♻️ Suggested formatting
- switch(syncIntent){ - case KOReaderSyncIntentState::COMPARE: { - compare(); - break; - } - case KOReaderSyncIntentState::PUSH_LOCAL: { - pushLocal(); - break; - } - case KOReaderSyncIntentState::PULL_REMOTE: { - pullRemote(); - break; - } - } + switch (syncIntent) { + case KOReaderSyncIntentState::COMPARE: + compare(); + break; + case KOReaderSyncIntentState::PUSH_LOCAL: + pushLocal(); + break; + case KOReaderSyncIntentState::PULL_REMOTE: + pullRemote(); + break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/KOReaderSyncActivity.cpp` around lines 122 - 135, The switch block on syncIntent (switch(syncIntent) over KOReaderSyncIntentState) has inconsistent indentation and brace placement; tidy it by aligning each case label with its block, place opening and closing braces consistently (e.g., case X: { ... break; } ), and ensure the closing brace of the switch is aligned with the switch keyword; adjust the cases invoking compare(), pushLocal(), and pullRemote() so their braces and breaks are consistently indented and aligned.
768-771: Deadtr(STR_NO_REMOTE_MSG)call inside the lambda.Same pattern as the other
updateUIlambdas — the translated string is discarded. Unlike the SYNCING/SYNC_FAILED cases this is not a user-visible bug, becauserender()forNO_REMOTE_PROGRESScallstr(STR_NO_REMOTE_MSG)directly rather than readingstatusMessage. Still, the line is misleading dead code; either drop it or assign tostatusMessagefor consistency with the rest of the helpers.♻️ Cleanup
updateUI(NO_REMOTE_PROGRESS,[this]() { - tr(STR_NO_REMOTE_MSG); hasRemoteProgress = false; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/KOReaderSyncActivity.cpp` around lines 768 - 771, The lambda passed to updateUI for NO_REMOTE_PROGRESS contains a dead call tr(STR_NO_REMOTE_MSG); remove that unused tr(...) or instead assign its result to statusMessage to match other updateUI handlers; specifically edit the updateUI(NO_REMOTE_PROGRESS, [this]() { ... }) lambda to either drop tr(STR_NO_REMOTE_MSG) and only set hasRemoteProgress = false, or set statusMessage = tr(STR_NO_REMOTE_MSG) and then set hasRemoteProgress = false so behavior is consistent with render() and other sync states.
603-696: Optional: extract the shared "fetch + prepare + map" prefix fromcompare()andpullRemote().The first ~10 lines of
compare()andpullRemote()are nearly identical (status update,beginPersistentSession,fetchRemoteProgress,prepareRemoteMapping,ensureRemotePositionMapped+ failure branch). A small helper (e.g.bool fetchAndMapRemote(bool closeSessionBeforeMapping)) would eliminate the duplication and make the intent-specific tails clearer. Defer if you'd rather keep the diff minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/KOReaderSyncActivity.cpp` around lines 603 - 696, The compare() and pullRemote() functions duplicate the same "status update + begin session + fetchRemoteProgress + prepareRemoteMapping + ensureRemotePositionMapped" sequence; extract that into a helper like KOReaderSyncActivity::fetchAndPrepareRemote(bool requireMapping) that calls updateUI(..., tr(STR_FETCH_PROGRESS)), KOReaderSyncClient::beginPersistentSession(), fetchRemoteProgress(), prepareRemoteMapping(), then runs ensureRemotePositionMapped(requireMapping) and returns false on failure. Replace the duplicated blocks in compare() and pullRemote() with calls to this helper (pass false from compare() and true/default from pullRemote()), and preserve the existing failure branch behavior (calling updateUI(SYNC_FAILED, lambda setting statusMessage) and returning) where the helper returns false.src/activities/reader/KOReaderSyncActivity.h (1)
118-118: Abbreviated function template definition in .cpp limits portability.Declaring
updateUIwith anauto&¶meter makes it an abbreviated function template (C++20). While the current code works because every call site is in the same translation unit (KOReaderSyncActivity.cpp), this pattern creates a maintainability risk: if a future change adds a caller from another TU, the linker will fail because the definition is not visible. Standard C++ practice keeps function template definitions in headers (inline in the class or outside it) to ensure the compiler can instantiate the template at every call site. Either move the definition into the header or replaceauto&&with a concrete callable type (e.g.,std::function<void()>or a bespoke delegate class) to keep the implementation in the .cpp file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/KOReaderSyncActivity.h` at line 118, The declaration of updateUI(State newState, auto&& runInCriticalSection, bool wait = false) creates an abbreviated function template which must be defined in the header for cross-translation-unit calls; to fix this, either move the template definition into the header (make updateUI a template inline in KOReaderSyncActivity.h) so the compiler can instantiate it at call sites, or change the signature to use a concrete callable type (for example replace auto&& runInCriticalSection with std::function<void()> or a custom delegate type) so you can keep the implementation in KOReaderSyncActivity.cpp; update callers to match the new callable type if you choose the concrete option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/activities/reader/KOReaderSyncActivity.cpp`:
- Around line 110-116: The lambda calls passed to updateUI are discarding the
translated strings (tr(...)) instead of assigning them to statusMessage, so
statusMessage remains stale; update the lambdas used in updateUI at the points
around computeLocalProgressAndChapter (and the other occurrences you noted near
lines 605 and 661) to assign the translation to statusMessage (e.g.,
statusMessage = tr(STR_MAPPING_LOCAL) and statusMessage =
tr(STR_SYNC_FAILED_MSG)) before returning, ensuring updateUI receives a callable
that sets statusMessage rather than throwing away the tr(...) result; keep the
existing updateUI, tr, statusMessage, computeLocalProgressAndChapter and
SYNC_FAILED symbols untouched otherwise.
- Around line 603-661: The lambdas passed to updateUI in compare() and
pullRemote() call tr(STR_FETCH_PROGRESS) but discard the result instead of
assigning it to statusMessage, so the UI keeps the old message; update both
lambdas (in KOReaderSyncActivity::compare and KOReaderSyncActivity::pullRemote)
to set statusMessage = tr(STR_FETCH_PROGRESS) when invoking updateUI(state, ...)
so the "Fetching progress" text is actually displayed (mirror the fix already
applied in performSync()).
---
Nitpick comments:
In `@src/activities/reader/KOReaderSyncActivity.cpp`:
- Around line 122-135: The switch block on syncIntent (switch(syncIntent) over
KOReaderSyncIntentState) has inconsistent indentation and brace placement; tidy
it by aligning each case label with its block, place opening and closing braces
consistently (e.g., case X: { ... break; } ), and ensure the closing brace of
the switch is aligned with the switch keyword; adjust the cases invoking
compare(), pushLocal(), and pullRemote() so their braces and breaks are
consistently indented and aligned.
- Around line 768-771: The lambda passed to updateUI for NO_REMOTE_PROGRESS
contains a dead call tr(STR_NO_REMOTE_MSG); remove that unused tr(...) or
instead assign its result to statusMessage to match other updateUI handlers;
specifically edit the updateUI(NO_REMOTE_PROGRESS, [this]() { ... }) lambda to
either drop tr(STR_NO_REMOTE_MSG) and only set hasRemoteProgress = false, or set
statusMessage = tr(STR_NO_REMOTE_MSG) and then set hasRemoteProgress = false so
behavior is consistent with render() and other sync states.
- Around line 603-696: The compare() and pullRemote() functions duplicate the
same "status update + begin session + fetchRemoteProgress + prepareRemoteMapping
+ ensureRemotePositionMapped" sequence; extract that into a helper like
KOReaderSyncActivity::fetchAndPrepareRemote(bool requireMapping) that calls
updateUI(..., tr(STR_FETCH_PROGRESS)),
KOReaderSyncClient::beginPersistentSession(), fetchRemoteProgress(),
prepareRemoteMapping(), then runs ensureRemotePositionMapped(requireMapping) and
returns false on failure. Replace the duplicated blocks in compare() and
pullRemote() with calls to this helper (pass false from compare() and
true/default from pullRemote()), and preserve the existing failure branch
behavior (calling updateUI(SYNC_FAILED, lambda setting statusMessage) and
returning) where the helper returns false.
In `@src/activities/reader/KOReaderSyncActivity.h`:
- Line 118: The declaration of updateUI(State newState, auto&&
runInCriticalSection, bool wait = false) creates an abbreviated function
template which must be defined in the header for cross-translation-unit calls;
to fix this, either move the template definition into the header (make updateUI
a template inline in KOReaderSyncActivity.h) so the compiler can instantiate it
at call sites, or change the signature to use a concrete callable type (for
example replace auto&& runInCriticalSection with std::function<void()> or a
custom delegate type) so you can keep the implementation in
KOReaderSyncActivity.cpp; update callers to match the new callable type if you
choose the concrete option.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26620a0b-ab06-49c4-8e07-74775a2763e9
📒 Files selected for processing (2)
src/activities/reader/KOReaderSyncActivity.cppsrc/activities/reader/KOReaderSyncActivity.h
Comment on lines
+110
to
116
|
|
||
| updateUI(state, []() {tr(STR_MAPPING_LOCAL);}, true); | ||
|
|
||
| if (!computeLocalProgressAndChapter()) { | ||
| { | ||
| RenderLock lock(*this); | ||
| state = SYNC_FAILED; | ||
| statusMessage = tr(STR_SYNC_FAILED_MSG); | ||
| } | ||
| requestUpdate(true); | ||
| updateUI(SYNC_FAILED,[]() {tr(STR_SYNC_FAILED_MSG);}); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: statusMessage is never assigned — the tr(...) return value is discarded.
Both lambdas here call tr(STR_MAPPING_LOCAL) / tr(STR_SYNC_FAILED_MSG) and throw the result away. Pre-refactor, the equivalent block did statusMessage = tr(...). As a result:
- Line 111: while mapping local progress, the popup keeps showing the previous "Calc hash" message (or whatever was last assigned) instead of "Mapping local".
- Line 114: on
SYNC_FAILED,render()drawsstatusMessageas the wrapped detail line, so users see a stale or empty message instead of the failure text.
This is a behavioral regression despite the PR description stating no logic changed.
🐛 Proposed fix
- updateUI(state, []() {tr(STR_MAPPING_LOCAL);}, true);
+ updateUI(state, [this]() { statusMessage = tr(STR_MAPPING_LOCAL); }, true);
if (!computeLocalProgressAndChapter()) {
- updateUI(SYNC_FAILED,[]() {tr(STR_SYNC_FAILED_MSG);});
+ updateUI(SYNC_FAILED, [this]() { statusMessage = tr(STR_SYNC_FAILED_MSG); });
return;
}The same bug exists at lines 605 and 661 — see comment there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/activities/reader/KOReaderSyncActivity.cpp` around lines 110 - 116, The
lambda calls passed to updateUI are discarding the translated strings (tr(...))
instead of assigning them to statusMessage, so statusMessage remains stale;
update the lambdas used in updateUI at the points around
computeLocalProgressAndChapter (and the other occurrences you noted near lines
605 and 661) to assign the translation to statusMessage (e.g., statusMessage =
tr(STR_MAPPING_LOCAL) and statusMessage = tr(STR_SYNC_FAILED_MSG)) before
returning, ensuring updateUI receives a callable that sets statusMessage rather
than throwing away the tr(...) result; keep the existing updateUI, tr,
statusMessage, computeLocalProgressAndChapter and SYNC_FAILED symbols untouched
otherwise.
Comment on lines
+603
to
+661
| void KOReaderSyncActivity::compare() { | ||
|
|
||
| updateUI(state, []() {tr(STR_FETCH_PROGRESS);}); | ||
|
|
||
| // Keep the GET connection alive so Upload can reuse the same session and | ||
| // avoid a second TLS handshake under fragmented heap. | ||
| KOReaderSyncClient::beginPersistentSession(); | ||
|
|
||
| if (!fetchRemoteProgress()) return; | ||
|
|
||
| // Prepare remote mapping state for the next step. | ||
| prepareRemoteMapping(); | ||
|
|
||
| // Compare intent keeps the legacy chooser flow (apply vs upload), which is | ||
| // still useful for manual conflict decisions. | ||
| // Pre-map remote progress now so compare UI always shows concrete chapter/ | ||
| // page data. The mapped result is cached and reused if Apply is chosen. | ||
| if (!ensureRemotePositionMapped(false)) { | ||
| updateUI(SYNC_FAILED,[this]() { | ||
| statusMessage = tr(STR_SYNC_FAILED_MSG); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Local progress was precomputed before network; keep using the cached value. | ||
| releaseEpubForMapping(); | ||
|
|
||
| { | ||
| RenderLock lock(*this); | ||
| state = SHOWING_RESULT; | ||
|
|
||
| // Default to the option that corresponds to the furthest progress. | ||
| // Compare in the shared CrossPoint coordinate system (spine → page → paragraph) | ||
| // rather than percentage, since percentages are derived differently on each | ||
| // side and lose resolution. Remote has already been mapped via | ||
| // ensureRemotePositionMapped() at this point. | ||
| auto isLocalAhead = [&]() { | ||
| if (remotePosition.spineIndex < 0) { | ||
| return localProgress.percentage > remoteProgress.percentage; // mapping unavailable; fall back | ||
| } | ||
| if (currentSpineIndex != remotePosition.spineIndex) { | ||
| return currentSpineIndex > remotePosition.spineIndex; | ||
| } | ||
| if (currentPage != remotePosition.pageNumber) { | ||
| return currentPage > remotePosition.pageNumber; | ||
| } | ||
| if (hasLocalParagraphIndex && remotePosition.hasParagraphIndex) { | ||
| return localParagraphIndex > remotePosition.paragraphIndex; | ||
| } | ||
| return false; | ||
| }; | ||
| selectedOption = isLocalAhead() ? 1 /* Upload local */ : 0 /* Apply remote */; | ||
| } | ||
| requestUpdate(true); | ||
| } | ||
|
|
||
| void KOReaderSyncActivity::pullRemote() { | ||
|
|
||
| updateUI(state, []() {tr(STR_FETCH_PROGRESS);}); |
There was a problem hiding this comment.
Same tr(...) discard bug in compare() and pullRemote().
Lines 605 and 661 reproduce the bug from performSync(): the lambda calls tr(STR_FETCH_PROGRESS) but never writes to statusMessage. While state is SYNCING, render() draws statusMessage in the popup, so users won't see "Fetching progress" — they'll see the previously set message ("Calc hash" / "Mapping local").
🐛 Proposed fix
- updateUI(state, []() {tr(STR_FETCH_PROGRESS);});
+ updateUI(state, [this]() { statusMessage = tr(STR_FETCH_PROGRESS); });Apply the same change at line 661 in pullRemote().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/activities/reader/KOReaderSyncActivity.cpp` around lines 603 - 661, The
lambdas passed to updateUI in compare() and pullRemote() call
tr(STR_FETCH_PROGRESS) but discard the result instead of assigning it to
statusMessage, so the UI keeps the old message; update both lambdas (in
KOReaderSyncActivity::compare and KOReaderSyncActivity::pullRemote) to set
statusMessage = tr(STR_FETCH_PROGRESS) when invoking updateUI(state, ...) so the
"Fetching progress" text is actually displayed (mirror the fix already applied
in performSync()).
goncaloMgomes
changed the title
jpirnay
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I don't know if these changes are usefull for your repo. I made them in order to better undestand the perfomSync flow and ended up with (in my opinion) is a little better to understand code. No logic was changed, just created some functions specific for each intent as well as other helper functions.
Additional Context
specific areas to focus on).
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? NO
Summary by CodeRabbit