Skip to content

Conversation

@zccrs
Copy link
Member

@zccrs zccrs commented Mar 6, 2025

Summary by Sourcery

Enhancements:

  • Refactor SettingsOption to use QObject::property and QObject::setProperty instead of DConfigWrapper::value and DConfigWrapper::setValue.

@zccrs zccrs requested a review from 18202781743 March 6, 2025 09:45
@sourcery-ai
Copy link

sourcery-ai bot commented Mar 6, 2025

Reviewer's Guide by Sourcery

This pull request modifies the SettingsContainer, SettingsGroup, and SettingsOption classes to support using a QObject instead of DConfigWrapper for the config object. It also updates the value handling in SettingsOption to use setProperty and property instead of setValue and value on the config object. Finally, it modifies DConfigWrapper to emit the new value in the valueChanged signal.

Sequence diagram for SettingsOption value reset

sequenceDiagram
  participant User
  participant SettingsOption
  participant ConfigObject

  User->>SettingsOption: resetValue()
  activate SettingsOption
  SettingsOption->>ConfigObject: p.reset(m_config)
  activate ConfigObject
  ConfigObject-->>SettingsOption: 
  deactivate ConfigObject
  SettingsOption-->>User: 
  deactivate SettingsOption
Loading

Updated class diagram for SettingsGroup

classDiagram
  class SettingsGroup {
    -QString m_name
    -QString m_title
    -bool m_visible
    -SettingsGroup* m_parentGroup
    -int m_index
    -QQmlListProperty~DTK_QUICK_NAMESPACE::SettingsGroup~ m_children
    -QQmlComponent* m_background
    -QObject* m_config
    +QString name() const
    +void setName(QString name)
    +QString title() const
    +void setTitle(QString title)
    +bool visible() const
    +void setVisible(bool visible)
    +QQmlListProperty~DTK_QUICK_NAMESPACE::SettingsGroup~ children()
    +QQmlComponent* background() const
    +void setBackground(QQmlComponent* background)
    +void setConfig(QObject* config)
    +SettingsGroup* parentGroup() const
    +void setParentGroup(SettingsGroup* parentGroup)
    +int index() const
    +void setIndex(int index)
  }
Loading

File-Level Changes

Change Details Files
Modified SettingsOption to use QObject instead of DConfigWrapper for the config object, and updated value handling.
  • Changed m_config type from DConfigWrapper* to QObject*.
  • Modified setConfig to accept a QObject*.
  • Replaced m_config->value(m_key) with m_config->property(m_key.toLocal8Bit()) to retrieve the value.
  • Added a signal connection to valueChanged(QString, QVariant) to update the value when the config changes.
  • Implemented onValueChanged slot to handle value changes from the config object.
  • Modified setValue to use setProperty instead of setValue on the config object.
  • Implemented resetValue to reset the property on the config object.
src/private/dsettingscontainer.cpp
src/private/dsettingscontainer_p.h
Modified SettingsGroup to use QObject instead of DConfigWrapper for the config object.
  • Changed setConfig to accept a QObject*.
src/private/dsettingscontainer.cpp
src/private/dsettingscontainer_p.h
Modified SettingsContainer to use QObject instead of DConfigWrapper for the config object.
  • Changed m_config type from DConfigWrapper* to QObject*.
  • Modified config and setConfig to use QObject*.
src/private/dsettingscontainer.cpp
src/private/dsettingscontainer_p.h
Modified DConfigWrapper to emit the new value in the valueChanged signal.
  • Modified DConfigWrapper::initializeProperties to emit the new value in the valueChanged signal.
src/private/dconfigwrapper.cpp
src/private/dconfigwrapper_p.h
Added logging category for settings.
  • Added logging category for settings.
src/private/dsettingscontainer.cpp

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!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

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 added a commit to linuxdeepin/dtk6declarative that referenced this pull request Mar 6, 2025
Synchronize source files from linuxdeepin/dtkdeclarative.

Source-pull-request: linuxdeepin/dtkdeclarative#474
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 @zccrs - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider renaming onConfigValueChanged to onConfigPropertyChanged for clarity, as it's triggered by property changes, not just value changes.
  • It looks like you're replacing DConfigWrapper with QObject - make sure this is correct and that you're not losing any functionality.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

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 added a commit to linuxdeepin/dtk6declarative that referenced this pull request Mar 6, 2025
Synchronize source files from linuxdeepin/dtkdeclarative.

Source-pull-request: linuxdeepin/dtkdeclarative#474
@zccrs zccrs changed the title WIP: Supports dconfig2cpp generate class in SettingsDialog Supports dconfig2cpp generate class in SettingsDialog Mar 6, 2025
@zccrs
Copy link
Member Author

zccrs commented Mar 6, 2025

功能我用example测试了

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Mar 20, 2025

TAG Bot

New tag: 5.7.13
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #476

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Apr 17, 2025

TAG Bot

New tag: 5.7.14
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #480

…by dconfig2cpp

In the implementation related to SettingsDialog, it no longer forcibly
depends on DConfigWrapper, but instead uses QObject as the base type,
and obtains properties, signals, and other information through its
property metatype system, allowing support for both classes generated
by dconfig2cpp and the DConfigWrapper class.

feat: 改进 SettingsDialog 的设计以支持dconfig2cpp生成的代码

在SettingsDialog相关的实现中不再强制依赖DConfigWrapper,而是使用QObject
这个基础类型,通过它的property这种元对象系统获取属性、信号等信息,这样
就可以同时支持dconfig2cpp生成的类,以及DConfigWrapper这个类。
deepin-ci-robot added a commit to linuxdeepin/dtk6declarative that referenced this pull request Apr 21, 2025
Synchronize source files from linuxdeepin/dtkdeclarative.

Source-pull-request: linuxdeepin/dtkdeclarative#474
@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

关键摘要:

  • SettingsDialog.qml文件中,config属性的类型从D.Config更改为QtObject,需要确认这一更改是否与整个项目的其他部分兼容。
  • dconfigwrapper.cpp文件中,callInGuiThread函数的lambda表达式增加了currentValue参数,但未在函数体内使用。如果不需要,应移除以简化代码。
  • dconfigwrapper.cpp文件中,wrapper->mo->setValue调用时使用了key.toLocal8Bit(),这可能会导致在非UTF-8编码的系统上出现问题。建议检查并确保键值是UTF-8编码的。
  • dsettingscontainer.cpp文件中,SettingsOption::setConfig函数中增加了对m_config的空指针检查,这是一个好的做法,可以避免潜在的空指针解引用问题。
  • dsettingscontainer.cpp文件中,SettingsOption::onValueChanged函数中增加了对value.isValid()的检查,这是一个好的做法,可以避免无效值被处理。
  • dsettingscontainer.cpp文件中,SettingsOption::resetValue函数中增加了对属性是否可重置的检查,这是一个好的做法,可以避免不必要的错误处理。
  • dsettingscontainer.cpp文件中,SettingsOption::resetValue函数中增加了对重置失败的日志记录,这有助于调试和问题追踪。
  • dsettingscontainer.cpp文件中,SettingsGroup::setConfig函数中增加了对子组的配置设置,这是一个好的做法,可以确保所有子组都正确配置。
  • dsettingscontainer_p.hdsettingscontainer.cpp文件中,config属性的类型从DConfigWrapper更改为QObject,需要确认这一更改是否与整个项目的其他部分兼容。

是否建议立即修改:

  • 确认类型更改是否兼容,并确保所有相关部分都已更新。
  • 移除不必要的参数currentValue以简化代码。
  • 确保键值是UTF-8编码的,以避免潜在的问题。
  • 增加对空指针和无效值的检查,以增强代码的健壮性。
  • 增加日志记录,以帮助调试和问题追踪。
  • 确保所有子组都正确配置,以保持设置的一致性。

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, zccrs

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

@18202781743 18202781743 merged commit 587e563 into linuxdeepin:master Apr 25, 2025
19 of 21 checks passed
18202781743 pushed a commit to linuxdeepin/dtk6declarative that referenced this pull request Apr 25, 2025
Synchronize source files from linuxdeepin/dtkdeclarative.

Source-pull-request: linuxdeepin/dtkdeclarative#474
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