fix: Incorrect positioning of error messages#3087
Open
JWWTSL wants to merge 2 commits intolinuxdeepin:masterfrom
Open
fix: Incorrect positioning of error messages#3087JWWTSL wants to merge 2 commits intolinuxdeepin:masterfrom
JWWTSL wants to merge 2 commits intolinuxdeepin:masterfrom
Conversation
log: D.AlertToolTip is of type Control (standard Item), not Popup, and is rendered within the visual tree of its parent container. The height of the page Item for customNTPServer is fixed at 40px; when AlertToolTip is displayed below the input field, it extends beyond this area and is obscured by the ‘System Time Zone’ component below (overridden by the z-order of the sibling DccObject). pms: bug-352961
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JWWTSL 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 guide (collapsed on small PRs)Reviewer's GuideAdjusts the custom NTP server row layout so that the D.AlertToolTip for the address field is positioned correctly and not clipped, and switches from the built‑in line edit alert text to an explicit tooltip control that is shown/hidden in sync with validation logic. Sequence diagram for custom NTP server validation and tooltip visibilitysequenceDiagram
actor User
participant AddrLineEdit as AddrLineEdit
participant AddrErrorToolTip as AddrErrorToolTip
participant EditButton as EditButton
User->>EditButton: click
EditButton->>AddrLineEdit: check text.length
alt text.length === 0
EditButton->>AddrLineEdit: addr.showAlert = true
EditButton->>AddrErrorToolTip: visible = true
else text.length > 0
EditButton->>AddrLineEdit: addr.showAlert = false
EditButton->>AddrErrorToolTip: visible = false
EditButton->>EditButton: proceed with ntpServerAddress update
end
User->>AddrLineEdit: type characters
AddrLineEdit->>AddrLineEdit: onTextChanged
alt text.length > 0
AddrLineEdit->>AddrLineEdit: addr.showAlert = false
AddrLineEdit->>AddrErrorToolTip: visible = false
end
Flow diagram for layout and tooltip-driven height of custom NTP server rowflowchart TD
A[item.implicitHeight evaluation] --> B{addrErrorTip.visible?}
B -- false --> C[implicitHeight = 40]
B -- true --> D[implicitHeight = 40 + addrErrorTip.height + 4]
subgraph AddressFieldLayout
E[AddrLineEdit anchors.top = parent.top]
F["AddrLineEdit topMargin = (40 - addr.height) / 2"]
G[EditButton anchors.verticalCenter = addr.verticalCenter]
E --> F --> G
end
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 1 issue, and left some high level feedback:
- The hard-coded constants
40and4in bothimplicitHeightandtopMargincouple layout to specific pixel values; consider deriving these from theaddr/parent heights or a shared constant to avoid layout breakage if the field size changes. - You now drive the tooltip via
addrErrorTip.visiblewhile still togglingaddr.showAlert; ifshowAlertis no longer functionally used byD.LineEdit, consider either wiring the tooltip visibility toshowAlertor removing the redundant state to keep the error handling logic in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded constants `40` and `4` in both `implicitHeight` and `topMargin` couple layout to specific pixel values; consider deriving these from the `addr`/parent heights or a shared constant to avoid layout breakage if the field size changes.
- You now drive the tooltip via `addrErrorTip.visible` while still toggling `addr.showAlert`; if `showAlert` is no longer functionally used by `D.LineEdit`, consider either wiring the tooltip visibility to `showAlert` or removing the redundant state to keep the error handling logic in one place.
## Individual Comments
### Comment 1
<location path="src/plugin-datetime/qml/TimeAndDate.qml" line_range="228" />
<code_context>
page: Item {
id: item
- implicitHeight: 40
+ implicitHeight: addrErrorTip.visible ? 40 + addrErrorTip.height + 4 : 40
implicitWidth: 300
+
</code_context>
<issue_to_address>
**suggestion:** Avoid repeating the `40` constant in layout calculations to reduce maintenance issues.
This height is now hardcoded in multiple places (e.g. `implicitHeight` and `topMargin: (40 - addr.height) / 2`), so changing the base row height requires updating each occurrence. Define a local property (e.g. `property int rowHeight: 40`) on the `Item` and use it in all related calculations instead.
Suggested implementation:
```
pageType: DccObject.Editor
page: Item {
id: item
property int rowHeight: 40
implicitHeight: addrErrorTip.visible ? rowHeight + addrErrorTip.height + 4 : rowHeight
implicitWidth: 300
```
Search within the same `Item` (and related layout code for this row) for other hardcoded `40` usages, such as `topMargin: (40 - addr.height) / 2`, and replace them with expressions that use `rowHeight` instead, e.g. `topMargin: (rowHeight - addr.height) / 2`. This ensures all layout calculations stay consistent when `rowHeight` is changed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| page: Item { | ||
| id: item | ||
| implicitHeight: 40 | ||
| implicitHeight: addrErrorTip.visible ? 40 + addrErrorTip.height + 4 : 40 |
There was a problem hiding this comment.
suggestion: Avoid repeating the 40 constant in layout calculations to reduce maintenance issues.
This height is now hardcoded in multiple places (e.g. implicitHeight and topMargin: (40 - addr.height) / 2), so changing the base row height requires updating each occurrence. Define a local property (e.g. property int rowHeight: 40) on the Item and use it in all related calculations instead.
Suggested implementation:
pageType: DccObject.Editor
page: Item {
id: item
property int rowHeight: 40
implicitHeight: addrErrorTip.visible ? rowHeight + addrErrorTip.height + 4 : rowHeight
implicitWidth: 300
Search within the same Item (and related layout code for this row) for other hardcoded 40 usages, such as topMargin: (40 - addr.height) / 2, and replace them with expressions that use rowHeight instead, e.g. topMargin: (rowHeight - addr.height) / 2. This ensures all layout calculations stay consistent when rowHeight is changed.
|
TAG Bot New tag: 6.1.77 |
log: In TimeAndDate.qml, instead of using the built-in showAlert method of D.LineEdit (whose AlertToolTip pops up from the bottom and gets obscured), a separate D.AlertToolTip is placed inside the addr element, with the y value set to a negative value so that it pops up from the top; this prevents it from being obscured by the system time zone displayed below. pms: bug-352961
deepin pr auto review这段代码的 diff 展示了对 1. 语法逻辑审查
2. 代码质量审查
改进后的代码示例： onClicked: {
if (addr.text.length === 0) {
// 先重置为 false 以确保 showAlert 属性能触发变化信号，
// 从而正确刷新 UI 状态（特别是处理连续点击时的动画重置）
addr.showAlert = false
addr.showAlert = true
return
}
// ... 其他逻辑
}3. 代码性能审查
4. 代码安全审查
总结这段代码修改是为了解决 UI 状态刷新的问题，逻辑正确且性能无碍。主要的改进建议是增加注释以说明代码意图，方便后续维护。 |
|
TAG Bot New tag: 6.1.78 |
|
TAG Bot New tag: 6.1.79 |
|
TAG Bot New tag: 6.1.80 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
log: D.AlertToolTip is of type Control (standard Item), not Popup, and is rendered within the visual tree of its parent container. The height of the page Item for customNTPServer is fixed at 40px; when AlertToolTip is displayed below the input field, it extends beyond this area and is obscured by the ‘System Time Zone’ component below (overridden by the z-order of the sibling DccObject).
pms: bug-352961
Summary by Sourcery
Bug Fixes: