-
Notifications
You must be signed in to change notification settings - Fork 106
fix: add wheel speed value validation in setWheelSpeed #1002
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
Added range validation for wheel speed configuration values to ensure they stay within the valid range of 1-100. Previously, the code only had a comment indicating the valid range but no actual validation. Now if the configured speed is below 1, it will be set to 1, and if above 100, it will be set to 100. This prevents invalid speed values from being used and ensures consistent behavior. Log: Fixed wheel speed configuration validation to enforce 1-100 range Influence: 1. Test setting wheel speed to values below 1 (should clamp to 1) 2. Test setting wheel speed to values above 100 (should clamp to 100) 3. Test setting wheel speed to values within 1-100 range (should use exact value) 4. Verify that imwheel process management still works correctly after validation 5. Check that debug logging continues to function properly fix: 在setWheelSpeed中添加滚轮速度值合法性校验 为滚轮速度配置值添加范围验证,确保其保持在1-100的有效范围内。之前代码仅 有注释说明有效范围但无实际验证。现在如果配置速度低于1将被设为1,高于100 将被设为100。这防止使用无效速度值并确保行为一致。 Log: 修复滚轮速度配置验证以强制执行1-100范围 PMS: BUG-348133 Influence: 1. 测试将滚轮速度设置为低于1的值(应限制为1) 2. 测试将滚轮速度设置为高于100的值(应限制为100) 3. 测试将滚轮速度设置为1-100范围内的值(应使用精确值) 4. 验证imwheel进程管理在验证后仍正常工作 5. 检查调试日志记录是否继续正常运行
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds runtime validation and clamping of the configured wheel speed to the documented valid range [1,100] before applying it, ensuring invalid configuration values are corrected instead of passed through. Flow diagram for wheel speed validation in setWheelSpeedflowchart TD
A[setWheelSpeed called] --> B[Read speed from WheelSpeed.Get]
B --> C{Is speed < 1?}
C -- Yes --> D[Set speed = 1]
C -- No --> E{Is speed > 100?}
E -- Yes --> F[Set speed = 100]
E -- No --> G[Keep original speed]
D --> H[logger.Debug setWheelSpeed speed]
F --> H
G --> H
H --> I[Proceed with imwheel process management]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review代码审查意见1. 语法与逻辑审查
2. 代码质量与风格
3. 性能
4. 安全性
改进建议代码以下是结合上述意见改进后的代码版本: const (
minWheelSpeed uint32 = 1
maxWheelSpeed uint32 = 100
)
func (m *Manager) setWheelSpeed() {
speed := m.WheelSpeed.Get()
// Ensure speed is within the valid range [1, 100]
if speed < minWheelSpeed {
speed = minWheelSpeed
} else if speed > maxWheelSpeed {
speed = maxWheelSpeed
}
logger.Debug("setWheelSpeed", speed)
// 为了避免imwheel对kwin影响,先杀死imwheel
// ... (后续代码)
}或者,如果使用 Go 1.21 或更高版本,可以使用 import "cmp" // Go 1.21+
// ...
func (m *Manager) setWheelSpeed() {
speed := m.WheelSpeed.Get()
// Clamp speed to range [1, 100]
speed = cmp.Or(cmp.Or(speed > maxWheelSpeed, maxWheelSpeed), speed)
// 注意:cmp.Or 的逻辑是 "如果第一个参数为 true,返回第二个;否则返回第三个"
// 上面这行代码逻辑略显晦涩,更推荐下面的写法:
speed = max(minWheelSpeed, min(speed, maxWheelSpeed))
logger.Debug("setWheelSpeed", speed)
// ...
}最推荐的写法(兼顾兼容性和可读性): func (m *Manager) setWheelSpeed() {
const (
minSpeed = 1
maxSpeed = 100
)
speed := m.WheelSpeed.Get()
// speed range is [minSpeed, maxSpeed]
if speed < minSpeed {
speed = minSpeed
} else if speed > maxSpeed {
speed = maxSpeed
}
logger.Debug("setWheelSpeed", speed)
// 为了避免imwheel对kwin影响,先杀死imwheel
// ...
}总结:主要改进点在于将魔法数字提取为常量,并优化了类型转换,使代码更健壮、易读。 |
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:
- Since
speedis already auint32, consider definingminandmaxasconstuint32values (e.g.,const minSpeed uint32 = 1) to avoid repeated casting and make the range intent clearer. - If wheel speed clamping is or may become needed in other places, consider extracting this range enforcement into a small helper function (e.g.,
clampWheelSpeed) to centralize the logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `speed` is already a `uint32`, consider defining `min` and `max` as `const` `uint32` values (e.g., `const minSpeed uint32 = 1`) to avoid repeated casting and make the range intent clearer.
- If wheel speed clamping is or may become needed in other places, consider extracting this range enforcement into a small helper function (e.g., `clampWheelSpeed`) to centralize the logic.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: fly602, mhduiy 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 |
Added range validation for wheel speed configuration values to ensure they stay within the valid range of 1-100. Previously, the code only had a comment indicating the valid range but no actual validation. Now if the configured speed is below 1, it will be set to 1, and if above 100, it will be set to 100. This prevents invalid speed values from being used and ensures consistent behavior.
Log: Fixed wheel speed configuration validation to enforce 1-100 range
Influence:
fix: 在setWheelSpeed中添加滚轮速度值合法性校验
为滚轮速度配置值添加范围验证,确保其保持在1-100的有效范围内。之前代码仅
有注释说明有效范围但无实际验证。现在如果配置速度低于1将被设为1,高于100
将被设为100。这防止使用无效速度值并确保行为一致。
Log: 修复滚轮速度配置验证以强制执行1-100范围
PMS: BUG-348133
Influence:
Summary by Sourcery
Bug Fixes: