Skip to content

Conversation

@fly602
Copy link
Contributor

@fly602 fly602 commented Oct 31, 2025

  1. Fix an audio crash issue
  2. Remove Firefox DPI settings

Influence: audio/firefox

@sourcery-ai
Copy link

sourcery-ai bot commented Oct 31, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

This PR fully removes Firefox DPI scaling support (including its tests) to eliminate related audio crashes, and strengthens updateDefaultSink by moving the sinkInfo nil check earlier to avoid null pointer dereferences.

Class diagram for removed Firefox DPI scaling code in XSManager

classDiagram
class XSManager {
  +updateXResources()
  -updateFirefoxDPI()  // Removed
}
%% The following helper functions were also removed:
class getFirefoxConfigs {
  -getFirefoxConfigs(dir string) ([]string, error)  // Removed
}
class setFirefoxDPI {
  -setFirefoxDPI(value float64, src, dest string) error  // Removed
}
XSManager --|> getFirefoxConfigs : used
XSManager --|> setFirefoxDPI : used
Loading

Class diagram for updated Audio.updateDefaultSink() method

classDiagram
class Audio {
  +updateDefaultSink(sinkName string)
  -defaultSinkName : string
  -defaultSink
  +setPropDefaultSink(path string)
  +moveSinkInputsToSink(sink)
}
%% updateDefaultSink now checks sinkInfo == nil earlier to prevent null pointer dereference.
Loading

File-Level Changes

Change Details Files
Removed Firefox DPI scaling feature and related code
  • Deleted updateFirefoxDPI and its helper functions (getFirefoxConfigs, setFirefoxDPI)
  • Removed DPI-related imports and variables from xsettings dpi module
  • Stripped out Firefox DPI test cases and helpers
  • Eliminated go m.updateFirefoxDPI() invocation in startup
xsettings1/xsettings_dpi.go
xsettings1/xsettings.go
xsettings1/xsettings_test.go
Enhanced default sink update by reordering sinkInfo nil validation
  • Inserted early nil check for sinkInfo with logging and fallback
  • Removed duplicate nil check block later in updateDefaultSink
audio1/audio.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

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 - here's some feedback:

  • Remove any leftover imports or constants in xsettings_dpi.go that are no longer used after deleting the Firefox DPI code to avoid dead code.
  • Add a unit test or integration scenario for the new early-return path in updateDefaultSink to verify nil sinkInfo handling works as expected.
  • Consider renaming or updating comments around updateDPI/updateXResources to reflect that Firefox DPI scaling has been removed, improving code clarity.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Remove any leftover imports or constants in xsettings_dpi.go that are no longer used after deleting the Firefox DPI code to avoid dead code.
- Add a unit test or integration scenario for the new early-return path in updateDefaultSink to verify nil sinkInfo handling works as expected.
- Consider renaming or updating comments around updateDPI/updateXResources to reflect that Firefox DPI scaling has been removed, improving code clarity.

## Individual Comments

### Comment 1
<location> `audio1/audio.go:1396-1401` </location>
<code_context>
 		logger.Info("virtual sink is", sinkName, ",sink_master is", sinkInfo.Name)
 	}
-
+	if sinkInfo == nil {
+		logger.Warning("failed to get sinkInfo for name:", sinkName)
+		a.setPropDefaultSink("/")
+		a.defaultSinkName = ""
+		a.defaultSink = nil
+		return
+	}
 	a.moveSinkInputsToSink(nil)
</code_context>

<issue_to_address>
**suggestion:** Duplicate nil check for sinkInfo can be removed.

Since the nil check is now handled earlier, please remove the redundant check and any unreachable code below to improve readability.
</issue_to_address>

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

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

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

1. Fix an audio crash issue
2. Remove Firefox DPI settings

Influence: audio/firefox
@deepin-ci-robot
Copy link

deepin pr auto review

我来对这段代码进行审查分析:

  1. audio.go 的修改:
if sinkInfo == nil {
    logger.Warning("failed to get sinkInfo for name:", sinkName)
    a.setPropDefaultSink("/")
    a.defaultSinkName = ""
    a.defaultSink = nil
    return
}

优点:

  • 将空指针检查提前,避免了后续可能的空指针访问
  • 增加了错误日志记录,便于问题追踪
  • 在出错时正确清理了状态

建议:

  • 可以考虑在日志中记录更多上下文信息,比如当前默认sink的名称
  1. xsettings.go 的修改:
- go m.updateFirefoxDPI()

优点:

  • 移除了Firefox DPI的异步更新,可能是因为这个功能不再需要或者有其他实现方式
  • 减少了不必要的goroutine使用
  1. xsettings_dpi.go 的修改:
    删除了大量与Firefox DPI相关的代码,包括:
  • updateFirefoxDPI函数
  • getFirefoxConfigs函数
  • setFirefoxDPI函数
  • 相关常量和变量

优点:

  • 移除了不再需要的功能代码,减少了维护负担
  • 删除了相关的测试代码,保持代码库的一致性
  1. xsettings_test.go 的修改:
    删除了与Firefox DPI相关的测试用例。

优点:

  • 与功能删除保持一致,删除了不再需要的测试代码

总体评价:

  1. 代码质量改进:

    • audio.go中的空指针检查提前是一个好的实践
    • 删除不再使用的代码有助于保持代码库的整洁
  2. 安全性改进:

    • audio.go中的修改增加了对空指针的防护
  3. 性能影响:

    • 移除Firefox DPI相关代码可能会略微减少启动时间和内存使用
    • audio.go中的修改不会对性能产生负面影响
  4. 维护性:

    • 代码删除减少了维护负担
    • 错误处理更加清晰

建议:

  1. 在audio.go中,可以考虑添加更多的错误上下文信息
  2. 对于删除的Firefox DPI功能,建议在代码注释或文档中记录删除原因
  3. 如果Firefox DPI功能被完全移除,建议检查是否有其他地方引用了这些被删除的代码

这些修改整体上是积极的,提高了代码的质量和可维护性。

@fly602 fly602 changed the title fix: remove Firefox DPI scaling to resolve audio crash fix: code optimization Nov 3, 2025
@fly602 fly602 merged commit 25905b7 into linuxdeepin:master Nov 3, 2025
14 of 17 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