fix: fix corner radius and memory leak issues #607
Merged
zccrs merged 1 commit intolinuxdeepin:develop/splash-screenfrom Oct 29, 2025
Merged
fix: fix corner radius and memory leak issues #607zccrs merged 1 commit intolinuxdeepin:develop/splash-screenfrom
zccrs merged 1 commit intolinuxdeepin:develop/splash-screenfrom
Conversation
wineee
changed the title
Reviewer's GuideAdds a prelaunch splash screen by integrating a QML PrelaunchSplash component and wiring its lifecycle in SurfaceWrapper and QmlEngine, updates decoration behavior to account for splash states, and includes a minor style fix in the shell handler’s container logic. Sequence diagram for splash screen lifecycle managementsequenceDiagram
participant User
participant SurfaceWrapper
participant QmlEngine
participant PrelaunchSplash
User->>SurfaceWrapper: Launch application
SurfaceWrapper->>QmlEngine: createPrelaunchSplash(parent, logoPath, initialRadius)
QmlEngine->>PrelaunchSplash: instantiate with properties
PrelaunchSplash->>SurfaceWrapper: destroyRequested (signal)
SurfaceWrapper->>PrelaunchSplash: deleteLater()
SurfaceWrapper->>SurfaceWrapper: prelaunchSplashChanged() (signal)
SurfaceWrapper->>User: Main surface ready
Updated class diagram for SurfaceWrapper and QmlEngine splash screen integrationclassDiagram
class SurfaceWrapper {
+Type type
+qreal implicitWidth
+qreal implicitHeight
+WSurface* surface()
+WToplevelSurface* shellSurface()
+WSurfaceItem* surfaceItem()
+QQuickItem* prelaunchSplash()
+void onPrelaunchSplashDestroyRequested()
+void surfaceItemChanged()
+void prelaunchSplashChanged()
+Q_SIGNAL surfaceItemChanged
+Q_SIGNAL prelaunchSplashChanged
}
class QmlEngine {
+QQuickItem* createPrelaunchSplash(QQuickItem* parent, const QString& logoPath, qreal initialRadius)
}
SurfaceWrapper --> QQuickItem : prelaunchSplash
SurfaceWrapper --> QmlEngine : uses
QmlEngine --> QQuickItem : createPrelaunchSplash
Updated class diagram for PrelaunchSplash QML componentclassDiagram
class PrelaunchSplash {
+string logoPath
+bool destroyAfterFade
+real initialRadius
+signal destroyRequested
}
PrelaunchSplash --> Rectangle : contains
PrelaunchSplash --> Image : contains
Updated class diagram for Decoration QML componentclassDiagram
class Decoration {
+SurfaceWrapper surface
+visible
+x
+y
+width
+height
}
Decoration --> Border : contains
Border --> SurfaceItem : parent (if available)
Border --> PrelaunchSplash : parent (fallback if SurfaceItem is null)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Resolve the leftover merge conflict markers and duplicated splash initialization in the SurfaceWrapper constructor to avoid creating multiple splash instances.
- Ensure you emit prelaunchSplashChanged right after creating the splash item so QML bindings react to the new splash property.
- Update all createPrelaunchSplash calls to match the new signature (logoPath and initialRadius) for consistent and unambiguous initialization.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Resolve the leftover merge conflict markers and duplicated splash initialization in the SurfaceWrapper constructor to avoid creating multiple splash instances.
- Ensure you emit prelaunchSplashChanged right after creating the splash item so QML bindings react to the new splash property.
- Update all createPrelaunchSplash calls to match the new signature (logoPath and initialRadius) for consistent and unambiguous initialization.
## Individual Comments
### Comment 1
<location> `src/surface/surfacewrapper.cpp:98-100` </location>
<code_context>
} else {
setImplicitSize(800, 600);
}
+ m_prelaunchSplash = m_engine->createPrelaunchSplash(this, QString(), radius());
+ // Connect to QML signal so C++ can destroy the QML item when requested
+ connect(m_prelaunchSplash, SIGNAL(destroyRequested()), this, SLOT(onPrelaunchSplashDestroyRequested()));
</code_context>
<issue_to_address>
**suggestion:** Consider handling the case where radius() may be undefined or invalid.
Validate radius() before passing it to createPrelaunchSplash to prevent unintended UI issues if the value is undefined or zero.
```suggestion
// Validate radius before passing to createPrelaunchSplash
qreal splashRadius = radius();
if (splashRadius <= 0 || std::isnan(splashRadius)) {
splashRadius = 0; // fallback to 0 or another sensible default
}
m_prelaunchSplash = m_engine->createPrelaunchSplash(this, QString(), splashRadius);
// Connect to QML signal so C++ can destroy the QML item when requested
connect(m_prelaunchSplash, SIGNAL(destroyRequested()), this, SLOT(onPrelaunchSplashDestroyRequested()));
```
</issue_to_address>
### Comment 2
<location> `src/surface/surfacewrapper.cpp:234` </location>
<code_context>
connect(m_surfaceItem, &WSurfaceItem::implicitHeightChanged, this, [this] {
setImplicitHeight(m_surfaceItem->implicitHeight());
});
+ Q_EMIT surfaceItemChanged();
if (!m_prelaunchSplash || !m_prelaunchSplash->isVisible())
</code_context>
<issue_to_address>
**suggestion (performance):** Emitting surfaceItemChanged unconditionally may cause unnecessary signal emissions.
Emit surfaceItemChanged only when m_surfaceItem changes to avoid redundant QML updates.
Suggested implementation:
```cpp
connect(m_surfaceItem, &WSurfaceItem::implicitHeightChanged, this, [this] {
setImplicitHeight(m_surfaceItem->implicitHeight());
});
// Only emit surfaceItemChanged if m_surfaceItem has changed
static WSurfaceItem* s_lastSurfaceItem = nullptr;
if (m_surfaceItem != s_lastSurfaceItem) {
Q_EMIT surfaceItemChanged();
s_lastSurfaceItem = m_surfaceItem;
}
if (!m_prelaunchSplash || !m_prelaunchSplash->isVisible())
setImplicitSize(m_surfaceItem->implicitWidth(), m_surfaceItem->implicitHeight());
```
If `m_surfaceItem` can change elsewhere in the class (e.g., via a setter or other assignment), you should move the conditional signal emission to those locations instead, and remove the static variable workaround. The above change works if this is the only place `m_surfaceItem` is set, but for best practice, emit the signal in the setter or wherever `m_surfaceItem` is assigned a new value.
</issue_to_address>
### Comment 3
<location> `src/core/qml/PrelaunchSplash.qml:25` </location>
<code_context>
color: "#ffffff"
- // Fill parent; size is dictated by parent
anchors.fill: parent
+ radius: initialRadius
// Centered logo
</code_context>
<issue_to_address>
**issue (bug_risk):** Setting radius to undefined may cause rendering issues.
If initialRadius is undefined, the Rectangle may not render correctly. Please set a default value for radius.
</issue_to_address>
### Comment 4
<location> `src/surface/surfacewrapper.cpp:102` </location>
<code_context>
+ // Connect to QML signal so C++ can destroy the QML item when requested
+ connect(m_prelaunchSplash, SIGNAL(destroyRequested()), this, SLOT(onPrelaunchSplashDestroyRequested()));
setNoDecoration(false);
+<<<<<<< HEAD
m_prelaunchSplash = m_engine->createPrelaunchSplash(this);
</code_context>
<issue_to_address>
**issue (complexity):** Consider resolving merge conflicts, consolidating splash setup into a helper, and centralizing signal emissions in setters for clarity and maintainability.
```markdown
You’ve introduced an unresolved merge conflict, duplicated calls to create the splash, ad-hoc cleanup, and scattered signal emissions. You can drastically simplify this by:
1. **Resolve the merge conflict & consolidate splash setup into one helper.**
2. **Parent the QML item (so Qt does cleanup) and drop `onPrelaunchSplashDestroyRequested`.**
3. **Emit your change signals in the setters, not sprinkled through `setup()`.**
For example:
```cpp
// In SurfaceWrapper.h
private:
QQuickItem *m_prelaunchSplash = nullptr;
void initPrelaunchSplash();
public:
QQuickItem *prelaunchSplash() const { return m_prelaunchSplash; }
Q_SIGNALS:
void prelaunchSplashChanged();
```
```cpp
// In constructor:
initPrelaunchSplash();
```
```cpp
// New helper in SurfaceWrapper.cpp
void SurfaceWrapper::initPrelaunchSplash()
{
// single, conflict-free creation
m_prelaunchSplash = m_engine->createPrelaunchSplash(this, QString(), radius());
m_prelaunchSplash->setParentItem(this); // auto-cleanup
m_prelaunchSplash->setZ(9999999999LL);
// connect visibility-change in one place
connect(m_prelaunchSplash, &QQuickItem::visibleChanged,
this, &SurfaceWrapper::onSplashVisibilityChanged);
Q_EMIT prelaunchSplashChanged();
}
```
```cpp
// Remove onPrelaunchSplashDestroyRequested entirely.
// Instead handle hide in QML and let the parent-child relationship handle deletion.
```
```cpp
// Move scattered emits into setters:
void SurfaceWrapper::setSurfaceItem(WSurfaceItem *item) {
if (m_surfaceItem == item) return;
m_surfaceItem = item;
Q_EMIT surfaceItemChanged();
// … other init …
}
```
This approach:
- Gets rid of conflict markers
- Ensures exactly one `createPrelaunchSplash` call
- Relies on Qt’s parent-child cleanup
- Centralizes signal emits in setters
- Keeps all behavior intact.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
wineee
force-pushed
the
radius
branch
2 times, most recently
from
5f94267 to
af184ae
Compare
wineee
force-pushed
the
radius
branch
3 times, most recently
from
b1baafc to
3de9f46
Compare
1. Fixed corner radius inheritance by passing initial radius parameter to prelaunch splash 2. Resolved memory leak by replacing direct destroy() call with signal- based destruction request 3. Improved surface item handling by adding proper change notifications 4. Enhanced type checking for mouse area to exclude undetermined surface types Log: Fixed window corner radius display and memory leak during splash screen cleanup Influence: 1. Test window creation with different corner radius values 2. Verify prelaunch splash screen properly disappears without memory leaks 3. Check mouse interaction works correctly on all surface types 4. Validate window decoration borders follow proper radius inheritance 5. Test surface type transitions from undetermined to normal state fix: 修复圆角和内存泄漏问题 1. 通过向预启动闪屏传递初始半径参数修复圆角继承问题 2. 使用基于信号的销毁请求替换直接destroy()调用解决内存泄漏 3. 添加适当的变更通知改进surface item处理 4. 增强鼠标区域的类型检查以排除未确定表面类型 Log: 修复窗口圆角显示和闪屏清理时的内存泄漏问题 Influence: 1. 测试不同圆角值下的窗口创建 2. 验证预启动闪屏正确消失且无内存泄漏 3. 检查所有表面类型上的鼠标交互是否正常 4. 验证窗口装饰边框是否遵循正确的圆角继承 5. 测试从未确定状态到正常状态的表面类型转换
zccrs
approved these changes
Oct 29, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wineee, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
zccrs
merged commit d140a7a
into
linuxdeepin:develop/splash-screen
5 of 9 checks passed
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
3 participants
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.
to prelaunch splash
based destruction request
types
Log: Fixed window corner radius display and memory leak during splash
screen cleanup
Influence:
leaks
fix: 修复圆角和内存泄漏问题
Log: 修复窗口圆角显示和闪屏清理时的内存泄漏问题
Influence: