feat: support treeland prelaunch splash manager v2#332
feat: support treeland prelaunch splash manager v2#332zccrs merged 1 commit intolinuxdeepin:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Pull request overview
Adds support for the treeland prelaunch splash manager v2 protocol by associating splashes with application instance IDs and managing their lifecycle alongside instance creation/removal.
Changes:
- Switch Wayland protocol generation and helper implementation from prelaunch-splash v1 to v2.
- Generate and propagate an instance UUID early so the compositor can associate a splash with a specific instance.
- Track and close splash objects per instance, including bulk cleanup on instance detachment and helper destruction.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dbus/prelaunchsplashhelper.h | Updates helper API to v2 and adds per-instance splash object tracking. |
| src/dbus/prelaunchsplashhelper.cpp | Implements v2 create/close logic and destructor cleanup for tracked splash objects. |
| src/dbus/applicationservice.h | Adds tracking for splash instance IDs and declares new cleanup helpers. |
| src/dbus/applicationservice.cpp | Generates instance UUID earlier, shows splash with instance association, and closes splashes on instance lifecycle events. |
| src/dbus/CMakeLists.txt | Switches generated Wayland protocol sources from v1 XML to v2 XML. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a701323 to
ade9878
Compare
Reviewer's GuideMigrates the prelaunch splash Wayland integration from the v1 manager to treeland-prelaunch-splash-manager v2, introduces instance-scoped splash tracking with UUIDs, and ensures proper lifecycle management and cleanup of splash surfaces when instances terminate or the application manager shuts down. Sequence diagram for instance-scoped prelaunch splash lifecyclesequenceDiagram
actor User
participant ApplicationService
participant ApplicationManager1Service as ApplicationManager
participant PrelaunchSplashHelper as SplashHelper
participant WaylandCompositor as Compositor
User->>ApplicationService: Launch(action, fields, options)
ApplicationService->>ApplicationService: validate options
ApplicationService->>ApplicationService: generate instanceRandomUUID
ApplicationService->>ApplicationManager: parent()
ApplicationManager-->>ApplicationService: pointer
ApplicationService->>ApplicationManager: splashHelper()
ApplicationManager-->>ApplicationService: SplashHelper
alt isAutostartLaunch or singletonWithInstance
ApplicationService-->>User: skip splash
else show splash
ApplicationService->>SplashHelper: show(appId, instanceRandomUUID, iconName)
SplashHelper->>SplashHelper: check isActive
SplashHelper->>SplashHelper: ensure no existing splash for instanceRandomUUID
SplashHelper->>SplashHelper: buildIconBuffer(icon)
SplashHelper->>Compositor: create_splash(appId, instanceRandomUUID, appManagerId, buffer)
Compositor-->>SplashHelper: treeland_prelaunch_splash_v2 object
SplashHelper->>SplashHelper: store splash in m_splashObjects[instanceRandomUUID]
ApplicationService->>ApplicationService: m_splashInstanceIds.insert(instanceRandomUUID)
end
ApplicationService->>ApplicationManager: jobManager.addJob(path, lambda(instanceRandomUUID))
ApplicationManager-->>ApplicationService: job created
rect rgb(230,230,250)
Note over ApplicationService,ApplicationManager: Later, when an instance is removed
ApplicationManager->>ApplicationService: removeOneInstance(instancePath)
ApplicationService->>ApplicationService: lookup InstanceService
ApplicationService->>ApplicationService: closeSplashForInstance(instanceId)
ApplicationService->>ApplicationService: m_splashInstanceIds.remove(instanceId)
ApplicationService->>ApplicationManager: splashHelper()
ApplicationManager-->>ApplicationService: SplashHelper
ApplicationService->>SplashHelper: closeSplash(instanceId)
SplashHelper->>SplashHelper: splash = m_splashObjects.take(instanceId)
alt splash exists
SplashHelper->>SplashHelper: splash.destroy()
SplashHelper->>SplashHelper: delete splash
SplashHelper-->>ApplicationService: splash closed
else no splash
SplashHelper-->>ApplicationService: no active splash
end
end
rect rgb(220,245,220)
Note over SplashHelper: Application manager shutdown
ApplicationManager->>ApplicationService: detachAllInstance()
ApplicationService->>ApplicationService: m_Instances.clear()
ApplicationService->>ApplicationService: closeAllSplashes()
loop for each instanceId in m_splashInstanceIds
ApplicationService->>ApplicationService: closeSplashForInstance(instanceId)
ApplicationService->>SplashHelper: closeSplash(instanceId)
end
SplashHelper->>SplashHelper: ~PrelaunchSplashHelper()
SplashHelper->>SplashHelper: destroy all remaining splash objects
end
Updated class diagram for ApplicationService and PrelaunchSplashHelperclassDiagram
class ApplicationService {
+QHash~QDBusObjectPath, QSharedPointer~InstanceService~~ m_Instances
+QSet~QString~ m_splashInstanceIds
+Launch(action: QString, fields: QStringList, options: QVariantMap) : QDBusObjectPath
+removeOneInstance(instance: QDBusObjectPath) void
+detachAllInstance() void
+closeSplashForInstance(instanceId: QString) void
+closeAllSplashes() void
+parent() ApplicationManager1Service*
+parent() const ApplicationManager1Service*
-updateAfterLaunch(isLaunch: bool) void
-shouldBeShown(entry: unique_ptr~DesktopEntry~) bool
-autostartCheck(filePath: QString) const bool
-setAutostartSource(source: AutostartSource&&) void
-appendExtraEnvironments(runtimeOptions: QVariantMap&) const void
-processCompatibility(action: QString, options: QVariantMap&, execStr: QString&) void
}
class PrelaunchSplashHelper {
+m_iconBuffers: vector~unique_ptr~QWaylandShmBuffer~~
+m_splashObjects: QHash~QString, QtWayland::treeland_prelaunch_splash_v2*~
+PrelaunchSplashHelper()
+~PrelaunchSplashHelper()
+show(appId: QString, instanceId: QString, iconName: QString) void
+closeSplash(instanceId: QString) void
+buildIconBuffer(icon: QIcon) wl_buffer*
+bufferRelease(data: void*, buffer: wl_buffer*) void
+handleBufferRelease(buffer: wl_buffer*) void
}
class ApplicationManager1Service {
+splashHelper() PrelaunchSplashHelper*
+jobManager() JobManager&
}
class InstanceService {
+instanceId() const QString
}
class JobManager {
+addJob(path: QString, callback: function) QVariant
}
class QWaylandClientExtensionTemplate_PrelaunchSplashHelper {
}
class treeland_prelaunch_splash_manager_v2 {
}
class treeland_prelaunch_splash_v2 {
+destroy() void
}
PrelaunchSplashHelper --|> QWaylandClientExtensionTemplate_PrelaunchSplashHelper
PrelaunchSplashHelper --|> treeland_prelaunch_splash_manager_v2
ApplicationService --> ApplicationManager1Service : parent
ApplicationManager1Service --> PrelaunchSplashHelper : owns
ApplicationService --> InstanceService : manages instances
ApplicationService --> JobManager : uses
PrelaunchSplashHelper --> treeland_prelaunch_splash_v2 : creates and stores
ApplicationService --> PrelaunchSplashHelper : uses for show and closeSplash
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/dbus/applicationservice.cpp" line_range="375" />
<code_context>
+ const auto iconVar = findEntryValue(DesktopFileEntryKey, "Icon", EntryValueType::IconString);
+ const QString iconName = iconVar.isNull() ? QString{} : iconVar.toString();
+ qCInfo(amPrelaunchSplash) << "Show prelaunch splash request" << id() << "instance" << instanceRandomUUID << "icon" << iconName;
+ helper->show(id(), instanceRandomUUID, iconName);
+ m_splashInstanceIds.insert(instanceRandomUUID);
+ } else {
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing `m_splashInstanceIds` from `ApplicationService` and delegating all splash lifecycle management to `PrelaunchSplashHelper` to avoid duplicated state and control flow.
You can drop `m_splashInstanceIds` and the extra close indirection and let `PrelaunchSplashHelper` be the single source of truth for splash state.
Instead of tracking instance IDs in `ApplicationService` and guarding every close with `m_splashInstanceIds.remove(...)`, you can:
1. Stop inserting into `m_splashInstanceIds` when showing the splash.
2. Remove `closeSplashForInstance` and `closeAllSplashes`.
3. Directly call the helper’s `closeSplash(instanceId)` wherever you tear down instances.
4. Optionally add a `closeAll()` to the helper if global teardown is needed.
This keeps all behavior (per-instance splash + teardown) but removes duplicated state and control flow.
### 1. Show splash without `m_splashInstanceIds`
```cpp
// Before
helper->show(id(), instanceRandomUUID, iconName);
m_splashInstanceIds.insert(instanceRandomUUID);
// After
helper->show(id(), instanceRandomUUID, iconName);
```
### 2. Simplify `removeOneInstance`
```cpp
void ApplicationService::removeOneInstance(const QDBusObjectPath &instance) noexcept
{
if (auto it = m_Instances.constFind(instance); it != m_Instances.cend()) {
const auto instanceId = it.value()->instanceId();
if (auto *am = parent()) {
if (auto *helper = am->splashHelper()) {
helper->closeSplash(instanceId); // ignore unknown ids internally
}
}
emit InterfacesRemoved(instance, getChildInterfacesFromObject(it->data()));
unregisterObjectFromDBus(instance.path());
m_Instances.remove(instance);
}
}
```
### 3. Simplify `detachAllInstance`
If you only need to close splashes for currently tracked instances:
```cpp
void ApplicationService::detachAllInstance() noexcept
{
for (auto it = m_Instances.constBegin(); it != m_Instances.constEnd(); ++it) {
const auto &instance = it.value();
orphanedInstances.append(instance);
instance->setProperty("Orphaned", true);
if (auto *am = parent()) {
if (auto *helper = am->splashHelper()) {
helper->closeSplash(instance->instanceId());
}
}
}
m_Instances.clear();
}
```
### 4. Optional: centralize a global close in the helper
If you truly need a “close every splash” path (e.g. on compositor restart), add it to `PrelaunchSplashHelper`:
```cpp
// In PrelaunchSplashHelper
void PrelaunchSplashHelper::closeAll()
{
for (auto it = m_splashObjects.begin(); it != m_splashObjects.end(); ++it) {
// existing teardown for a single splash
closeSplash(it.key());
}
}
```
Then call it instead of `closeAllSplashes()`:
```cpp
if (auto *am = parent()) {
if (auto *helper = am->splashHelper()) {
helper->closeAll();
}
}
```
This removes `m_splashInstanceIds`, `closeSplashForInstance`, and `closeAllSplashes` from `ApplicationService`, cuts the extra loops/guards, and keeps all functionality while reducing cross-component state and complexity.
</issue_to_address>
### Comment 2
<location path="src/dbus/applicationservice.h" line_range="203" />
<code_context>
DesktopFile m_desktopSource;
QSharedPointer<DesktopEntry> m_entry{nullptr};
QHash<QDBusObjectPath, QSharedPointer<InstanceService>> m_Instances;
+ QSet<QString> m_splashInstanceIds;
void updateAfterLaunch(bool isLaunch) noexcept;
static bool shouldBeShown(const std::unique_ptr<DesktopEntry> &entry) noexcept;
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing splash-related state from ApplicationService and delegating splash closing directly to the helper to avoid duplicated state and tighter coupling.
You’ve added `m_splashInstanceIds` plus `closeSplashForInstance` / `closeAllSplashes`, which introduces new state in `ApplicationService` solely to mirror splash state that should already be owned by the splash helper. That does increase complexity and coupling.
If `PrelaunchSplashHelper` (or equivalent) already tracks splash objects per instance, you can simplify by:
* Removing the extra container from `ApplicationService`.
* Letting `ApplicationService` delegate directly to the helper for per-instance and “close all” behavior.
* Keeping any close helpers private and stateless.
For example, instead of:
```cpp
// applicationservice.h
QHash<QDBusObjectPath, QSharedPointer<InstanceService>> m_Instances;
QSet<QString> m_splashInstanceIds;
void closeSplashForInstance(const QString &instanceId) noexcept;
void closeAllSplashes() noexcept;
```
you can reduce state to:
```cpp
// applicationservice.h
QHash<QDBusObjectPath, QSharedPointer<InstanceService>> m_Instances;
// No splash ID container here; helper owns splash state
void closeSplashForInstance(const QString &instanceId) noexcept;
void closeAllSplashes() noexcept;
```
and implement the methods as thin delegates in the `.cpp`:
```cpp
// applicationservice.cpp
void ApplicationService::closeSplashForInstance(const QString &instanceId) noexcept
{
if (auto helper = parent()->prelaunchSplashHelper()) {
helper->closeSplash(instanceId);
}
}
void ApplicationService::closeAllSplashes() noexcept
{
if (auto helper = parent()->prelaunchSplashHelper()) {
helper->closeAllSplashesForApp(m_desktopId);
}
}
```
If `closeSplashesForApp` doesn’t exist yet, it can be added to the helper:
```cpp
// prelaunchsplashhelper.h
void closeAllSplashesForApp(const QString &desktopId) noexcept;
```
```cpp
// prelaunchsplashhelper.cpp
void PrelaunchSplashHelper::closeAllSplashesForApp(const QString &desktopId) noexcept
{
for (const auto &id : m_splashObjects.keys()) {
if (/* id belongs to desktopId */) {
closeSplash(id);
}
}
}
```
This keeps all splash-related state (`instanceId` tracking, lifetime, etc.) in one place (the helper), and `ApplicationService` becomes a simple client rather than a second source of truth.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
43955c7 to
2699235
Compare
1. Upgrade from treeland-prelaunch-splash-v1 to v2 protocol 2. Generate instance UUID early for splash association 3. Track splash objects per instance ID for proper lifecycle management 4. Add closeSplashForInstance and closeAllSplashes methods 5. Implement proper cleanup of splash objects in destructor Log: Added support for treeland prelaunch splash manager v2 with instance-based splash management Influence: 1. Test application launch with prelaunch splash display 2. Verify splash closes when instance is removed normally 3. Test multiple instances of same application with separate splashes 4. Verify splash cleanup during application manager shutdown 5. Test client crash scenarios to ensure orphaned splashes are cleaned up 6. Validate Wayland protocol compatibility with updated v2 interface feat: 支持 treeland 预启动闪屏管理器 v2 1. 从 treeland-prelaunch-splash-v1 升级到 v2 协议 2. 提前生成实例 UUID 用于闪屏关联 3. 按实例 ID 跟踪闪屏对象以实现正确的生命周期管理 4. 添加 closeSplashForInstance 和 closeAllSplashes 方法 5. 在析构函数中实现闪屏对象的正确清理 Log: 新增支持 treeland 预启动闪屏管理器 v2,支持基于实例的闪屏管理 Influence: 1. 测试应用程序启动时的预启动闪屏显示 2. 验证实例正常移除时闪屏正确关闭 3. 测试同一应用程序多个实例的独立闪屏管理 4. 验证应用程序管理器关闭时的闪屏清理 5. 测试客户端崩溃场景确保孤儿闪屏被清理 6. 验证与更新 v2 接口的 Wayland 协议兼容性
deepin pr auto review这份代码审查主要针对 以下是详细的审查意见: 1. 语法与逻辑审查1.1 Wayland 协议升级与接口变更
1.2 实例 ID 的生成与传递
1.3 Splash 的关闭时机
2. 代码质量2.1 Lambda 捕获与可变性
2.2 代码风格与格式
3. 代码性能
4. 代码安全
总结与改进建议总体评价:这是一次高质量的代码提交,主要完成了 Wayland 启动画面协议从 v1 到 v2 的迁移,并修复了实例 ID 管理相关的逻辑问题。代码逻辑严密,资源管理得当。 改进建议:
|
|
TAG Bot New tag: 1.2.46 |
|
[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 |
Log: Added support for treeland prelaunch splash manager v2 with instance-based splash management
Influence:
feat: 支持 treeland 预启动闪屏管理器 v2
Log: 新增支持 treeland 预启动闪屏管理器 v2,支持基于实例的闪屏管理
Influence:
Summary by Sourcery
Upgrade prelaunch splash handling to the treeland v2 protocol with instance-scoped lifecycle management.
New Features:
Bug Fixes:
Enhancements:
Build: