fix： Speaker Display Anomaly #3012
Merged
+81
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
log: The audio module's output device list/dropdown relies on SoundWorker::cardsChanged() for refreshes. However, in certain backend implementations, plugging/unplugging wired headphones primarily triggers either the DBus port enabled state change (PortEnabledChanged) or solely CardsChanged events. Previously, only CardsWithoutUnavailableChanged was monitored, causing the UI to fail to refresh and persistently display “Speaker”. pms: bug-345709
deepin pr auto review这段代码主要是在处理音频设备的热插拔、状态变更以及音频端口的过滤逻辑。整体逻辑较为完善，考虑了多种后端（如PipeWire、PulseAudio）和不同硬件（蓝牙、有线耳机）的场景。以下是对代码的详细审查和改进建议： 1. 语法逻辑优点：
改进建议：
2. 代码质量优点：
改进建议：
3. 代码性能改进建议：
4. 代码安全改进建议：
5. 其他建议
总结这段代码整体质量较高，但在性能优化、安全性和可维护性方面仍有改进空间。建议重点关注：
|
Reviewer's GuideWire up additional DBus and Qt multimedia signals to keep the audio device list in sync on hotplug events and adjust port filtering so that when headphones are plugged, only the headphone output is shown instead of speakers on multi-port laptop cards. Sequence diagram for headphone hotplug refresh and UI updatesequenceDiagram
actor User
participant HeadphoneJack as HeadphoneJack
participant AudioBackend as AudioBackend
participant SoundDBusProxy as SoundDBusProxy
participant QMediaDevices as QMediaDevices
participant SoundWorker as SoundWorker
participant SoundModel as SoundModel
participant AudioDeviceDropdown as AudioDeviceDropdown
User->>HeadphoneJack: Plug headphones
HeadphoneJack->>AudioBackend: Electrical state change
AudioBackend-->>SoundDBusProxy: PortEnabledChanged(cardId, portId, enabled)
SoundDBusProxy-->>SoundWorker: PortEnabledChanged(cardId, portId, enabled)
activate SoundWorker
SoundWorker->>SoundModel: findPort(portId, cardId)
alt Port exists
SoundWorker->>SoundModel: Port.setEnabled(enabled)
SoundWorker->>SoundModel: updatePortCombo()
SoundWorker->>SoundModel: updateSoundDeviceModel(port)
SoundWorker->>SoundModel: updateActiveComboIndex()
else Port missing
SoundWorker->>SoundDBusProxy: cardsWithoutUnavailable()
SoundDBusProxy-->>SoundWorker: cardsSnapshot
SoundWorker->>SoundModel: setAudioCards(cardsSnapshot)
SoundModel->>SoundWorker: audioCardsChanged(cards)
SoundWorker->>SoundWorker: cardsChanged(cards)
SoundWorker->>SoundModel: updatePortCombo()
SoundWorker->>SoundModel: updateSoundDeviceModel(port)
SoundWorker->>SoundModel: updateActiveComboIndex()
end
SoundModel-->>AudioDeviceDropdown: modelChanged
deactivate SoundWorker
Note over SoundWorker,SoundModel: Inside cardsChanged(cards), headphone ports are detected and non-headphone
Note over SoundWorker,SoundModel: output ports are filtered out when headphones are plugged so dropdown shows Headphones only
rect rgb(230, 245, 255)
AudioBackend-->>SoundDBusProxy: SinksChanged or SourcesChanged
SoundDBusProxy-->>SoundWorker: SinksChanged(value) or SourcesChanged(value)
SoundWorker->>SoundDBusProxy: cardsWithoutUnavailable()
SoundDBusProxy-->>SoundWorker: cardsSnapshot
SoundWorker->>SoundModel: setAudioCards(cardsSnapshot)
end
rect rgb(235, 255, 235)
QMediaDevices-->>SoundWorker: audioOutputsChanged
SoundWorker->>SoundDBusProxy: cardsWithoutUnavailable()
SoundDBusProxy-->>SoundWorker: cardsSnapshot
SoundWorker->>SoundModel: setAudioCards(cardsSnapshot)
end
Updated class diagram for SoundWorker sound hotplug handling and port filteringclassDiagram
class SoundWorker {
-SoundModel* m_model
-SoundDBusProxy* m_soundDBusInter
-QMediaDevices* m_mediaDevices
-QTimer* m_pingTimer
-QTimer* m_playAnimationTime
-uint m_activeOutputCard
-QString m_activeSinkPort
+SoundWorker(SoundModel* model, QObject* parent)
+initConnect() void
+cardsChanged(QString cards) void
}
class SoundModel {
+setAudioCards(QJsonArray cards) void
+findPort(QString portId, uint cardId) Port*
+updatePortCombo() void
+updateSoundDeviceModel(Port* port) void
+updateActiveComboIndex() void
+setMaxUIVolume(int volume) void
+setIncreaseVolume(bool enable) void
+setReduceNoise(bool enable) void
+setPausePlayer(bool pause) void
+setBluetoothAudioModeOpts(QVariant opts) void
+setAutoSwitchSpeaker(bool enable) void
+setEnableSoundEffect(bool enable) void
+setIsLaptop(bool isLaptop) void
+signals audioCardsChanged(QString cards)
+signals defaultSinkChanged()
+signals defaultSourceChanged()
}
class SoundDBusProxy {
+cardsWithoutUnavailable() QJsonArray
+signal CardsWithoutUnavailableChanged(QJsonArray cards)
+signal CardsChanged(QJsonArray cards)
+signal SinksChanged(QList~QDBusObjectPath~ sinks)
+signal SourcesChanged(QList~QDBusObjectPath~ sources)
+signal PortEnabledChanged(uint cardId, QString portId, bool enabled)
+signal MaxUIVolumeChanged(int volume)
+signal IncreaseVolumeChanged(bool enable)
+signal ReduceNoiseChanged(bool enable)
+signal PausePlayerChanged(bool pause)
+signal BluetoothAudioModeOptsChanged(QVariant opts)
+signal AutoSwitchSpeakerChanged(bool enable)
+signal EnableSoundEffectChanged(bool enable)
+signal HasBatteryChanged(bool hasBattery)
+Tick() void
}
class QMediaDevices {
+signal audioInputsChanged()
+signal audioOutputsChanged()
}
class Port {
<<QObject>>
+enum Direction
+setEnabled(bool enabled) void
+isEnabled() bool
+getDirection() Direction
+getId() QString
+getCardId() uint
}
class AudioDeviceDropdown {
+setModel(SoundModel* model) void
+refreshView() void
}
SoundWorker --> SoundModel : uses
SoundWorker --> SoundDBusProxy : calls
SoundWorker --> QMediaDevices : observes
SoundModel --> Port : contains
AudioDeviceDropdown --> SoundModel : reads
SoundDBusProxy ..> SoundWorker : emits_CardsWithoutUnavailableChanged
SoundDBusProxy ..> SoundWorker : emits_CardsChanged
SoundDBusProxy ..> SoundWorker : emits_SinksChanged
SoundDBusProxy ..> SoundWorker : emits_SourcesChanged
SoundDBusProxy ..> SoundWorker : emits_PortEnabledChanged
QMediaDevices ..> SoundWorker : emits_audioInputsChanged
QMediaDevices ..> SoundWorker : emits_audioOutputsChanged
SoundModel ..> AudioDeviceDropdown : notifies_modelChanged
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider extracting the
refreshCardslambda into a small private helper method onSoundWorkerso it can be reused more clearly across different signal handlers and more easily extended/debugged in the future. - In
cardsChanged, the headphone detection and filtering logic does multipletoString()/toDouble()calls on the sameQJsonObject; caching these values per iteration (and possibly encapsulatingisHeadphonesPortand theshouldFilterToHeadphonesOutcomputation) would improve both readability and efficiency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the `refreshCards` lambda into a small private helper method on `SoundWorker` so it can be reused more clearly across different signal handlers and more easily extended/debugged in the future.
- In `cardsChanged`, the headphone detection and filtering logic does multiple `toString()`/`toDouble()` calls on the same `QJsonObject`; caching these values per iteration (and possibly encapsulating `isHeadphonesPort` and the `shouldFilterToHeadphonesOut` computation) would improve both readability and efficiency.
## Individual Comments
### Comment 1
<location> `src/plugin-sound/operation/soundworker.cpp:55` </location>
<code_context>
void SoundWorker::initConnect()
{
+ auto refreshCards = [this] {
+ if (!m_soundDBusInter)
+ return;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new lambdas and headphone-detection logic into small named helper methods/slots so the connection setup and cardsChanged() body remain simple and declarative.
You can reduce the added complexity substantially by extracting the inline lambdas into small, named member functions/slots and helpers, while keeping behavior identical.
### 1. `initConnect()` lambdas
`initConnect()` is now mixing connection wiring with non‑trivial logic in lambdas (`refreshCards`, `PortEnabledChanged`, `QMediaDevices` connections).
**Suggestion: extract `refreshCards` and `PortEnabledChanged` logic into private member functions/slots.**
```cpp
// soundworker.h
private slots:
void refreshCardsSnapshot();
void onPortEnabledChanged(uint cardId, const QString &portId, bool enabled);
```
```cpp
// soundworker.cpp
void SoundWorker::refreshCardsSnapshot()
{
if (!m_soundDBusInter)
return;
m_model->setAudioCards(m_soundDBusInter->cardsWithoutUnavailable());
}
void SoundWorker::onPortEnabledChanged(uint cardId, const QString &portId, bool enabled)
{
Port *port = m_model->findPort(portId, cardId);
if (!port) {
refreshCardsSnapshot();
return;
}
port->setEnabled(enabled);
m_model->updatePortCombo();
m_model->updateSoundDeviceModel(port);
m_model->updateActiveComboIndex();
}
```
Then `initConnect()` becomes declarative again:
```cpp
void SoundWorker::initConnect()
{
// ...
connect(m_soundDBusInter, &SoundDBusProxy::CardsWithoutUnavailableChanged,
m_model, &SoundModel::setAudioCards);
connect(m_soundDBusInter, &SoundDBusProxy::CardsChanged,
m_model, &SoundModel::setAudioCards);
connect(m_soundDBusInter, &SoundDBusProxy::SinksChanged,
this, [this](const QList<QDBusObjectPath> &) { refreshCardsSnapshot(); });
connect(m_soundDBusInter, &SoundDBusProxy::SourcesChanged,
this, [this](const QList<QDBusObjectPath> &) { refreshCardsSnapshot(); });
connect(m_soundDBusInter, &SoundDBusProxy::PortEnabledChanged,
this, &SoundWorker::onPortEnabledChanged);
connect(m_mediaDevices, &QMediaDevices::audioInputsChanged,
this, &SoundWorker::refreshCardsSnapshot);
connect(m_mediaDevices, &QMediaDevices::audioOutputsChanged,
this, &SoundWorker::refreshCardsSnapshot);
// ...
}
```
This keeps all current behavior but moves the business logic out of the constructor helper.
### 2. Headphone filtering logic in `cardsChanged()`
The new headphone logic adds an inline `isHeadphonesPort` lambda and a two‑pass loop structure. This is a good candidate for small helpers to keep `cardsChanged()` focused.
**Suggestion: extract headphone helpers and reuse them in a single, simple pattern.**
```cpp
// soundworker.h
private:
bool isHeadphonesPort(const QJsonObject &jPort) const;
bool shouldFilterToHeadphonesOut(const QJsonArray &jPorts, uint cardId) const;
```
```cpp
// soundworker.cpp
bool SoundWorker::isHeadphonesPort(const QJsonObject &jPort) const
{
const QString portId = jPort["Name"].toString();
const QString desc = jPort["Description"].toString();
return portId.contains("headphone", Qt::CaseInsensitive)
|| desc.contains("headphone", Qt::CaseInsensitive)
|| desc.contains(QStringLiteral("耳机"));
}
bool SoundWorker::shouldFilterToHeadphonesOut(const QJsonArray &jPorts, uint cardId) const
{
for (const QJsonValue &pV : jPorts) {
const QJsonObject jPort = pV.toObject();
const auto dir = Port::Direction(jPort["Direction"].toDouble());
if (dir != Port::Out || !isHeadphonesPort(jPort))
continue;
const double portAvai = jPort["Available"].toDouble();
const QString portId = jPort["Name"].toString();
if (portAvai == 2.0) // Available
return true;
if (cardId == m_activeOutputCard && portId == m_activeSinkPort)
return true;
}
return false;
}
```
Then in `cardsChanged()`:
```cpp
const bool filterToHeadphones = shouldFilterToHeadphonesOut(jPorts, cardId);
for (QJsonValue pV : jPorts) {
QJsonObject jPort = pV.toObject();
const double portAvai = jPort["Available"].toDouble();
if (portAvai != 2.0 && portAvai != 0.0)
continue;
const auto dir = Port::Direction(jPort["Direction"].toDouble());
if (filterToHeadphones && dir == Port::Out && !isHeadphonesPort(jPort))
continue;
const QString portId = jPort["Name"].toString();
const QString portName = jPort["Description"].toString();
const bool isEnabled = jPort["Enabled"].toBool();
const bool isBluetooth = jPort["Bluetooth"].toBool();
// existing Port creation/update logic...
}
```
This preserves all the new behavior (two‑phase decision: first decide if we filter, then apply it) but reduces nesting in `cardsChanged()` and clarifies intent via named helpers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| void SoundWorker::initConnect() | ||
| { | ||
| auto refreshCards = [this] { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the new lambdas and headphone-detection logic into small named helper methods/slots so the connection setup and cardsChanged() body remain simple and declarative.
You can reduce the added complexity substantially by extracting the inline lambdas into small, named member functions/slots and helpers, while keeping behavior identical.
1. initConnect() lambdas
initConnect() is now mixing connection wiring with non‑trivial logic in lambdas (refreshCards, PortEnabledChanged, QMediaDevices connections).
Suggestion: extract refreshCards and PortEnabledChanged logic into private member functions/slots.
// soundworker.h
private slots:
void refreshCardsSnapshot();
void onPortEnabledChanged(uint cardId, const QString &portId, bool enabled);// soundworker.cpp
void SoundWorker::refreshCardsSnapshot()
{
if (!m_soundDBusInter)
return;
m_model->setAudioCards(m_soundDBusInter->cardsWithoutUnavailable());
}
void SoundWorker::onPortEnabledChanged(uint cardId, const QString &portId, bool enabled)
{
Port *port = m_model->findPort(portId, cardId);
if (!port) {
refreshCardsSnapshot();
return;
}
port->setEnabled(enabled);
m_model->updatePortCombo();
m_model->updateSoundDeviceModel(port);
m_model->updateActiveComboIndex();
}Then initConnect() becomes declarative again:
void SoundWorker::initConnect()
{
// ...
connect(m_soundDBusInter, &SoundDBusProxy::CardsWithoutUnavailableChanged,
m_model, &SoundModel::setAudioCards);
connect(m_soundDBusInter, &SoundDBusProxy::CardsChanged,
m_model, &SoundModel::setAudioCards);
connect(m_soundDBusInter, &SoundDBusProxy::SinksChanged,
this, [this](const QList<QDBusObjectPath> &) { refreshCardsSnapshot(); });
connect(m_soundDBusInter, &SoundDBusProxy::SourcesChanged,
this, [this](const QList<QDBusObjectPath> &) { refreshCardsSnapshot(); });
connect(m_soundDBusInter, &SoundDBusProxy::PortEnabledChanged,
this, &SoundWorker::onPortEnabledChanged);
connect(m_mediaDevices, &QMediaDevices::audioInputsChanged,
this, &SoundWorker::refreshCardsSnapshot);
connect(m_mediaDevices, &QMediaDevices::audioOutputsChanged,
this, &SoundWorker::refreshCardsSnapshot);
// ...
}This keeps all current behavior but moves the business logic out of the constructor helper.
2. Headphone filtering logic in cardsChanged()
The new headphone logic adds an inline isHeadphonesPort lambda and a two‑pass loop structure. This is a good candidate for small helpers to keep cardsChanged() focused.
Suggestion: extract headphone helpers and reuse them in a single, simple pattern.
// soundworker.h
private:
bool isHeadphonesPort(const QJsonObject &jPort) const;
bool shouldFilterToHeadphonesOut(const QJsonArray &jPorts, uint cardId) const;// soundworker.cpp
bool SoundWorker::isHeadphonesPort(const QJsonObject &jPort) const
{
const QString portId = jPort["Name"].toString();
const QString desc = jPort["Description"].toString();
return portId.contains("headphone", Qt::CaseInsensitive)
|| desc.contains("headphone", Qt::CaseInsensitive)
|| desc.contains(QStringLiteral("耳机"));
}
bool SoundWorker::shouldFilterToHeadphonesOut(const QJsonArray &jPorts, uint cardId) const
{
for (const QJsonValue &pV : jPorts) {
const QJsonObject jPort = pV.toObject();
const auto dir = Port::Direction(jPort["Direction"].toDouble());
if (dir != Port::Out || !isHeadphonesPort(jPort))
continue;
const double portAvai = jPort["Available"].toDouble();
const QString portId = jPort["Name"].toString();
if (portAvai == 2.0) // Available
return true;
if (cardId == m_activeOutputCard && portId == m_activeSinkPort)
return true;
}
return false;
}Then in cardsChanged():
const bool filterToHeadphones = shouldFilterToHeadphonesOut(jPorts, cardId);
for (QJsonValue pV : jPorts) {
QJsonObject jPort = pV.toObject();
const double portAvai = jPort["Available"].toDouble();
if (portAvai != 2.0 && portAvai != 0.0)
continue;
const auto dir = Port::Direction(jPort["Direction"].toDouble());
if (filterToHeadphones && dir == Port::Out && !isHeadphonesPort(jPort))
continue;
const QString portId = jPort["Name"].toString();
const QString portName = jPort["Description"].toString();
const bool isEnabled = jPort["Enabled"].toBool();
const bool isBluetooth = jPort["Bluetooth"].toBool();
// existing Port creation/update logic...
}This preserves all the new behavior (two‑phase decision: first decide if we filter, then apply it) but reduces nesting in cardsChanged() and clarifies intent via named helpers.
caixr23
approved these changes
Feb 6, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, JWWTSL 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 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
log: The audio module's output device list/dropdown relies on SoundWorker::cardsChanged() for refreshes. However, in certain backend implementations, plugging/unplugging wired headphones primarily triggers either the DBus port enabled state change (PortEnabledChanged) or solely CardsChanged events. Previously, only CardsWithoutUnavailableChanged was monitored, causing the UI to fail to refresh and persistently display “Speaker”.
pms: bug-345709
Summary by Sourcery
Improve audio device hotplug handling so the output device list and labels stay in sync with actual hardware, especially when plugging or unplugging wired headphones.
Bug Fixes: