-
Notifications
You must be signed in to change notification settings - Fork 55
feat: add icon theme change handling for X11 windows #1170
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
Conversation
Reviewer's GuideIntroduces dynamic icon theme support by clearing and reloading window icons when the system icon theme changes, ensuring task manager icons stay in sync. Sequence diagram for icon theme change handling in X11WindowMonitorsequenceDiagram
participant DPlatformTheme as DPlatformTheme
participant X11WindowMonitor as X11WindowMonitor
participant X11Window as X11Window
DPlatformTheme->>X11WindowMonitor: iconThemeNameChanged()
X11WindowMonitor->>X11Window: resetIcon() (for each window)
X11Window-->>X11WindowMonitor: iconChanged() signal (emitted)
Class diagram for X11Window and X11WindowMonitor icon theme change supportclassDiagram
class X11Window {
+QString icon()
+void resetIcon()
+QString title()
...
}
class X11WindowMonitor {
+void onIconThemeChanged()
...
}
X11WindowMonitor --> X11Window : manages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743 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 |
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 @18202781743 - I've reviewed your changes - here's some feedback:
- Consider disconnecting the iconThemeNameChanged signal in X11WindowMonitor’s destructor (or using a scoped connection) to avoid dangling subscriptions if the monitor is torn down before the application helper.
- Resetting icons for all tracked windows in one go on every theme change could be costly; consider debouncing rapid theme-change events or limiting resets to only visible/mapped windows to reduce unnecessary work.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider disconnecting the iconThemeNameChanged signal in X11WindowMonitor’s destructor (or using a scoped connection) to avoid dangling subscriptions if the monitor is torn down before the application helper.
- Resetting icons for all tracked windows in one go on every theme change could be costly; consider debouncing rapid theme-change events or limiting resets to only visible/mapped windows to reduce unnecessary work.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
TAG Bot New tag: 2.0.2 |
1. Added sorting of notification entities by creation time (cTime) in descending order 2. Ensures newest notifications appear first in the list 3. Improves user experience by showing most recent notifications at the top 4. Sorting happens before the model reset to maintain data consistency fix: 在显示前按时间戳排序通知 1. 添加了按创建时间(cTime)降序排列通知实体的功能 2. 确保最新的通知显示在列表最前面 3. 通过将最新通知显示在顶部来提升用户体验 4. 排序操作在模型重置前完成以保持数据一致性 pms:BUG-299935
deepin pr auto review关键摘要:
是否建议立即修改:
|
emit change signal
new theme
theme changes
feat: 为X11窗口添加图标主题变更处理
Summary by Sourcery
Handle icon theme changes for X11 windows by clearing cached icons and forcing reload when the system icon theme changes
New Features: