style: replace shadows with borders in OSD notifications #1441
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qxp930712 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 |
Reviewer's GuideThis PR updates OSD notification visuals to use inside/outside border components instead of shadow components, adds corresponding border palettes for checked/normal and light/dark themes, slightly tweaks dark-mode background opacity, and wires the new border styling into the main OSD window. Class diagram for updated OSD notification border stylingclassDiagram
class OSDWindow {
+D.Palette insideBorderColor
+D.Palette outsideBorderColor
+real windowRadius
}
class DisplayModeItem {
+bool isCurrent
+D.Palette backgroundColor
+D.Palette checkedBackgroundColor
+D.Palette insideBorderColor
+D.Palette checkedInsideBorderColor
+D.Palette outsideBorderColor
+D.Palette checkedOutsideBorderColor
}
class WindowEffectItem {
+bool isCurrent
+D.Palette backgroundColor
+D.Palette checkedBackgroundColor
+D.Palette insideBorderColor
+D.Palette checkedInsideBorderColor
+D.Palette outsideBorderColor
+D.Palette checkedOutsideBorderColor
}
class D_InsideBoxBorder {
+real radius
+D.Palette color
}
class D_OutsideBoxBorder {
+real radius
+D.Palette color
}
class D_BoxShadow {
-real shadowOffsetX
-real shadowOffsetY
-real shadowBlur
-real cornerRadius
-real spread
-D.Palette shadowColor
-bool hollow
}
class D_BoxInsetShadow {
-real shadowOffsetX
-real shadowOffsetY
-real shadowBlur
-real cornerRadius
-real spread
-D.Palette shadowColor
}
class D_Palette {
+color normal
+color normalDark
}
OSDWindow --> D_InsideBoxBorder : uses
OSDWindow --> D_OutsideBoxBorder : uses
DisplayModeItem --> D_InsideBoxBorder : uses
DisplayModeItem --> D_OutsideBoxBorder : uses
DisplayModeItem --> D_Palette : uses palettes
WindowEffectItem --> D_InsideBoxBorder : uses
WindowEffectItem --> D_OutsideBoxBorder : uses
WindowEffectItem --> D_Palette : uses palettes
D_BoxShadow ..x DisplayModeItem : removed
D_BoxInsetShadow ..x DisplayModeItem : removed
D_BoxShadow ..x WindowEffectItem : removed
D_BoxInsetShadow ..x WindowEffectItem : removed
Flow diagram for OSD item border color selectionflowchart TD
A[OSD item to render] --> B{isCurrent}
B -- true --> C[Use checked palettes]
B -- false --> D[Use normal palettes]
C --> E{Theme}
D --> E
E -- Light mode --> F[Background color = checkedBackgroundColor.normal or backgroundColor.normal]
E -- Dark mode --> G[Background color = checkedBackgroundColor.normalDark or backgroundColor.normalDark]
C --> H[Inside border color = checkedInsideBorderColor]
C --> I[Outside border color = checkedOutsideBorderColor]
D --> J[Inside border color = insideBorderColor]
D --> K[Outside border color = outsideBorderColor]
F --> L[Render D_InsideBoxBorder with chosen inside border color]
G --> L
H --> L
J --> L
F --> M[Render D_OutsideBoxBorder with chosen outside border color]
G --> M
I --> M
K --> M
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 - I've found 5 issues, and left some high level feedback:
- The new inside/outside border palettes are defined inline in multiple QML files with identical values; consider centralizing these palette definitions in a shared place (e.g., on the ColorSelector or a helper component) to avoid duplication and keep future visual tweaks consistent.
- Previously the shadows were hidden for the current item (
visible: !itemView.isCurrent), but the new borders are always drawn and only change color; verify this behavior matches the intended spec, or reintroduce conditional visibility if the current item should remain borderless. - The new
D.OutsideBoxBorderon the main OSD window is anchored to the full window and rendered above content (z: D.DTK.AboveOrder); ensure it does not interfere with hit-testing or clipping of child controls, and adjust its z-order orenabled/visibleproperties if needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new inside/outside border palettes are defined inline in multiple QML files with identical values; consider centralizing these palette definitions in a shared place (e.g., on the ColorSelector or a helper component) to avoid duplication and keep future visual tweaks consistent.
- Previously the shadows were hidden for the current item (`visible: !itemView.isCurrent`), but the new borders are always drawn and only change color; verify this behavior matches the intended spec, or reintroduce conditional visibility if the current item should remain borderless.
- The new `D.OutsideBoxBorder` on the main OSD window is anchored to the full window and rendered above content (`z: D.DTK.AboveOrder`); ensure it does not interfere with hit-testing or clipping of child controls, and adjust its z-order or `enabled`/`visible` properties if needed.
## Individual Comments
### Comment 1
<location> `panels/notification/osd/displaymode/package/main.qml:121-130` </location>
<code_context>
+ D.InsideBoxBorder {
</code_context>
<issue_to_address>
**issue (bug_risk):** ColorSelector references for inside border palettes appear to be undefined or misplaced.
`insideBorderColor` and `checkedInsideBorderColor` are defined on `D.InsideBoxBorder`, but the binding uses `D.ColorSelector.checkedInsideBorderColor` / `D.ColorSelector.insideBorderColor`. Unless `ColorSelector` is extended elsewhere with these properties, this will resolve to `undefined` or bypass the local palettes. Either move these properties into `D.ColorSelector` with the other colors, or bind directly to the local properties (e.g. `color: itemView.isCurrent ? checkedInsideBorderColor : insideBorderColor`).
</issue_to_address>
### Comment 2
<location> `panels/notification/osd/displaymode/package/main.qml:135-144` </location>
<code_context>
color: D.ColorSelector.insideBorderColor
}
+ D.OutsideBoxBorder {
+ property D.Palette outsideBorderColor: D.Palette {
+ normal: Qt.rgba(0, 0, 0, 0.1)
+ normalDark: Qt.rgba(0, 0, 0, 0.7)
+ }
+ radius: root.windowRadius
+ anchors.fill: parent
+ z: D.DTK.AboveOrder
+ color: D.ColorSelector.outsideBorderColor
+ }
+
Control {
property D.Palette textColor: D.Palette {
normal: Qt.rgba(0, 0, 0, 1)
diff --git a/panels/notification/osd/windoweffect/package/main.qml b/panels/notification/osd/windoweffect/package/main.qml
index 560fc13d8..d2670c72e 100644
--- a/panels/notification/osd/windoweffect/package/main.qml
+++ b/panels/notification/osd/windoweffect/package/main.qml
</code_context>
<issue_to_address>
**issue (bug_risk):** Outside border palettes are defined on the border item but bound via ColorSelector, which likely has no such properties.
`outsideBorderColor` and `checkedOutsideBorderColor` are defined on `D.OutsideBoxBorder`, but the binding still points to `D.ColorSelector.*`. Unless `ColorSelector` defines these properties, the new palettes won’t be used. Please either add these properties to `ColorSelector` or bind `color` directly to the local `D.OutsideBoxBorder` properties.
</issue_to_address>
### Comment 3
<location> `panels/notification/osd/windoweffect/package/main.qml:170-179` </location>
<code_context>
+ D.InsideBoxBorder {
</code_context>
<issue_to_address>
**issue (bug_risk):** Same potential mismatch between local inside border palettes and ColorSelector references as in displaymode.
The palettes `insideBorderColor` / `checkedInsideBorderColor` are defined on `D.InsideBoxBorder`, but the `color` binding uses `D.ColorSelector.*`, matching the inconsistent pattern in `displaymode/package/main.qml`. Please either move these palettes into `ColorSelector` or bind directly to the local border properties to avoid the mismatch.
</issue_to_address>
### Comment 4
<location> `panels/notification/osd/windoweffect/package/main.qml:184-193` </location>
<code_context>
color: D.ColorSelector.insideBorderColor
}
+ D.OutsideBoxBorder {
+ property D.Palette outsideBorderColor: D.Palette {
+ normal: Qt.rgba(0, 0, 0, 0.1)
+ normalDark: Qt.rgba(0, 0, 0, 0.7)
+ }
+ radius: root.windowRadius
+ anchors.fill: parent
+ z: D.DTK.AboveOrder
+ color: D.ColorSelector.outsideBorderColor
+ }
+
Control {
property D.Palette textColor: D.Palette {
normal: Qt.rgba(0, 0, 0, 1)
diff --git a/panels/notification/osd/windoweffect/package/main.qml b/panels/notification/osd/windoweffect/package/main.qml
index 560fc13d8..d2670c72e 100644
--- a/panels/notification/osd/windoweffect/package/main.qml
+++ b/panels/notification/osd/windoweffect/package/main.qml
</code_context>
<issue_to_address>
**issue (bug_risk):** Outside border color binding likely does not use the newly defined palettes on the border item.
Because `outsideBorderColor` and `checkedOutsideBorderColor` are defined on `D.OutsideBoxBorder` but the `color` binding uses `D.ColorSelector.outsideBorderColor`, the new palettes likely won’t apply unless `ColorSelector` forwards these properties. Please either bind `color` to the local properties or move the palette definitions into the `ColorSelector` configuration so they are actually used.
</issue_to_address>
### Comment 5
<location> `panels/notification/osd/package/main.qml:75-83` </location>
<code_context>
}
- D.BoxInsetShadow {
- visible: !itemView.isCurrent
+ D.OutsideBoxBorder {
+ property D.Palette outsideBorderColor: D.Palette {
+ normal: Qt.rgba(0, 0, 0, 0.05)
</code_context>
<issue_to_address>
**issue (bug_risk):** OutsideBoxBorder defines its own outsideBorderColor palette but uses ColorSelector for the binding.
Here you declare a local `outsideBorderColor` palette, but `color` is still bound to `D.ColorSelector.outsideBorderColor`. If `ColorSelector` doesn’t expose this property, the color may be `undefined` and won’t use the local palette. To align with the inside border and the local definition, bind `color` to the local `outsideBorderColor` (or move this palette into `ColorSelector` if that’s the intended source).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| D.InsideBoxBorder { | ||
| property D.Palette insideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.1) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.05) | ||
| } | ||
| property D.Palette checkedInsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.15) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.08) | ||
| } | ||
| radius: backgroundRect.radius |
There was a problem hiding this comment.
issue (bug_risk): ColorSelector references for inside border palettes appear to be undefined or misplaced.
insideBorderColor and checkedInsideBorderColor are defined on D.InsideBoxBorder, but the binding uses D.ColorSelector.checkedInsideBorderColor / D.ColorSelector.insideBorderColor. Unless ColorSelector is extended elsewhere with these properties, this will resolve to undefined or bypass the local palettes. Either move these properties into D.ColorSelector with the other colors, or bind directly to the local properties (e.g. color: itemView.isCurrent ? checkedInsideBorderColor : insideBorderColor).
| D.OutsideBoxBorder { | ||
| property D.Palette outsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(0, 0, 0, 0.05) | ||
| normalDark: Qt.rgba(0, 0, 0, 0.4) | ||
| } | ||
| property D.Palette checkedOutsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(0, 0, 0, 0.1) | ||
| normalDark: Qt.rgba(0, 0, 0, 0.45) | ||
| } | ||
| radius: backgroundRect.radius |
There was a problem hiding this comment.
issue (bug_risk): Outside border palettes are defined on the border item but bound via ColorSelector, which likely has no such properties.
outsideBorderColor and checkedOutsideBorderColor are defined on D.OutsideBoxBorder, but the binding still points to D.ColorSelector.*. Unless ColorSelector defines these properties, the new palettes won’t be used. Please either add these properties to ColorSelector or bind color directly to the local D.OutsideBoxBorder properties.
| D.InsideBoxBorder { | ||
| property D.Palette insideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.1) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.05) | ||
| } | ||
| property D.Palette checkedInsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.15) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.08) | ||
| } | ||
| radius: backgroundRect.radius |
There was a problem hiding this comment.
issue (bug_risk): Same potential mismatch between local inside border palettes and ColorSelector references as in displaymode.
The palettes insideBorderColor / checkedInsideBorderColor are defined on D.InsideBoxBorder, but the color binding uses D.ColorSelector.*, matching the inconsistent pattern in displaymode/package/main.qml. Please either move these palettes into ColorSelector or bind directly to the local border properties to avoid the mismatch.
| D.OutsideBoxBorder { | ||
| property D.Palette outsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(0, 0, 0, 0.05) | ||
| normalDark: Qt.rgba(0, 0, 0, 0.4) | ||
| } | ||
| property D.Palette checkedOutsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(0, 0, 0, 0.1) | ||
| normalDark: Qt.rgba(0, 0, 0, 0.45) | ||
| } | ||
| radius: backgroundRect.radius |
There was a problem hiding this comment.
issue (bug_risk): Outside border color binding likely does not use the newly defined palettes on the border item.
Because outsideBorderColor and checkedOutsideBorderColor are defined on D.OutsideBoxBorder but the color binding uses D.ColorSelector.outsideBorderColor, the new palettes likely won’t apply unless ColorSelector forwards these properties. Please either bind color to the local properties or move the palette definitions into the ColorSelector configuration so they are actually used.
| D.OutsideBoxBorder { | ||
| property D.Palette outsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(0, 0, 0, 0.1) | ||
| normalDark: Qt.rgba(0, 0, 0, 0.7) | ||
| } | ||
| radius: root.windowRadius | ||
| anchors.fill: parent | ||
| z: D.DTK.AboveOrder | ||
| color: D.ColorSelector.outsideBorderColor |
There was a problem hiding this comment.
issue (bug_risk): OutsideBoxBorder defines its own outsideBorderColor palette but uses ColorSelector for the binding.
Here you declare a local outsideBorderColor palette, but color is still bound to D.ColorSelector.outsideBorderColor. If ColorSelector doesn’t expose this property, the color may be undefined and won’t use the local palette. To align with the inside border and the local definition, bind color to the local outsideBorderColor (or move this palette into ColorSelector if that’s the intended source).
| } | ||
| D.BoxShadow { | ||
| visible: !itemView.isCurrent | ||
| D.InsideBoxBorder { |
| color: D.ColorSelector.insideBorderColor | ||
| } | ||
|
|
||
| D.OutsideBoxBorder { |
There was a problem hiding this comment.
这个真的有效果么？这都超出窗口了吧，它应该显示不了吧，
There was a problem hiding this comment.
Pull request overview
This PR updates the OSD notification UI styling to replace shadow-based effects with inside/outside border components, aiming for clearer visual separation across light/dark themes and improved rendering consistency.
Changes:
- Replace
D.BoxShadow/D.BoxInsetShadowwithD.InsideBoxBorder/D.OutsideBoxBorderin display mode and window effect item delegates. - Add checked/normal border palettes for inside and outside borders, and tweak dark-mode checked background opacity (0.6 → 0.5).
- Add an
D.OutsideBoxBorderaround the main OSD window.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| panels/notification/osd/displaymode/package/main.qml | Replaces delegate shadows with inside/outside borders and updates checked background dark alpha. |
| panels/notification/osd/windoweffect/package/main.qml | Same border-based styling update for window effect delegates + checked background dark alpha tweak. |
| panels/notification/osd/package/main.qml | Adds an outside border around the OSD window container. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| D.InsideBoxBorder { | ||
| property D.Palette insideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.1) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.05) | ||
| } | ||
| property D.Palette checkedInsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.15) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.08) | ||
| } | ||
| radius: backgroundRect.radius | ||
| anchors.fill: parent | ||
| shadowOffsetX: 0 | ||
| shadowOffsetY: 1 | ||
| shadowColor: itemView.D.ColorSelector.dropShadowColor | ||
| shadowBlur: 1 | ||
| cornerRadius: backgroundRect.radius | ||
| spread: 0 | ||
| hollow: true | ||
| color: itemView.isCurrent ? D.ColorSelector.checkedInsideBorderColor | ||
| : D.ColorSelector.insideBorderColor | ||
| } | ||
| D.BoxInsetShadow { | ||
| visible: !itemView.isCurrent | ||
| D.OutsideBoxBorder { | ||
| property D.Palette outsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(0, 0, 0, 0.05) | ||
| normalDark: Qt.rgba(0, 0, 0, 0.4) | ||
| } | ||
| property D.Palette checkedOutsideBorderColor: D.Palette { |
There was a problem hiding this comment.
The border palette values are defined inline inside the border components, which duplicates styling data and creates extra Palette objects per delegate instance. Consider promoting these palettes to itemView (alongside backgroundColor / checkedBackgroundColor) or extracting a small shared delegate-background component so the Rectangle and borders bind to one source of truth and stay consistent across OSD panels.
| D.InsideBoxBorder { | ||
| property D.Palette insideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.1) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.05) | ||
| } | ||
| property D.Palette checkedInsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(1, 1, 1, 0.15) | ||
| normalDark: Qt.rgba(1, 1, 1, 0.08) | ||
| } | ||
| radius: backgroundRect.radius | ||
| anchors.fill: parent | ||
| shadowOffsetX: 0 | ||
| shadowOffsetY: 1 | ||
| shadowColor: itemView.D.ColorSelector.dropShadowColor | ||
| shadowBlur: 1 | ||
| cornerRadius: backgroundRect.radius | ||
| spread: 0 | ||
| hollow: true | ||
| color: itemView.isCurrent ? D.ColorSelector.checkedInsideBorderColor | ||
| : D.ColorSelector.insideBorderColor | ||
| } | ||
| D.BoxInsetShadow { | ||
| visible: !itemView.isCurrent | ||
| D.OutsideBoxBorder { | ||
| property D.Palette outsideBorderColor: D.Palette { | ||
| normal: Qt.rgba(0, 0, 0, 0.05) | ||
| normalDark: Qt.rgba(0, 0, 0, 0.4) | ||
| } | ||
| property D.Palette checkedOutsideBorderColor: D.Palette { |
There was a problem hiding this comment.
The inside/outside border palettes are declared inside the border items, which makes the delegate styling harder to maintain and repeats Palette object creation for every row. Prefer defining the border palettes on itemView (or using a shared delegate-background component) and binding the border color to itemView.D.ColorSelector.* so all state colors live in one place.
1. Replaced D.BoxShadow and D.BoxInsetShadow components with D.InsideBoxBorder and D.OutsideBoxBorder in display mode and window effect panels 2. Added new color palette properties for inside and outside borders with checked and normal states 3. Adjusted background color darkness in dark mode from 0.6 to 0.5 for better contrast 4. Added D.OutsideBoxBorder component to main OSD window 5. Removed obsolete dropShadowColor and innerShadowColor palette properties This change transitions from shadow-based visual effects to border-based effects for clearer definition of UI elements while maintaining visual hierarchy. Border effects provide better visibility across different themes and improve rendering performance. Log: Updated OSD notification visual style with borders replacing shadows for better clarity and theme consistency Influence: 1. Test OSD notifications in both light and dark modes to verify border visibility 2. Verify selected and unselected states show proper border colors 3. Check display mode switching and ensure visual consistency 4. Test window effects and notification popups for proper border rendering 5. Verify that the changes don't affect touch interactions or responsiveness 6. Confirm that the visual hierarchy remains clear with the new border effects PMS: BUG-311255
deepin pr auto review这段代码的 diff 展示了两个 QML 文件（ 以下是对这段 diff 的详细审查意见，分为语法逻辑、代码质量、代码性能和代码安全四个方面： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进建议
|
4 participants
This change transitions from shadow-based visual effects to border-based effects for clearer definition of UI elements while maintaining visual hierarchy. Border effects provide better visibility across different themes and improve rendering performance.
Log: Updated OSD notification visual style with borders replacing shadows for better clarity and theme consistency
Influence:
PMS: BUG-311255
Summary by Sourcery
Update OSD notification visuals to use border-based styling instead of shadow-based effects for clearer, more consistent appearance across themes.
New Features:
Bug Fixes:
Enhancements: