feat: enhance window identification with ApplicationManager #1317
Conversation
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
Reviewer's GuideThis PR integrates ApplicationManager for pidfd-based window identification in the TaskManager, refactors X11Window identity to remove circular desktopfileID dependencies, reorders window tracking signal emission for proper initialization, and updates dependencies for syscalls and D-Bus usage. Sequence diagram for window identification with ApplicationManager D-Bus integrationsequenceDiagram
participant TaskManager
participant ApplicationManager
participant Model
TaskManager->>ApplicationManager: Identify(QDBusUnixFileDescriptor(pidfd))
ApplicationManager-->>TaskManager: desktopId
TaskManager->>Model: match(desktopId)
Model-->>TaskManager: matchRes
Note over TaskManager: If matchRes is valid, use it
TaskManager->>Model: match(identifies.value(0))
Model-->>TaskManager: fallbackRes
Entity relationship diagram for window and desktop ID matchingerDiagram
WINDOW {
pid int
wm_class string
}
APPLICATION_MANAGER {
desktop_id string
pidfd int
}
MODEL {
desktop_id string
}
WINDOW ||--o{ APPLICATION_MANAGER: "identified by pidfd"
APPLICATION_MANAGER ||--o{ MODEL: "desktop_id used for matching"
Class diagram for updated X11Window identity and TaskManager integrationclassDiagram
class X11Window {
+QStringList identity()
-m_identity: QStringList
-m_windowID: xcb_window_t
-pid(): int
-getAppItem() [removed desktopfileID usage]
}
class TaskManager {
+init()
-model: Model
-roleNames: QHash
-window identification logic (uses ApplicationManager)
}
X11Window <.. TaskManager: uses identity()
class ApplicationManager {
+Identify(QDBusUnixFileDescriptor)
}
TaskManager ..> ApplicationManager: D-Bus call
File-Level Changes
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:
- Wrap the pidfd_open syscall and ApplicationManager D-Bus logic in compile-time guards (e.g. check SYS_pidfd_open availability) to maintain portability on platforms that don’t support pidfd.
- The synchronous D-Bus Identify call in init() could block the main thread—consider dispatching it asynchronously or moving it off the UI thread and caching results to avoid UI stutters.
- Avoid recreating DDBusSender on every identify call by reusing a single sender instance or lazily initializing it for better performance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Wrap the pidfd_open syscall and ApplicationManager D-Bus logic in compile-time guards (e.g. check SYS_pidfd_open availability) to maintain portability on platforms that don’t support pidfd.
- The synchronous D-Bus Identify call in init() could block the main thread—consider dispatching it asynchronously or moving it off the UI thread and caching results to avoid UI stutters.
- Avoid recreating DDBusSender on every identify call by reusing a single sender instance or lazily initializing it for better performance.
## Individual Comments
### Comment 1
<location> `panels/dock/taskmanager/taskmanager.cpp:152-162` </location>
<code_context>
+
+ if (res.isValid()) {
+ auto reply = res.reply();
+ QList<QVariant> data = reply.arguments();
+ QString desktopId = data.first().toString();
+
+ if (!desktopId.isEmpty()) {
</code_context>
<issue_to_address>
**suggestion:** Check that 'data' is not empty before accessing 'first()'.
Consider adding a check to ensure 'data' is not empty before calling 'first()' to prevent a potential exception.
```suggestion
QList<QVariant> data = reply.arguments();
QString desktopId;
if (!data.isEmpty()) {
desktopId = data.first().toString();
}
if (!desktopId.isEmpty()) {
// 使用获取到的 desktop ID 进行匹配
auto matchRes = model->match(model->index(0, 0), roleNames.key(MODEL_DESKTOPID), desktopId, 1, Qt::MatchFixedString | Qt::MatchWrap).value(0);
if (matchRes.isValid()) {
qCDebug(taskManagerLog) << "matched by AM desktop ID:" << desktopId << matchRes;
return matchRes;
}
}
```
</issue_to_address>
### Comment 2
<location> `panels/dock/taskmanager/taskmanager.cpp:135` </location>
<code_context>
}
}
+ pid_t windowPid = identifies.last().toInt();
+ if (windowPid > 0) {
+ bool amIsAvailable = QDBusConnection::sessionBus().interface()->isServiceRegistered("org.desktopspec.ApplicationManager1");
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the deeply nested logic into a guard-clause-based helper function to simplify and flatten the code.
Here’s one way to collapse that deep nesting into a small, testable helper with guard-clauses and Qt’s higher-level D-Bus API. This preserves exactly the same logic but:
1. fails early on any error
2. lives in one place so your lambda stays flat
3. avoids manual `DDBusSender` chaining
```cpp
// in desktopidentifier.h
#pragma once
#include <optional>
#include <QString>
#include <QDBusUnixFileDescriptor>
std::optional<QString> identifyDesktopId(pid_t windowPid);
// in desktopidentifier.cpp
#include "desktopidentifier.h"
#include <QDBusConnection>
#include <QDBusInterface>
#include <unistd.h>
#include <sys/syscall.h>
std::optional<QString> identifyDesktopId(pid_t windowPid)
{
// 1) must have a valid PID
if (windowPid <= 0) {
return std::nullopt;
}
// 2) check ApplicationManager1 is up
auto session = QDBusConnection::sessionBus();
if (!session.interface()->isServiceRegistered("org.desktopspec.ApplicationManager1")) {
return std::nullopt;
}
// 3) open pidfd
int pidfd = syscall(SYS_pidfd_open, windowPid, 0);
if (pidfd < 0) {
return std::nullopt;
}
// 4) call Identify()
QDBusInterface iface(
"org.desktopspec.ApplicationManager1",
"/org/desktopspec/ApplicationManager1",
"org.desktopspec.ApplicationManager1",
session);
auto reply = iface.call("Identify", QDBusUnixFileDescriptor(pidfd));
::close(pidfd);
if (!reply.isValid()) {
qCDebug(taskManagerLog) << "AM Identify failed:" << reply.error().message();
return std::nullopt;
}
// 5) extract desktopId
const auto args = reply.arguments();
const auto desktopId = args.value(0).toString();
if (desktopId.isEmpty()) {
return std::nullopt;
}
return desktopId;
}
```
Then your lambda becomes:
```cpp
pid_t windowPid = identifies.last().toInt();
if (auto desktopId = identifyDesktopId(windowPid)) {
auto idx = model->match(
model->index(0,0),
roleNames.key(MODEL_DESKTOPID),
*desktopId,
/*count=*/1,
Qt::MatchFixedString | Qt::MatchWrap);
if (!idx.isEmpty()) {
qCDebug(taskManagerLog) << "matched by AM desktop ID:" << *desktopId << idx.value(0);
return idx.value(0);
}
}
// fallback to old matching
auto res = model->match(
model->index(0,0),
roleNames.key(MODEL_DESKTOPID),
identifies.value(0),
/*count=*/1,
Qt::MatchEndsWith);
qCDebug(taskManagerLog) << "matched" << res.value(0);
return res.value(0);
```
This pulls out all the nesting into a helper, uses guard‐clauses to exit on error, and keeps your original 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.
18202781743
reviewed
Oct 27, 2025
| } | ||
| } | ||
|
|
||
| pid_t windowPid = identifies.last().toInt(); |
Contributor
There was a problem hiding this comment.
调用dtkcore的DSGApplication::id这种接口试试，它获取的是AppId，应该跟这个是一样的，
18202781743
reviewed
Oct 27, 2025
| } | ||
| } | ||
|
|
||
| pid_t windowPid = identifies.last().toInt(); |
18202781743
reviewed
Oct 27, 2025
Contributor
18202781743
left a comment
18202781743
left a comment
There was a problem hiding this comment.
分两笔提交，一个pr，先revert之前那一笔，再在基础上修改，
wjyrich
force-pushed
the
fix-identify
branch
2 times, most recently
from
0d78d0e to
d8956c2
Compare
18202781743
previously approved these changes
Oct 27, 2025
wjyrich
force-pushed
the
fix-identify
branch
2 times, most recently
from
afebbd5 to
00942a7
Compare
1. Added ApplicationManager D-Bus integration for more accurate window identification 2. Implemented pidfd-based process identification to get desktop IDs from ApplicationManager 3. Removed desktopfileID from X11Window identity to avoid circular dependencies 4. Reordered window tracking and signal emission to ensure proper initialization 5. Added syscall and D-Bus dependencies for process identification feat: 增强窗口识别功能，集成 ApplicationManager 1. 添加 ApplicationManager D-Bus 集成以实现更精确的窗口识别 2. 实现基于 pidfd 的进程识别，从 ApplicationManager 获取桌面 ID 3. 从 X11Window 身份信息中移除 desktopfileID 以避免循环依赖 4. 重新排序窗口跟踪和信号发射以确保正确初始化 5. 添加系统调用和 D-Bus 依赖以支持进程识别
deepin pr auto review我来对这段代码变更进行审查：
static QString getDesktopIdByPid(const QStringList &identifies)
{
if (identifies.isEmpty())
return {};
bool ok;
pid_t windowPid = identifies.last().toInt(&ok);
if (!ok || windowPid <= 0 || windowPid > PID_MAX_LIMIT) {
qCWarning(taskManagerLog) << "Invalid pid value:" << identifies.last();
return {};
}
auto appId = DSGApplication::getId(windowPid);
if (appId.isEmpty()) {
qCDebug(taskManagerLog) << "appId is empty, AM failed to identify window with pid:" << windowPid;
return {};
}
return QString::fromUtf8(appId);
}
这些修改将提高代码的健壮性、可维护性和性能，同时保持代码的安全性。 |
18202781743
approved these changes
Oct 28, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, wjyrich 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 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: behind) |
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.
feat: 增强窗口识别功能，集成 ApplicationManager
Summary by Sourcery
Use pidfd and ApplicationManager D-Bus integration to enhance window identification accuracy and adjust window tracking initialization sequence
New Features:
Enhancements: