-
Notifications
You must be signed in to change notification settings - Fork 55
feat: add category-based skip list for cgroups grouping #1384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ivy233 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 |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Reviewer's GuideAdds a configurable, category-based skip list to the task manager’s cgroups-based window grouping, so that apps in specified desktop Categories (default: TerminalEmulator) bypass cgroup-based grouping and avoid being incorrectly grouped under terminal emulators. Sequence diagram for cgroups-based grouping with category skip checksequenceDiagram
actor User
participant TerminalEmulator
participant GUIApp
participant TaskManager
participant Settings
participant ApplicationManager
participant Application
User->>TerminalEmulator: Start terminal emulator
User->>TerminalEmulator: Run graphical app command
TerminalEmulator->>GUIApp: Spawn GUI process
GUIApp->>TaskManager: New window detected
TaskManager->>Settings: cgroupsBasedGrouping()
alt cgroups based grouping enabled
TaskManager->>TaskManager: getDesktopIdByPid(identifies)
alt desktopId is empty
TaskManager->>TaskManager: Fallback grouping (no desktopId)
else desktopId is not empty
TaskManager->>Settings: cgroupsBasedGroupingSkipIds()
alt desktopId in skip id list
TaskManager->>TaskManager: Skip cgroups grouping (whitelisted by id)
else desktopId not in skip id list
TaskManager->>TaskManager: shouldSkipCgroupsByCategories(desktopId)
TaskManager->>Settings: cgroupsBasedGroupingSkipCategories()
TaskManager->>ApplicationManager: Create Application proxy with desktopId
alt Application proxy is valid
TaskManager->>Application: categories()
alt category in skip category list
TaskManager->>TaskManager: Skip cgroups grouping (whitelisted by category)
else no matching category
TaskManager->>TaskManager: Perform cgroups based grouping
end
else Application proxy invalid
TaskManager->>TaskManager: Perform cgroups based grouping
end
end
end
else cgroups based grouping disabled
TaskManager->>TaskManager: Use existing non cgroups grouping
end
TaskManager-->>User: Window appears in taskbar (grouped or independent depending on checks)
Class diagram for updated TaskManagerSettings and Application interface usageclassDiagram
class TaskManagerSettings {
-bool m_cgroupsBasedGrouping
-QStringList m_dockedElements
-QStringList m_cgroupsBasedGroupingSkipAppIds
-QStringList m_cgroupsBasedGroupingSkipCategories
+TaskManagerSettings(QObject parent)
+bool cgroupsBasedGrouping()
+QStringList cgroupsBasedGroupingSkipIds()
+QStringList cgroupsBasedGroupingSkipCategories()
+QStringList dockedElements()
+void setDockedElements(QStringList elements)
+void toggleDockedElement(QString element)
}
class TaskManager {
+bool init()
}
class Application {
+QStringList categories()
+bool isValid()
}
class Settings {
+bool cgroupsBasedGrouping()
+QStringList cgroupsBasedGroupingSkipIds()
+QStringList cgroupsBasedGroupingSkipCategories()
}
TaskManager --> Settings : uses
TaskManager --> Application : queries categories via DBus
Settings --> TaskManagerSettings : wraps persistent config
TaskManagerSettings ..> QStringList : stores skip lists
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- The per-window DBus lookup in shouldSkipCgroupsByCategories may add noticeable overhead in the hot path of window matching; consider caching categories by desktopId or precomputing the skip decision to avoid repeated DBus calls.
- When checking skipCategories.contains(category), you might want to normalize case or use a QSet for skipCategories to avoid subtle mismatches and to make the membership checks more efficient.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The per-window DBus lookup in shouldSkipCgroupsByCategories may add noticeable overhead in the hot path of window matching; consider caching categories by desktopId or precomputing the skip decision to avoid repeated DBus calls.
- When checking skipCategories.contains(category), you might want to normalize case or use a QSet for skipCategories to avoid subtle mismatches and to make the membership checks more efficient.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| QString dbusPath = QStringLiteral("/org/desktopspec/ApplicationManager1/") + escapeToObjectPath(desktopId); | ||
|
|
||
| // 创建 Application 接口 | ||
| Application appInterface(QStringLiteral("org.desktopspec.ApplicationManager1"), | ||
| dbusPath, | ||
| QDBusConnection::sessionBus()); | ||
|
|
||
| if (!appInterface.isValid()) { | ||
| qCDebug(taskManagerLog) << "Failed to create Application interface for:" << desktopId; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不再绕一次 dbus 了,在 AppItem 一层新增一个 CategoryRole(可以参考现有的 DDECategoryRole),然后直接使用这个新增的 role 即可。
|
recheck |
a0e2e66 to
3b87e6d
Compare
| if (activeAppModel) { | ||
| auto existingItem = activeAppModel->match(activeAppModel->index(0, 0), roleNames.key("desktopId"), desktopId, 1, Qt::MatchFixedString | Qt::MatchWrap).value(0); | ||
| if (existingItem.isValid()) { | ||
| categories = activeAppModel->data(existingItem, roleNames.key("categories")).toStringList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不用再从 roleNames.key("categories") 获取 role 了吧?已经有 TaskManager::CategoriesRole 了。
| } | ||
|
|
||
| // 检查是否有任何 category 在跳过列表中 | ||
| for (const QString &category : categories) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是不是反了,不应该是迭代 skipCategories 然后判断 categories 是否包含 skipCategoriy 吗?
skipCategories 配置里预期不会有太多值,默认只有终端,也不期望用户自己随便加。
| } | ||
|
|
||
| // 检查应用的 Categories 是否在跳过列表中 | ||
| static bool shouldSkipCgroupsByCategories(const QString &desktopId, QAbstractItemModel *activeAppModel, const QHash<int, QByteArray> &roleNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
倒不如直接把 “尝试通过AM(Application Manager)匹配应用程序” 那块提出来。只提目前这一部分有点怪怪的。
|
TAG Bot New tag: 2.0.25 |
|
recheck |
|
TAG Bot New tag: 2.0.26 |
1. 新增 DConfig 配置项 cgroupsBasedGroupingSkipCategories 2. 默认跳过 TerminalEmulator 类别的应用,避免终端中启动的程序被错误识别 3. 在 dde-apps 的 AppItem 中添加 categories 属性 4. 在 AppItemModel 中添加 CategoriesRole 5. 在 AMAppItem 构造函数中保存从 AM 获取的 categories 6. 实现 shouldSkipCgroupsByCategories() 函数,从 dde-apps 缓存读取 categories 7. 提取 tryMatchByApplicationManager() 函数,优化代码结构 8. 使用 TaskManager::CategoriesRole 枚举常量替代运行时查找 9. 优化迭代逻辑,迭代 skipCategories 而非 categories,提升性能 10. 在窗口匹配流程中集成类别检查逻辑 Log: 新增基于应用类别的 cgroups 分组跳过列表,解决终端中启动的图形程序被错误归类的问题 Influence: 1. 测试在 Alacritty 等终端模拟器中启动图形应用 2. 验证新启动的应用在任务栏显示独立图标 3. 验证 categories 从 dde-apps 缓存读取,无额外 DBus 调用 4. 检查 DConfig 配置项可正常读取和修改 5. 确认不影响非终端类应用的正常分组行为 6. 验证深度终端等原有白名单应用仍正常工作 7. 确认代码结构优化不影响功能正确性 PMS: TASK-384865
3b87e6d to
62a5b07
Compare
deepin pr auto review代码审查报告概述这段代码主要是为任务管理器添加基于应用类别的分组跳过功能。当应用属于特定类别时,可以跳过基于cgroups的应用分组。下面是对代码的详细审查: 语法逻辑
代码质量
代码性能
代码安全
改进建议
总体评价这段代码实现了基于应用类别的分组跳过功能,逻辑清晰,与现有代码集成良好。主要改进点在于性能优化和错误处理。建议实施上述优化措施,以提高代码的健壮性和性能。 |
概述
新增基于应用类别(Categories)的 cgroups 分组跳过列表功能,解决终端模拟器中启动的图形程序被错误归类到终端应用的问题。
问题描述
目前任务栏基于 cgroup 识别应用时,终端中启动的图形程序会继承父进程(终端)的 cgroup 信息,导致窗口被错误识别为归属终端应用。虽然存在白名单 cgroupsBasedGroupingSkipAppIds,但需要逐个手动添加终端模拟器的 appId。
解决方案
新增 DConfig 配置项 cgroupsBasedGroupingSkipCategories
实现类别检查函数 shouldSkipCgroupsByCategories()
集成到窗口匹配流程
修改文件
测试验证
Summary by Sourcery
Add configurable category-based skip list for cgroup-based taskbar grouping to avoid mis-grouping terminal-launched GUI apps.
New Features:
Enhancements: