feat: modernize launch splash#7210
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcf97a2ad4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (phase->resetsBar) { | ||
| // Long independent phases get their own full bar | ||
| displayProgress = 0.0; | ||
| phaseStart = phase->start; |
There was a problem hiding this comment.
In showMessage, every message that matches a resetsBar phase unconditionally sets displayProgress = 0.0. During wallet rescans, ShowProgress emits repeated "Rescanning…" updates with increasing percentages (src/wallet/wallet.cpp emits this at startup and then periodically), so each update re-enters this block and forces the bar back to zero before it can advance, producing visible backward jumps and inaccurate progress reporting for long rescans. The reset should happen only when entering the phase, not on every progress callback.
Useful? React with 👍 / 👎.
WalkthroughIntroduces a phase-driven, time- and progress-aware SplashScreen. Adds PhaseInfo, a phase lookup mapping init messages to phase ranges and flags (resetsBar, snapsToEnd, long phases). Adds thread-safe PostMessageAndProgress wiring to marshal messages/progress to the GUI thread, public setProgress(int) and private calcOverallProgress() APIs, an animation timer that interpolates displayProgress toward targets, and per-phase timing/state fields. Reworks painting to a rounded card layout with logo, title/version, optional network badge, and an integrated bottom progress bar; showMessage now triggers phase lookup and starts animation updates. Sequence Diagram(s)sequenceDiagram
participant Core
participant GUIThread as "GUI Thread / Event Queue"
participant Splash as "SplashScreen"
participant Anim as "Anim Timer"
participant Painter as "Paint / QWidget"
Core->>GUIThread: PostMessageAndProgress(msg, progress, color)
GUIThread->>Splash: showMessage(msg, color)
GUIThread->>Splash: setProgress(subProgress)
Splash->>Splash: LookupPhase(msg) -> set phaseStart/phaseEnd/flags
Splash->>Anim: ensure animTimer running
Anim->>Splash: timeout tick
Splash->>Splash: calcOverallProgress() -> update displayProgress (interpolate)
Splash->>Painter: schedule repaint()
Painter->>Splash: paintEvent -> draw card, logo, texts, progress bar (displayProgress)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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: 1
🧹 Nitpick comments (1)
src/qt/splashscreen.cpp (1)
386-389: Assert the documented-1or0..100contract insetProgress().
calcOverallProgress()assumes that range when it scales the current phase. Guarding it here makes bad signal values fail fast instead of pushing the bar outside its phase window.Based on learnings, use fail-fast asserts in setters to enforce invariants and catch programmer errors early during development.Suggested fix
void SplashScreen::setProgress(int value) { + assert(value == -1 || (value >= 0 && value <= 100)); curProgress = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/splashscreen.cpp` around lines 386 - 389, Add a fail-fast assertion in SplashScreen::setProgress to enforce the documented contract (value == -1 or 0..100) before assigning curProgress; specifically, in the SplashScreen::setProgress(int value) method assert that value == -1 || (value >= 0 && value <= 100) so invalid signal values are caught early (calcOverallProgress relies on this invariant).
🤖 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/qt/splashscreen.cpp`:
- Around line 357-381: The code currently reinitializes phase state every time
showMessage() is called (via PostMessageAndProgress()), causing timers and
progress bases to restart for repeated identical messages; modify the logic to
only reinitialize when the phase actually changes by comparing the
LookupPhase(message) result to a stored previous-phase pointer (e.g. add a
member like lastPhase or lastPhaseMessage and update it after changes). Only run
the block that sets displayProgress, phaseStart, phaseEnd, phaseIsLong,
animTimer.start/stop and phaseTimer.restart when the new phase pointer differs
from lastPhase; when LookupPhase(message) returns nullptr (message-only phase)
explicitly reset curProgress to 0 so exponential-progress handling isn’t
skipped, and update lastPhase appropriately. Ensure lastPhase is updated
whenever you perform a real transition.
---
Nitpick comments:
In `@src/qt/splashscreen.cpp`:
- Around line 386-389: Add a fail-fast assertion in SplashScreen::setProgress to
enforce the documented contract (value == -1 or 0..100) before assigning
curProgress; specifically, in the SplashScreen::setProgress(int value) method
assert that value == -1 || (value >= 0 && value <= 100) so invalid signal values
are caught early (calcOverallProgress relies on this invariant).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43db32d3-f40c-47a0-b25d-c242a5afbff3
📒 Files selected for processing (2)
src/qt/splashscreen.cppsrc/qt/splashscreen.h
…yout Redesign the splash screen with a modern card-style appearance featuring rounded corners, a translucent background with OS compositor support, and a refined visual layout with properly scoped QPainter lifetime.
Add a progress bar that tracks initialization phases with weighted time estimates. Includes smooth easing transitions, phase-aware progress for long operations like wallet loading and rescan, and snap-to-100% on completion. - Phase lookup uses _() to translate keys at runtime for locale-safe matching - messageColor is const, captured by value in lambdas for thread safety - Animation timer deferred until first message to avoid idle repaints - Time-based exponential fill capped at 95% (EXPONENTIAL_FILL_CAP)
PastaPastaPasta
force-pushed
the
feat/improve-ui
branch
from
fcf97a2 to
09fc59c
Compare
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98333549c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/qt/splashscreen.cpp
Outdated
| if (phase != nullptr && phase != m_current_phase) { | ||
| m_current_phase = phase; |
There was a problem hiding this comment.
showMessage only reinitializes a phase when phase != m_current_phase, so resetsBar phases are not reset when the same phase text reappears later. Wallet rescans emit titles prefixed with each wallet name (src/wallet/wallet.cpp uses "<wallet> Rescanning…"), so rescanning wallet #2 still maps to the same PhaseInfo and skips this block; because the animation never moves backward unless displayProgress is explicitly zeroed, the bar remains near completion and reports incorrect progress for subsequent wallets during multi-wallet startup.
Useful? React with 👍 / 👎.
During wallet rescans, repeated ShowProgress callbacks with the same message re-entered the resetsBar block, slamming displayProgress back to 0 and restarting phaseTimer on every update. Track the current phase pointer and skip reinitialization when the phase hasn't changed.
When multiple wallets rescan at startup, each emits ShowProgress with a wallet-name-prefixed message (e.g. '[wallet2] Rescanning…'). Since these map to the same PhaseInfo entry, the phase \!= m_current_phase guard skipped reinitialization for subsequent wallets. Track the triggering message text and also reinitialize resetsBar phases when the message changes.
PastaPastaPasta
force-pushed
the
feat/improve-ui
branch
from
9833354 to
4c97eb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/qt/splashscreen.cpp (1)
394-397: Assert thesetProgress()invariant at the boundary.This slot is the single ingress for the documented
-1/0..100contract. Failing fast here is better than lettingcalcOverallProgress()accept impossible values and render misleading widths.Based on learnings, use fail-fast asserts in setters to enforce invariants.🛡️ Suggested change
void SplashScreen::setProgress(int value) { + assert(value == -1 || (value >= 0 && value <= 100)); curProgress = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/splashscreen.cpp` around lines 394 - 397, Add a fail-fast assertion to SplashScreen::setProgress to enforce the documented contract that the incoming value must be either -1 or within 0..100; validate the parameter at the boundary (in SplashScreen::setProgress) and assert (or otherwise abort in debug builds) when value is outside these allowed ranges before assigning curProgress so calcOverallProgress() never receives impossible values.src/qt/splashscreen.h (1)
71-80: Prefer the usual=initializer style for the new primitive members.These POD defaults stand out from the surrounding Dash/Bitcoin Core header style, which generally uses simple
=initializers for primitive members.Based on learnings, in header files prefer in-class initialization for POD types and use simple defaults for primitive members.♻️ Suggested change
- int curProgress{-1}; + int curProgress = -1; // Phase-based progress tracking - qreal phaseStart{0.0}; // Overall progress at start of current phase - qreal phaseEnd{0.0}; // Overall progress at end of current phase - bool phaseIsLong{false}; // True for long independent phases (rescan, wallet load) + qreal phaseStart = 0.0; // Overall progress at start of current phase + qreal phaseEnd = 0.0; // Overall progress at end of current phase + bool phaseIsLong = false; // True for long independent phases (rescan, wallet load) QElapsedTimer phaseTimer; // Time since current phase started - const struct PhaseInfo* m_current_phase{nullptr}; // Current phase (defined in splashscreen.cpp) + const struct PhaseInfo* m_current_phase = nullptr; // Current phase (defined in splashscreen.cpp) QString m_current_phase_message; // Message that triggered current phase - qreal displayProgress{0.0}; // Smoothly animated display value (0.0-1.0) + qreal displayProgress = 0.0; // Smoothly animated display value (0.0-1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/splashscreen.h` around lines 71 - 80, The header uses brace initializers for primitive/POD members; change them to the usual equals style: replace the brace initializers for curProgress, phaseStart, phaseEnd, phaseIsLong, displayProgress and m_current_phase (and any other primitive/POD members in the same block) to use = -1, = 0.0, = 0.0, = false, = 0.0 and = nullptr respectively; leave complex types like QElapsedTimer and QString as-is if desired. This keeps the class consistent with surrounding Dash/Bitcoin Core header style and makes the defaults easier to read.
🤖 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/qt/splashscreen.cpp`:
- Around line 217-230: The timeout lambda attached to animTimer never stops
until 100%, causing needless repaints when the target stalls; modify the lambda
(the QTimer::timeout connection using calcOverallProgress(), displayProgress,
and target) to stop animTimer once displayProgress reaches the current target
(use the same snap epsilon, e.g. when target - displayProgress < 0.002), i.e.
set displayProgress = target and call animTimer.stop() when close to target so
the timer halts even if overall progress < 0.999; keep update() behavior so the
final frame is painted before stopping.
---
Nitpick comments:
In `@src/qt/splashscreen.cpp`:
- Around line 394-397: Add a fail-fast assertion to SplashScreen::setProgress to
enforce the documented contract that the incoming value must be either -1 or
within 0..100; validate the parameter at the boundary (in
SplashScreen::setProgress) and assert (or otherwise abort in debug builds) when
value is outside these allowed ranges before assigning curProgress so
calcOverallProgress() never receives impossible values.
In `@src/qt/splashscreen.h`:
- Around line 71-80: The header uses brace initializers for primitive/POD
members; change them to the usual equals style: replace the brace initializers
for curProgress, phaseStart, phaseEnd, phaseIsLong, displayProgress and
m_current_phase (and any other primitive/POD members in the same block) to use =
-1, = 0.0, = 0.0, = false, = 0.0 and = nullptr respectively; leave complex types
like QElapsedTimer and QString as-is if desired. This keeps the class consistent
with surrounding Dash/Bitcoin Core header style and makes the defaults easier to
read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37802fed-e0b6-4484-accc-6eace07d8130
📒 Files selected for processing (2)
src/qt/splashscreen.cppsrc/qt/splashscreen.h
| connect(&animTimer, &QTimer::timeout, this, [this]() { | ||
| qreal target = calcOverallProgress(); | ||
| // Smoothly animate toward target (never go backwards) | ||
| if (target > displayProgress) { | ||
| qreal gap = target - displayProgress; | ||
| // Faster easing for larger gaps so fast phases don't lag behind | ||
| qreal ease = gap > 0.1 ? 0.3 : 0.15; | ||
| displayProgress += gap * ease; | ||
| // Snap if very close | ||
| if (target - displayProgress < 0.002) displayProgress = target; | ||
| } | ||
| update(); | ||
| // Stop timer once progress is complete | ||
| if (displayProgress >= 0.999) animTimer.stop(); |
There was a problem hiding this comment.
Stop the animation timer once the current target is reached.
This lambda keeps calling update() every 30 ms and only stops at 100%. In a progress-backed phase—or if the first message does not resolve to a PHASE_TABLE entry so the target stays flat—the splash will repaint indefinitely with no visual change.
💡 Suggested change
connect(&animTimer, &QTimer::timeout, this, [this]() {
- qreal target = calcOverallProgress();
+ const qreal target = calcOverallProgress();
+ const bool time_driven = curProgress < 0 && phaseEnd > phaseStart;
// Smoothly animate toward target (never go backwards)
if (target > displayProgress) {
qreal gap = target - displayProgress;
// Faster easing for larger gaps so fast phases don't lag behind
qreal ease = gap > 0.1 ? 0.3 : 0.15;
displayProgress += gap * ease;
// Snap if very close
if (target - displayProgress < 0.002) displayProgress = target;
}
- update();
- // Stop timer once progress is complete
- if (displayProgress >= 0.999) animTimer.stop();
+ if (target > displayProgress || time_driven) {
+ update();
+ } else {
+ animTimer.stop();
+ }
+ if (displayProgress >= 0.999) animTimer.stop();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/splashscreen.cpp` around lines 217 - 230, The timeout lambda attached
to animTimer never stops until 100%, causing needless repaints when the target
stalls; modify the lambda (the QTimer::timeout connection using
calcOverallProgress(), displayProgress, and target) to stop animTimer once
displayProgress reaches the current target (use the same snap epsilon, e.g. when
target - displayProgress < 0.002), i.e. set displayProgress = target and call
animTimer.stop() when close to target so the timer halts even if overall
progress < 0.999; keep update() behavior so the final frame is painted before
stopping.
2 participants
Issue being fixed or feature implemented
Old
New
Screen.Recording.2026-03-11.at.23.21.01.mov
What was done?
Commit 1: Visual modernization
QPainterlifetime to release the pixmap before timer/signal setupCommit 2: Phase-based progress bar
ShowProgresscallbacks (block verification, replaying, rescanning) interpolate linearly within their range_()at runtime so matching works in all localesmessageColorisconstand captured by value in signal lambdas for thread-safe cross-thread callbacksHow Has This Been Tested?
--rescanflag (bar resets and uses slow curve)Breaking Changes
None. Visual-only changes to the splash screen with no impact on consensus, RPC, or wallet behavior.
Checklist: