Skip to content

Conversation

@robertkill
Copy link
Contributor

@robertkill robertkill commented Oct 28, 2025

Fixed MTU spinbox locale issue that caused incorrect number formatting with thousand separators. Changed from custom text formatter to using C locale which doesn't include number grouping. Added displayTextChanged handler to properly detect when MTU value changes and trigger edit state. This ensures consistent number display and reliable change detection.

Log: Fixed MTU input field formatting and change detection

Influence:

  1. Test MTU input field displays numbers without thousand separators
  2. Verify MTU value changes are properly detected when editing
  3. Check that edit state is triggered correctly on MTU modifications
  4. Test with various MTU values including edge cases (min/max values)
  5. Verify console warnings appear when MTU values are changed

fix: 修复MTU输入格式化和变更检测问题

修复了MTU微调框的区域设置问题,该问题导致数字格式错误显示千位分隔
符。从自定义文本格式化器改为使用不包含数字分组的C区域设置。添加了
displayTextChanged处理程序以正确检测MTU值变化并触发编辑状态。这确保了数
字显示的一致性和变更检测的可靠性。

Log: 修复MTU输入字段格式化和变更检测问题

Influence:

  1. 测试MTU输入字段显示数字时不带千位分隔符
  2. 验证编辑时MTU值变化能被正确检测
  3. 检查MTU修改时编辑状态是否正确触发
  4. 测试各种MTU值,包括边界情况(最小/最大值)
  5. 验证MTU值更改时控制台警告是否出现

PMS: BUG-338289

Summary by Sourcery

Ensure consistent MTU input formatting without grouping and add reliable change detection via displayTextChanged handler

Bug Fixes:

  • Force C locale on the MTU spinbox to remove thousand separators instead of using a custom formatter
  • Add displayTextChanged handler to reliably detect MTU value edits, trigger edit state, and log warnings

Fixed MTU spinbox locale issue that caused incorrect number formatting
with thousand separators. Changed from custom text formatter to using C
locale which doesn't include number grouping. Added displayTextChanged
handler to properly detect when MTU value changes and trigger edit
state. This ensures consistent number display and reliable change
detection.

Log: Fixed MTU input field formatting and change detection

Influence:
1. Test MTU input field displays numbers without thousand separators
2. Verify MTU value changes are properly detected when editing
3. Check that edit state is triggered correctly on MTU modifications
4. Test with various MTU values including edge cases (min/max values)
5. Verify console warnings appear when MTU values are changed

fix: 修复MTU输入格式化和变更检测问题

修复了MTU微调框的区域设置问题,该问题导致数字格式错误显示千位分隔
符。从自定义文本格式化器改为使用不包含数字分组的C区域设置。添加了
displayTextChanged处理程序以正确检测MTU值变化并触发编辑状态。这确保了数
字显示的一致性和变更检测的可靠性。

Log: 修复MTU输入字段格式化和变更检测问题

Influence:
1. 测试MTU输入字段显示数字时不带千位分隔符
2. 验证编辑时MTU值变化能被正确检测
3. 检查MTU修改时编辑状态是否正确触发
4. 测试各种MTU值,包括边界情况(最小/最大值)
5. 验证MTU值更改时控制台警告是否出现

PMS: BUG-338289
@sourcery-ai
Copy link

sourcery-ai bot commented Oct 28, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Replaced custom MTU spinbox formatter with the C locale to eliminate thousand separators and added a displayTextChanged handler to detect MTU edits and trigger the component’s edit state reliably.

Sequence diagram for MTU value change detection and edit state triggering

sequenceDiagram
participant User as actor User
participant "MTU SpinBox"
participant "Root Component"
User->>"MTU SpinBox": Edit MTU value
"MTU SpinBox"->>"MTU SpinBox": displayTextChanged event
"MTU SpinBox"->>"Root Component": Call editClicked() if value changed
"Root Component"->>"Root Component": Enter edit state
"MTU SpinBox"->>"Root Component": Log warning if MTU value changed
Loading

File-Level Changes

Change Details Files
Switched MTU input formatting to use C locale
  • Removed custom textFromValue formatter
  • Added locale: Qt.locale("C") to the spinbox
dcc-network/qml/SectionDevice.qml
Added displayTextChanged handler for change detection
  • Implemented onDisplayTextChanged callback
  • Compared new value and displayText (stripped of commas) against config.mtu
  • Logged changes via console.warn and invoked root.editClicked()
dcc-network/qml/SectionDevice.qml

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link

deepin pr auto review

我来对这个代码变更进行审查:

  1. 代码逻辑分析:
  • 原代码使用 textFromValue 函数来格式化显示文本,禁用千位分隔符
  • 新代码改为使用 Qt.locale("C") 来强制使用 C locale,同样能达到不显示千位分隔符的效果
  • 新增了 onDisplayTextChanged 处理器来检测显示文本的变化
  1. 代码质量评价:
    优点:
  • 使用 locale 设置比自定义 textFromValue 函数更简洁
  • 添加了更严格的值变化检测

潜在问题:

  • onDisplayTextChanged 中的条件判断较为复杂,使用了正则表达式和多重条件判断
  • 存在重复的值变化检测逻辑(onValueChanged 和 onDisplayTextChanged)
  1. 性能影响:
  • 正则表达式 replace(/,/g, "") 在每次显示文本变化时都会执行,可能带来轻微性能开销
  • 双重检测机制可能导致不必要的重复处理
  1. 安全性考虑:
  • 代码没有明显安全隐患

改进建议:

  1. 简化值变化检测逻辑:
onDisplayTextChanged: {
    if (hasMTU) {
        const cleanDisplayText = displayText.replace(/,/g, "")
        if (root.config.mtu !== value || cleanDisplayText !== value.toString()) {
            console.warn("MTU value changed: config.mtu=", root.config.mtu, "value=", value, "displayText=", displayText)
            root.editClicked()
        }
    }
}
  1. 考虑合并值变化检测逻辑:
onValueChanged: {
    if (hasMTU) {
        if (!root.config.hasOwnProperty("mtu") || root.config.mtu !== value) {
            root.config.mtu = value
            root.editClicked()
        }
    }
}
  1. 或者使用更优雅的方式:
property bool mtuChanged: false

onDisplayTextChanged: {
    if (hasMTU) {
        const cleanDisplayText = displayText.replace(/,/g, "")
        mtuChanged = (root.config.mtu !== value || cleanDisplayText !== value.toString())
        if (mtuChanged) {
            console.warn("MTU value changed: config.mtu=", root.config.mtu, "value=", value, "displayText=", displayText)
        }
    }
}

onValueChanged: {
    if (hasMTU && mtuChanged) {
        root.config.mtu = value
        root.editClicked()
        mtuChanged = false
    }
}
  1. 建议添加输入验证:
validator: IntValidator {
    bottom: 68
    top: 9000
}

这些改进可以:

  • 提高代码可读性
  • 减少重复逻辑
  • 提供更好的用户体验
  • 确保数据的有效性

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caixr23, robertkill

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@robertkill robertkill merged commit 5435edd into linuxdeepin:master Oct 28, 2025
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants