Skip to content

Conversation

@mhduiy
Copy link
Contributor

@mhduiy mhduiy commented Sep 11, 2025

  1. Replaced gio.Settings with dconfig (ConfigManager) for reading sound settings
  2. Added dconfig manager creation function with proper error handling
  3. Updated schema IDs and key names to match dconfig structure
  4. Added logging support for better debugging
  5. Maintained backward compatibility with default values when dconfig is unavailable

feat: 从 gsettings 迁移到 dconfig 用于声音设置

  1. 使用 dconfig (ConfigManager) 替代 gio.Settings 读取声音设置
  2. 添加了 dconfig 管理器创建函数并包含错误处理
  3. 更新了 schema ID 和键名以匹配 dconfig 结构
  4. 添加了日志支持以便更好地调试
  5. 在 dconfig 不可用时保持向后兼容性并提供默认值

pms: TASK-381277

Summary by Sourcery

Migrate sound settings backend from GSettings to dconfig and improve error handling and debugging

New Features:

  • Use dconfig ConfigManager instead of gio.Settings for reading sound settings
  • Add logging support in soundplayer for better debugging

Enhancements:

  • Introduce makeDConfigManager helper with robust error handling
  • Update schema identifiers and key names to match dconfig conventions
  • Fall back to default sound theme and settings when dconfig is unavailable

1. Replaced gio.Settings with dconfig (ConfigManager) for reading sound
settings
2. Added dconfig manager creation function with proper error handling
3. Updated schema IDs and key names to match dconfig structure
4. Added logging support for better debugging
5. Maintained backward compatibility with default values when dconfig
is unavailable

feat: 从 gsettings 迁移到 dconfig 用于声音设置

1. 使用 dconfig (ConfigManager) 替代 gio.Settings 读取声音设置
2. 添加了 dconfig 管理器创建函数并包含错误处理
3. 更新了 schema ID 和键名以匹配 dconfig 结构
4. 添加了日志支持以便更好地调试
5. 在 dconfig 不可用时保持向后兼容性并提供默认值

pms: TASK-381277
@sourcery-ai
Copy link

sourcery-ai bot commented Sep 11, 2025

Reviewer's Guide

This PR replaces gio.Settings-based sound settings retrieval with dconfig (ConfigManager), introducing a new factory function with error handling, updating schema IDs and keys, adding logging, and maintaining backward compatibility with default values.

Sequence diagram for sound settings retrieval using dconfig

sequenceDiagram
    participant SP as soundplayer
    participant Log as logger
    participant DBus as dbus.SystemBus
    participant CM as configManager
    participant DCM as dconfig.Manager

    SP->>DBus: Get system bus
    DBus-->>SP: bus
    SP->>CM: NewConfigManager(bus)
    CM-->>SP: dsMgr
    SP->>CM: AcquireManager(appID, id)
    CM-->>SP: dsPath
    SP->>CM: NewManager(bus, dsPath)
    CM-->>SP: dconfigMgr
    SP->>DCM: Value(0, key)
    DCM-->>SP: value
    alt error
        SP->>Log: logger.Warning(err)
        SP-->>SP: Use default value
    end
Loading

Class diagram for migration from gio.Settings to dconfig (ConfigManager)

classDiagram
    class soundplayer {
        +PlaySystemSound(event string, device string) error
        +PlayThemeSound(theme string, event string, device string) error
        +CanPlayEvent(event string) bool
        +GetSoundTheme() string
        +GetThemeSoundFile(theme string, event string) string
        +makeDConfigManager(appID string, id string) (configManager.Manager, error)
        -logger
    }
    class gio {
        +NewSettings(schema string) Settings
    }
    class configManager {
        +NewConfigManager(bus Bus) ConfigManager
        +NewManager(bus Bus, path string) Manager
        +Manager
    }
    class log {
        +NewLogger(name string) Logger
        +Logger
    }
    soundplayer ..> configManager : uses
    soundplayer ..> log : uses
    soundplayer ..> gio : replaced by dconfig
Loading

File-Level Changes

Change Details Files
Migrate settings access from GSettings to DConfigManager
  • Removed gio.NewSettings calls and Unref usage
  • Replaced GetBoolean/GetString with configManager.Value and type assertions
  • Replaced settings.ListKeys with KeyList().Get and strv.Strv
soundutils/soundplayer.go
Introduce makeDConfigManager factory with error handling
  • Created makeDConfigManager to acquire and initialize dconfig manager
  • Added DBus SystemBus acquisition and nil checks
  • Included warnings for manager acquisition and instantiation failures
soundutils/soundplayer.go
Update schema IDs and key names for dconfig compatibility
  • Declared new dconfigDaemonAppId, dconfigSoundEffectId, dconfigAppearanceAppId, dconfigAppearanceId
  • Changed keySoundTheme to match dconfig naming conventions
soundutils/soundplayer.go
Add logging support for better debugging
  • Initialized a logger instance with log.NewLogger
  • Replaced silent errors with logger.Warning calls
soundutils/soundplayer.go
Maintain backward compatibility when dconfig is unavailable
  • Fall back to defaultSoundTheme on GetSoundTheme errors
  • Return false in CanPlayEvent when dconfig retrieval fails
soundutils/soundplayer.go

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. 错误处理不一致

    • CanPlayEvent函数中,当获取soundeffectDconfig失败时只记录警告并返回false,但在GetSoundTheme中同样情况下返回默认值,这种处理方式不一致。
    • 建议:统一错误处理策略,或者至少在文档中说明为什么某些情况下返回false而其他情况下返回默认值。
  2. 变量命名问题

    • appeearanceDconfig变量名有拼写错误(应为"appearance")。
    • 建议:修正拼写错误,保持命名一致性。
  3. 魔法数字

    • makeDConfigManager函数中多次使用0作为参数,但没有明确含义。
    • 建议:使用命名常量代替这些魔法数字。

代码质量方面

  1. 资源管理

    • 代码中移除了原来的settings.Unref()调用,但没有明确说明新方案中如何管理资源。
    • 建议:明确说明资源管理方式,或者添加注释说明为什么不需要显式释放资源。
  2. 注释不足

    • makeDConfigManager函数只有简单的TODO注释,缺少对函数功能、参数和返回值的详细说明。
    • 建议:添加完整的函数文档注释。
  3. 常量定义

    • 新增的常量定义分散在代码中,没有统一组织。
    • 建议:将所有常量定义放在文件开头,并按功能分组。

性能方面

  1. 重复连接

    • 每次调用makeDConfigManager都会创建新的DBus连接,可能导致性能问题。
    • 建议:考虑使用单例模式或缓存机制管理DBus连接。
  2. 频繁查询

    • CanPlayEvent函数可能被频繁调用,每次都进行DBus查询可能影响性能。
    • 建议:添加缓存机制,缓存查询结果。

安全性方面

  1. 类型断言风险

    • 代码中使用.Value().(bool).Value().(string)进行类型断言,如果实际类型不匹配会导致运行时panic。
    • 建议:添加类型检查,使用安全的类型转换方式。
  2. 错误信息泄露

    • 错误信息可能包含敏感的系统信息。
    • 建议:在记录日志时,过滤或脱敏敏感信息。

具体改进建议

  1. makeDConfigManager添加完整的文档注释:
// makeDConfigManager 创建并返回一个DConfig管理器实例
// 参数:
//   - appID: 应用程序ID
//   - id: 配置管理器ID
// 返回值:
//   - configManager.Manager: 配置管理器实例
//   - error: 错误信息,如果创建失败
  1. 添加类型安全检查:
func safeGetBoolValue(value dbus.Variant) (bool, error) {
    b, ok := value.Value().(bool)
    if !ok {
        return false, errors.New("value is not a boolean")
    }
    return b, nil
}
  1. 添加缓存机制:
var (
    soundThemeCache string
    soundThemeMutex sync.RWMutex
)

func GetSoundTheme() string {
    soundThemeMutex.RLock()
    if soundThemeCache != "" {
        soundThemeMutex.RUnlock()
        return soundThemeCache
    }
    soundThemeMutex.RUnlock()
    
    // ... 原有逻辑获取soundTheme ...
    
    soundThemeMutex.Lock()
    soundThemeCache = theme
    soundThemeMutex.Unlock()
    
    return theme
}
  1. 修正常量定义组织:
const (
    // DBus相关常量
    dconfigDaemonAppId     = "org.deepin.dde.daemon"
    dconfigSoundEffectId   = "org.deepin.dde.daemon.soundeffect"
    dconfigAppearanceAppId = "org.deepin.dde.appearance"
    dconfigAppearanceId    = dconfigAppearanceAppId
    
    // 配置键常量
    keySoundTheme     = "Sound_Theme"
    keyEnabled        = "enabled"
    keyPlayer         = "player"
    
    // 默认值常量
    defaultSoundTheme = "deepin"
)
  1. 添加连接池管理DBus连接:
var (
    busConn *dbus.Conn
    connMutex sync.Mutex
)

func getDBusConnection() (*dbus.Conn, error) {
    connMutex.Lock()
    defer connMutex.Unlock()
    
    if busConn != nil {
        return busConn, nil
    }
    
    var err error
    busConn, err = dbus.SystemBus()
    return busConn, err
}

这些改进将提高代码的健壮性、性能和可维护性,同时减少潜在的安全风险。

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: mhduiy, 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

@mhduiy mhduiy merged commit 824f8f3 into linuxdeepin:master Sep 11, 2025
16 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