fix(slider): adjust path calculation for handle alignment#616
fix(slider): adjust path calculation for handle alignment#61652cyb wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
When handleType is less than 0 and x-value more than 0, the x-coordinate calculation now includes the handle width offset to correct the visual alignment of the slider path. Log: adjust path calculation for handle alignment PMS: BUG-358623 Influence: 检查输入设备的反馈音量显示是否正确,同时关注静音场景(PMS:350837)
|
Hi @52cyb. Thanks for your PR. 😃 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the slider path's x-coordinate calculation so that, for horizontal sliders with a negative handleType and a positive handle x-position, the path end aligns with the visible handle edge by including the handle width offset. Flow diagram for slider path x coordinate calculationflowchart LR
A[control.horizontal?] -->|no| B[Use sliderGroove.width / 2 as x]
A -->|yes| C[Check control.handleType < 0]
C -->|no| D[Use control.handle.x as x]
C -->|yes| E[Check control.handle.x > 0]
E -->|no| D[Use control.handle.x as x]
E -->|yes| F[Use control.handle.x + control.handle.width as x]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @52cyb. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The nested ternary expression for the horizontal x-coordinate is getting hard to read; consider extracting the handle offset calculation into a small helper property or function so the condition on handleType and handle.x is clearer.
- The new condition
control.handleType < 0 && control.handle.x > 0appears to handle a specific alignment case; it would help future maintainers to clarify whyhandleType < 0andhandle.x > 0are the correct thresholds (e.g., by naming constants or using an enum instead of a raw comparison). - Please confirm whether this path adjustment should also apply in non-left-to-right or mirrored layouts; if so, you may need to account for layout direction rather than relying solely on
handle.x > 0.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The nested ternary expression for the horizontal x-coordinate is getting hard to read; consider extracting the handle offset calculation into a small helper property or function so the condition on handleType and handle.x is clearer.
- The new condition `control.handleType < 0 && control.handle.x > 0` appears to handle a specific alignment case; it would help future maintainers to clarify why `handleType < 0` and `handle.x > 0` are the correct thresholds (e.g., by naming constants or using an enum instead of a raw comparison).
- Please confirm whether this path adjustment should also apply in non-left-to-right or mirrored layouts; if so, you may need to account for layout direction rather than relying solely on `handle.x > 0`.
## Individual Comments
### Comment 1
<location path="qt6/src/qml/Slider.qml" line_range="103" />
<code_context>
startY: control.horizontal ? sliderGroove.height / 2 : sliderGroove.height
PathLine {
- x: control.horizontal ? control.handle.x : sliderGroove.width / 2
+ x: control.horizontal ? ((control.handleType < 0 && control.handle.x > 0) ? control.handle.x + control.handle.width : control.handle.x) : sliderGroove.width / 2
y: control.horizontal ? sliderGroove.height / 2 : control.handle.y + control.handle.height / 2
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider clarifying the `handleType < 0 && handle.x > 0` condition to avoid possible off-by-one/branching issues at x=0.
This logic creates a discontinuity: for `control.handleType < 0`, `x > 0` uses `handle.x + handle.width` while `x <= 0` uses `handle.x`. If the goal is to re-anchor from center to right edge for certain handle types, tying that change to `x > 0` may cause a jump when crossing zero. Consider basing this purely on `handleType` (or an explicit state) or making the threshold (`>= 0`, etc.) explicit so behavior is consistent around the origin.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 52cyb 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 |
| startY: control.horizontal ? sliderGroove.height / 2 : sliderGroove.height | ||
| PathLine { | ||
| x: control.horizontal ? control.handle.x : sliderGroove.width / 2 | ||
| x: control.horizontal ? ((control.handleType < 0 && control.handle.x > 0) ? control.handle.x + control.handle.width : control.handle.x) : sliderGroove.width / 2 |
There was a problem hiding this comment.
control.handleType < 0 这个不会成立吧,它是个枚举,
When handleType is less than 0 and x-value more than 0, the x-coordinate calculation now includes the handle width offset to correct the visual alignment of the slider path.
Log: adjust path calculation for handle alignment
PMS: BUG-358623
Influence: 检查输入设备的反馈音量显示是否正确,同时关注静音场景(PMS:350837)
Summary by Sourcery
Bug Fixes: