Repository navigation
fix: ambient Supported reports hardware, not sampling - #194
Conversation
Reviewer's GuideThe PR changes ambient brightness Supported from a sampling-status proxy into an asynchronously probed hardware capability, while modeling disabled sampling separately, centralizing lifecycle behavior in the Service, preserving state across policy rebuilds, and adding self-contained state-machine coverage. Sequence diagram for asynchronous ambient sensor capability detectionsequenceDiagram
participant Service as AmbientBrightnessService
participant SensorProxy as SensorProxy
participant Model as AmbientBrightnessModel
participant Watcher as QDBusPendingCallWatcher
Service->>SensorProxy: LookupAmbientLightSensor()
SensorProxy-->>Watcher: asynchronous reply
Watcher->>Service: handleSensorPresentChanged(present)
Service->>Model: setSupported(present)
alt sensor unavailable
Service->>Model: makeUnavailable()
else sensor available but lifecycle stopped
Service->>Model: setDisabled()
else sensor available and lifecycle runnable
Service->>Service: connectSensor()
end
State diagram for ambient brightness capability and sampling lifecyclestateDiagram-v2
[*] --> Unavailable
Unavailable --> Disabled: capability detected
Disabled --> WaitingForSample: lifecycle allows sampling
WaitingForSample --> Active: valid LightLevel
Active --> Disabled: stopSampling()
WaitingForSample --> Disabled: stopSampling()
Disabled --> WaitingForSample: lifecycle restored
Disabled --> Unavailable: capability lost
Active --> Unavailable: capability lost
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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/plugin-qt/ambient-brightness/ambientbrightnessservice.h" line_range="55-60" />
<code_context>
void enabledChanged(bool value);
void recommendedBrightnessChanged(double value);
+public Q_SLOTS:
+ /// 启动运行时逻辑。与 initialize() 分开，便于测试在没有系统总线时构造服务。
+ void start();
+ /// 异步能力探测的回调入口。
+ void handleSensorPresentChanged(bool present);
+
private Q_SLOTS:
void onSensorServiceRegistered();
</code_context>
<issue_to_address>
**Bus clients can forge sensor state**
When a D-Bus client invokes either newly exported slot, `ExportAllSlots` exposes `start()` and `handleSensorPresentChanged(bool)`, allowing callers to overwrite `Supported` and `State` or trigger a sensor claim without a capability probe. The service can therefore report fabricated sensor state or start sampling.
Make these methods unavailable to D-Bus clients, for example by removing them from the exported slots.
</issue_to_address>
### Comment 2
<location path="src/plugin-qt/ambient-brightness/ambientbrightnessservice.cpp" line_range="505" />
<code_context>
void AmbientBrightnessService::onSensorServiceUnregistered()
{
- disconnectSensor();
+ // 传感器服务退出后需要重新探测：Supported 必须随之变为 false 并对外广播。
+ refreshSensorCapability();
</code_context>
<issue_to_address>
**Stop events block the main thread**
When a lifecycle stop occurs while a sensor is claimed, `stopSampling()` and the lifecycle refresh path call `disconnectSensor()`, which synchronously calls `ReleaseLight`. Lid close, sleep, session backgrounding, or switch-off therefore blocks the event thread on D-Bus.
Make the `ReleaseLight` call non-blocking on lifecycle stop paths.
Also at `src/plugin-qt/ambient-brightness/ambientbrightnessservice.cpp:462`, `src/plugin-qt/ambient-brightness/README.md:51-55`.
</issue_to_address>
### Comment 3
<location path="src/plugin-qt/ambient-brightness/ambientbrightnessservice.cpp" line_range="538" />
<code_context>
+ }
+ // 直接启动，不经 refreshSensorConnection()，避免探测→刷新→探测的自我循环。
+ if (!m_claimed)
+ connectSensor();
+}
+
</code_context>
<issue_to_address>
**Lifecycle refresh still blocks on D-Bus**
When the lid opens or the system wakes while sampling is enabled, `refreshSensorConnection()` calls `connectSensor()` directly; that function synchronously reads sensor properties and calls `ClaimLight`. Lid-open and wake recovery reach this path via the refresh timer, so those lifecycle events still block the main thread on D-Bus despite the stated hot-path guarantee.
Make sensor setup and claiming asynchronous as well, or schedule the synchronous operations off the lifecycle event path.
</issue_to_address>
|
Thanks for the review. Re #1 (exported slots) — fixed in 8eadf97: Re #2/#3 (synchronous ClaimLight/ReleaseLight) — deliberate and pre-existing; this change only consolidates the two inline |
|
Follow-up on #2/#3: the claim/release path is now bounded instead of left as-is.
Measured with a peer that holds
Claim/release stays synchronous by design (some drivers report the first frame during
|
|
Correction — I've reverted the timeout change described above; it was not a good fit for this fix, and the branch is back to the state reviewed. Why it was wrong, on this repo's own terms:
The underlying blocking concern is real and pre-existing, and deserves its own change: per-call timeouts that also cover the |
|
Added a second commit (
Measured end-to-end against a fake
Claim/release stays synchronous by design — the claim needs its own result and the first-frame-during- |
1. Supported now reports whether the machine has a usable ambient light sensor, independent of the switch, lid, sleep and session state. 2. Probe capability asynchronously via LookupAmbientLightSensor, with no synchronous D-Bus round trip on lid/sleep/session hot paths. 3. Stop treating "should not sample" as "no hardware": stopping sampling keeps Supported and moves State to Disabled instead of Unavailable. 4. Make the model capability flag service-owned, and stop disconnectSensor from writing model state so one exit no longer answers every cause. 5. Unify both switch-off paths into stopSampling, and keep Disabled or Unavailable across policy rebuilds. 6. Notify Enabled on cold start, and change the default of ambientLightAdjustBrightness to off. 7. Cover the capability semantics with a self-contained test: drive the service state machine directly and point the only bus address at an unreachable path, so no dbus-daemon is required. Log: Ambient brightness reported itself unsupported when the switch was off Influence: 1. On a machine with an ambient light sensor and the switch off at start, verify Supported is true and State is Disabled, and that no ClaimLight is issued. 2. Turn the switch off at runtime and verify Supported stays true while State becomes Disabled. 3. Verify a machine without iio-sensor-proxy reports Supported=false and State=Unavailable, and that a non-lux unit is rejected. 4. Verify lid close/sleep/session background only stop sampling and never clear Supported. 5. Run ctest for ambient-brightness-policy and ambient-brightness-capability. fix: 修正 Supported 语义为硬件能力而非采样状态 1. Supported 改为上报本机是否具备可用环境光传感器，与开关、盖子、 休眠和会话状态无关。 2. 通过 LookupAmbientLightSensor 异步探测能力，合盖/休眠/会话切换等 热路径上不再有同步 D-Bus 往返。 3. 不再把"不应采样"当成"没有硬件"：停止采样保留 Supported，状态转为 Disabled 而非 Unavailable。 4. 能力标志改由 Service 独占写入，disconnectSensor 不再写模型状态， 一个出口不再同时解释所有停止原因。 5. 合并两条关闭开关路径到 stopSampling，策略重建时保留 Disabled 或 Unavailable。 6. 冷启动补发 Enabled 通知，并将 ambientLightAdjustBrightness 默认值 改为关闭。 7. 补充自包含的能力语义用例：直接驱动服务状态机，并把唯一使用的总线 地址指向不可达路径，因此不需要 dbus-daemon。 Log: 自动亮度在开关关闭时把自己误报为不支持 Influence: 1. 在具备环境光传感器且启动时开关关闭的机器上，验证 Supported 为 true、 State 为 Disabled，且未调用 ClaimLight。 2. 运行中关闭开关，验证 Supported 保持 true 且 State 变为 Disabled。 3. 验证无 iio-sensor-proxy 时 Supported=false、State=Unavailable， 且非 lux 单位被拒绝。 4. 验证合盖/休眠/会话切后台只停止采样，不会清掉 Supported。 5. 运行 ctest 的 ambient-brightness-policy 与 ambient-brightness-capability。 PMS: BUG-378895
1. Give the synchronous sensor calls an explicit 3 s cap: ClaimLight, the property reads, and the gate taken before constructing QDBusInterface, whose synchronous introspection no interface timeout can bound. 2. Send ReleaseLight asynchronously: the stop-sampling path no longer waits for a reply, which it never used. 3. A sensor that does not answer now only moves State to Unavailable, leaving Supported to the capability probe, and the next lifecycle refresh retries. Log: An unresponsive iio-sensor-proxy froze ambient brightness for up to 25 seconds; the worst case is now 3 s and light release no longer waits Influence: 1. With an ambient light sensor present, toggle the switch repeatedly and verify Supported stays true while State goes Disabled and back to WaitingForSample/Active. 2. Stop iio-sensor-proxy or point DBUS_SYSTEM_BUS_ADDRESS at an unreachable address; verify Enable() and the properties still answer, State is Unavailable, and nothing freezes for 25 seconds. 3. Bring the sensor service back and verify the next lid/sleep/session refresh claims the sensor again and brightness recommendations resume. fix: 传感器调用限时并异步释放光线 1. 给同步的传感器调用加 3 秒上限：ClaimLight、属性读取，以及构造 QDBusInterface 之前的那道闸门（构造期的同步内省不受接口超时约束）。 2. ReleaseLight 改为异步发出：停止采样路径不再等回复，它本来就不用返回值。 3. 传感器不应答时只把 State 置为 Unavailable，Supported 仍归能力探测改写， 下一次生命周期刷新会自动重试。 Log: 光感服务无响应时，自动亮度不再把服务卡住 25 秒；最坏 3 秒， 且释放光线不再等待 Influence: 1. 在有光感的机器上反复开关自动亮度，确认 Supported 保持 true，State 在 Disabled 与 WaitingForSample/Active 之间正确切换。 2. 停掉 iio-sensor-proxy（或把 DBUS_SYSTEM_BUS_ADDRESS 指向不可达地址）， 确认 Enable() 与属性读取仍有响应、State 为 Unavailable，且不会卡 25 秒。 3. 再把传感器服务拉起来，确认下一次合盖/休眠/会话刷分会重新 claim 并恢复 亮度推荐。 PMS: BUG-378895
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 良好 ✅ 通过 潜在问题：
建议： 1.在 connectSensor() 的 setSupported(false) 路径中补充 m_model.makeUnavailable() 调用以保持 State 一致性 2.可在 refreshSensorCapability() 中增加 m_pendingProbe 标志位避免重复探测 2. 代码质量 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 代码整体结构清晰，注释详尽且解释了'为什么'而非仅'是什么'。stopSampling() 有效消除了 onAutomaticBrightnessEnabledChanged() 和 Enable() 中的重复逻辑。测试覆盖8个场景，涵盖核心状态转换。建议将异步 ReleaseLight 调用封装为 releaseLightAsync() 辅助方法。 3. 代码性能 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： 本次重构显著提升了性能：refreshSensorCapability() 完全异步化，消除了合盖/休眠/会话切换/开关切换等热路径上的同步 D-Bus 往返；connectSensor() 增加3秒超时闸门避免 Qt 默认25秒阻塞；disconnectSensor() 的 ReleaseLight 改为异步调用；refreshSensorConnection() 不再调用 isServiceRegistered() 同步方法。QDBusPendingCallWatcher 使用规范，均通过 deleteLater() 清理。 4. 代码安全 🔒评价： 优秀 ✅ 通过
安全漏洞详情： 建议： 存在0个安全漏洞。所有 D-Bus 调用使用常量服务路径和方法名，无用户输入注入风险。无硬编码密钥或敏感信息。错误日志仅输出 D-Bus 错误名和消息，不含敏感数据。3秒超时机制防止拒绝服务。 💡 改进建议代码示例// 建议：在 connectSensor() 的 setSupported(false) 路径中补充状态同步
void AmbientBrightnessService::connectSensor()
{
// ...
if (!m_sensor->isValid()) {
qCWarning(logAmbientBrightness) << "sensor interface invalid";
delete m_sensor;
m_sensor = nullptr;
m_model.setSupported(false);
m_model.makeUnavailable(); // 补充：保持 State 一致
return;
}
// ...
}
// 建议：封装异步 ReleaseLight 调用
void AmbientBrightnessService::releaseLightAsync()
{
auto systemBus = QDBusConnection::systemBus();
auto *watcher = new QDBusPendingCallWatcher(
systemBus.asyncCall(QDBusMessage::createMethodCall(
QString::fromLatin1(kSensorService), QString::fromLatin1(kSensorPath),
QString::fromLatin1(kSensorInterface), QStringLiteral("ReleaseLight"))),
this);
connect(watcher, &QDBusPendingCallWatcher::finished, watcher,
[](QDBusPendingCallWatcher *self) {
if (self->isError())
qCWarning(logAmbientBrightness)
<< "ReleaseLight failed" << self->error().message();
self->deleteLater();
});
}本报告由 AI 代码审查工具自动生成 |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerger |
|
This pr force merged! (status: blocked) |
sensor, independent of the switch, lid, sleep and session state.
synchronous D-Bus round trip on lid/sleep/session hot paths.
keeps Supported and moves State to Disabled instead of Unavailable.
from writing model state so one exit no longer answers every cause.
Unavailable across policy rebuilds.
ambientLightAdjustBrightness to off.
service state machine directly and point the only bus address at an
unreachable path, so no dbus-daemon is required.
Log: Ambient brightness reported itself unsupported when the switch was off
Influence:
verify Supported is true and State is Disabled, and that no ClaimLight
is issued.
State becomes Disabled.
State=Unavailable, and that a non-lux unit is rejected.
clear Supported.
fix: 修正 Supported 语义为硬件能力而非采样状态
休眠和会话状态无关。
热路径上不再有同步 D-Bus 往返。
Disabled 而非 Unavailable。
一个出口不再同时解释所有停止原因。
Unavailable。
改为关闭。
地址指向不可达路径，因此不需要 dbus-daemon。
Log: 自动亮度在开关关闭时把自己误报为不支持
Influence:
State 为 Disabled，且未调用 ClaimLight。
且非 lux 单位被拒绝。
PMS: BUG-378895
Summary by Sourcery
Report ambient-light hardware support independently from sampling eligibility while keeping lifecycle transitions responsive and accurately reflected.
Bug Fixes:
Enhancements:
Documentation:
Tests: