fix(power): fix idle ignored after wakeup, add wakeup lock, and suppo…#74
Open
mhduiy wants to merge 1 commit into
Open
fix(power): fix idle ignored after wakeup, add wakeup lock, and suppo…#74mhduiy wants to merge 1 commit into
mhduiy wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Reviewer's GuideUpdates power/session behavior so idle is correctly reset on wakeup, lock-on-sleep/wakeup is applied consistently, and Wayland output DPMS/brightness continues to work across hotplug and output reordering by keeping wl_registry alive and using stable lookup keys instead of indices. Sequence diagram for updated sleep/wakeup lock and idle reset behaviorsequenceDiagram
participant System as SystemPowerEvents
participant PM as PowerManager
participant PSP as PowerSavePlan
participant Timer as QTimer_singleShot
System->>PM: handleBeforeSleep()
PM->>PM: setBlackScreenActive(true)
alt sleepLock enabled
PM->>PM: doLock(true)
end
System->>PM: handleWakeup()
PM->>PM: setDPMSModeOn()
PM->>PSP: HandleIdleOff()
PSP->>PM: SetPrepareSuspend(PS_Normal)
PM->>Timer: singleShot(m_delayWakeupInterval,...)
Timer-->>PM: wakeup delay callback
PM->>PM: m_delayInActive = false
alt sleepLock enabled
PM->>PM: doLock(true)
end
PM->>PM: setBlackScreenActive(false)
alt scheduledShutdownState
PM->>PM: scheduledShutdown(SchedInit)
end
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:
- In
WaylandScreenController::discoverOutputs,m_registryis overwritten each time without destroying any previously created registry, so ifdiscoverOutputs()can be called more than once per instance you should clean up or reset the existingm_registrybefore assigning a new one to avoid leaks or double-destruction issues in the destructor. - With the new unconditional
doLock(true)in bothhandleBeforeSleepand the delayed callback inhandleWakeupwhenm_sleepLockis set, it would be good to confirm the lock semantics (and user experience) for non-Wayland sessions and ensure the session is not redundantly re-locked on wake if it was already locked before suspend.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `WaylandScreenController::discoverOutputs`, `m_registry` is overwritten each time without destroying any previously created registry, so if `discoverOutputs()` can be called more than once per instance you should clean up or reset the existing `m_registry` before assigning a new one to avoid leaks or double-destruction issues in the destructor.
- With the new unconditional `doLock(true)` in both `handleBeforeSleep` and the delayed callback in `handleWakeup` when `m_sleepLock` is set, it would be good to confirm the lock semantics (and user experience) for non-Wayland sessions and ensure the session is not redundantly re-locked on wake if it was already locked before suspend.
## Individual Comments
### Comment 1
<location path="src/plugin-qt/power/session/screen/screencontroller_wl.cpp" line_range="189" />
<code_context>
- if (idx < int(sc->m_outputs.size())) {
- sc->m_outputs[idx].currentMode = m;
- Q_EMIT sc->modeChanged(idx, m == 0 ? ScreenController::Off : ScreenController::On);
+ auto *pwr = out.power.get();
+ QObject::connect(pwr, &OutputPower::modeChanged,
+ sc, [sc, pwr](uint32_t m) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated output-lookup loops into small helper methods on WaylandScreenController and calling those from the lambdas to keep the new dynamic lookup behavior while simplifying the signal handlers.
You can reduce the added complexity by centralizing the lookup logic instead of hand‑rolling three similar loops in lambdas.
Introduce small lookup helpers on `WaylandScreenController`:
```cpp
int WaylandScreenController::indexForPower(const OutputPower *pwr) const
{
for (int i = 0; i < int(m_outputs.size()); ++i) {
if (m_outputs[i].power.get() == pwr) {
return i;
}
}
return -1;
}
int WaylandScreenController::indexForColorControl(const OutputColorControl *cc) const
{
for (int i = 0; i < int(m_outputs.size()); ++i) {
if (m_outputs[i].colorControl.get() == cc) {
return i;
}
}
return -1;
}
int WaylandScreenController::indexForRegistry(uint32_t regName) const
{
for (int i = 0; i < int(m_outputs.size()); ++i) {
if (m_outputs[i].registryName == regName) {
return i;
}
}
return -1;
}
```
Then simplify the lambdas:
```cpp
auto *pwr = out.power.get();
QObject::connect(pwr, &OutputPower::modeChanged,
sc, [sc, pwr](uint32_t m) {
const int i = sc->indexForPower(pwr);
if (i < 0)
return;
sc->m_outputs[i].currentMode = m;
Q_EMIT sc->modeChanged(i, m == 0 ? ScreenController::Off : ScreenController::On);
});
```
```cpp
auto *cc = out.colorControl.get();
QObject::connect(cc, &OutputColorControl::brightnessChanged,
sc, [sc, cc](double v) {
const int i = sc->indexForColorControl(cc);
if (i < 0)
return;
Q_EMIT sc->brightnessChanged(i, v);
});
```
```cpp
auto regName = out.registryName;
connect(anim, &QVariantAnimation::valueChanged, this,
[this, regName](const QVariant &v) {
const int i = indexForRegistry(regName);
if (i < 0)
return;
auto &o = m_outputs[i];
if (!o.colorControl)
return;
o.colorControl->set_brightness(wl_fixed_from_double(v.toDouble()));
o.colorControl->commit();
wl_display_flush(m_display);
});
```
This keeps the new behavior (dynamic lookups resilient to vector reordering and registry‑based animations) but removes duplicated search logic, making the signal handlers easier to read and future changes to lookup strategy localized to a few 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.
…rt output hotplug 1. Reset prepareSuspend to PS_Normal in HandleIdleOff so idle-on events are not permanently ignored after suspend/resume cycle 2. Call doLock in handleBeforeSleep unconditionally (not just Wayland) and in the wakeup delayed callback when sleepLock is set, ensuring lock screen shows after idle-timeout suspend 3. Keep wl_registry alive in WaylandScreenController to receive output hotplug events (zwlr_output_power_v1 and treeland color control for new monitors) 4. Use pointer-based lookup in signal handlers instead of captured vector index to avoid stale references after output removal 5. Switch brightness animation key from int index to registryName to survive output reordering on unplug Log: Fix idle state reset after wakeup, add wakeup lock logic, and support DPMS/brightness for hotplugged Wayland outputs fix(power): 修复唤醒后 idle 被忽略、增加唤醒锁屏、支持屏幕热插拔 1. HandleIdleOff 中重置 prepareSuspend 为 PS_Normal，修复待机唤醒后 idle 事件被永久忽略的问题 2. handleBeforeSleep 中不再限制 Wayland 才锁屏，handleWakeup 延迟回调中 sleepLock 开启时调用 doLock，确保 idle 超时待机唤醒后显示锁屏界面 3. WaylandScreenController 保持 wl_registry 活跃以接收热插拔事件，新接入屏幕自动获取 DPMS 和亮度控制 4. 信号回调改用裸指针查找当前 index，避免拔屏后 index 偏移导致访问错误 output 5. 亮度动画 key 改为 registryName，拔屏后 key 不受 index 偏移影响 Log: 修复唤醒后 idle 状态重置、增加唤醒锁屏、支持 Wayland 屏幕热插拔 DPMS/亮度管控
deepin pr auto review这份 Git Diff 主要对电源管理模块和 Wayland 屏幕控制模块进行了重构和 Bug 修复。整体来看，修改方向非常正确，特别是将基于数组索引的信号/槽连接重构为基于指针或唯一标识符的查找，这极大地增强了代码的健壮性，避免了由于数组增删导致的悬空指针或越界访问。 不过，在代码审查中，我仍发现了一些关于语法逻辑、代码质量、代码性能和代码安全方面的改进空间。以下是详细的审查意见： 1. 语法与逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与修改建议代码片段针对最核心的安全与生命周期问题，建议对 // 改进前：
auto *pwr = out.power.get();
QObject::connect(pwr, &OutputPower::modeChanged,
sc, [sc, pwr](uint32_t m) {
auto &outs = sc->m_outputs;
for (int i = 0; i < int(outs.size()); ++i) {
if (outs[i].power.get() == pwr) {
outs[i].currentMode = m;
Q_EMIT sc->modeChanged(i, m == 0 ? ScreenController::Off : ScreenController::On);
return;
}
}
});
// 改进后 (使用 QPointer 防止悬空指针)：
auto *pwr = out.power.get();
QPointer<WaylandScreenController> safeSc = sc; // 确保控制器生命周期安全
QObject::connect(pwr, &OutputPower::modeChanged,
sc, [safeSc, pwr](uint32_t m) {
if (!safeSc) return; // 如果控制器已销毁，直接返回
auto &outs = safeSc->m_outputs;
for (int i = 0; i < int(outs.size()); ++i) {
if (outs[i].power.get() == pwr) {
outs[i].currentMode = m;
Q_EMIT safeSc->modeChanged(i, m == 0 ? ScreenController::Off : ScreenController::On);
return;
}
}
});总体而言，这次重构解决了之前基于数组索引带来的核心隐患，方向明确。只需在指针生命周期管理上再严谨一些，代码的质量和安全性将会更高。 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
…rt output hotplug
Log: Fix idle state reset after wakeup, add wakeup lock logic, and support DPMS/brightness for hotplugged Wayland outputs
fix(power): 修复唤醒后 idle 被忽略、增加唤醒锁屏、支持屏幕热插拔
Log: 修复唤醒后 idle 状态重置、增加唤醒锁屏、支持 Wayland 屏幕热插拔 DPMS/亮度管控
Summary by Sourcery
Fix power management idle handling after wakeup and improve Wayland output handling for hotplug and brightness control.
Bug Fixes:
Enhancements: