fix: skip screen off if already in dpms off state#1148
Closed
fly602 wants to merge 1 commit into
Closed
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602 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 guide (collapsed on small PRs)Reviewer's GuideAdds a DPMS state check before turning off the screen, introduces a helper to read /tmp/dpms-state, and reorders suspend state constants for clearer semantics. Sequence diagram for DPMS-aware systemTurnOffScreensequenceDiagram
actor User
participant PowerButton
participant Manager
participant isDpmsOff
participant DPMS
User->>PowerButton: press
PowerButton->>Manager: systemTurnOffScreen
Manager->>isDpmsOff: isDpmsOff
isDpmsOff->>isDpmsOff: os.ReadFile_tmp_dpms_state
isDpmsOff-->>Manager: dpmsOffFlag
alt [dpmsOffFlag == true]
Manager-->>PowerButton: return (no action)
else [dpmsOffFlag == false]
Manager->>DPMS: DPMS Off
DPMS-->>Manager: screen off
Manager->>DPMS: os.WriteFile_tmp_dpms_state
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:
- Reordering the iota-based suspendState constants changes their numeric values; if these are persisted or used across processes, consider assigning explicit values or appending new states instead of reordering to avoid breaking existing consumers.
- The
/tmp/dpms-statepath and magic value"1"are duplicated between writer and reader; consider extracting a shared constant and/or helper to avoid drift if the file location or format changes. - isDpmsOff treats any read error as 'not off' and logs only at debug; if a missing or unreadable state file should be handled differently from a deliberate 'screen on' state, consider distinguishing ENOENT and other errors and logging unexpected failures at a higher level.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Reordering the iota-based suspendState constants changes their numeric values; if these are persisted or used across processes, consider assigning explicit values or appending new states instead of reordering to avoid breaking existing consumers.
- The `/tmp/dpms-state` path and magic value `"1"` are duplicated between writer and reader; consider extracting a shared constant and/or helper to avoid drift if the file location or format changes.
- isDpmsOff treats any read error as 'not off' and logs only at debug; if a missing or unreadable state file should be handled differently from a deliberate 'screen on' state, consider distinguishing ENOENT and other errors and logging unexpected failures at a higher level.
## Individual Comments
### Comment 1
<location path="keybinding1/utils.go" line_range="33-34" />
<code_context>
const (
suspendStateUnknown = iota + 1
suspendStateLidOpen
+ suspendStateLidClose
suspendStateFinish
suspendStateWakeup
</code_context>
<issue_to_address>
**issue (bug_risk):** Reordering iota-based suspend state constants may break any persisted or external uses of these numeric values
Inserting `suspendStateLidClose` here shifts the numeric values of all following states. If these integers are persisted (config/DB), sent over IPC, or shared across processes, this can cause regressions. If reordering is safe in this case, consider assigning explicit values to each constant so future reordering doesn’t change their numeric representation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
/tmp/dpms-state 分别在keybinding和power这两个模块中都有读写，属于是共享一个状态标志，不适合写入内存中。 |
1. Add a check at the beginning of the systemTurnOffScreen function to skip processing if the display is already in a DPMS off state 2. Introduce a new isDpmsOff helper function to read the state from / tmp/dpms-state and determine if the screen is already off 3. Adjust the ordering of suspend state constants for logical consistency, moving suspendStateLidClose before suspendStateFinish Log: Optimized power button logic to avoid redundant screen-off commands Influence: 1. Test pressing the power button when the screen is already off via DPMS; verify the screen state does not change and no redundant actions occur 2. Test pressing the power button when the screen is on; verify the screen turns off correctly 3. Test the behavior after waking the system from suspend (lid open); ensure the screen state is correctly reported and handled 4. Verify that the /tmp/dpms-state file is created and updated correctly on screen state changes 5. Test the interaction between the power button and other screen off mechanisms (like idle timeout) to ensure no conflicts fix: 如果屏幕已处于DPMS关闭状态则跳过屏幕关闭处理 1. 在systemTurnOffScreen函数开头添加检查，如果显示器已处于DPMS off状态则 跳过处理 2. 新增isDpmsOff辅助函数，通过读取/tmp/dpms-state文件来判断屏幕是否已 关闭 3. 调整挂起状态常量的顺序以保持逻辑一致性，将suspendStateLidClose移到 suspendStateFinish之前 Log: 优化电源键逻辑，避免重复发送屏幕关闭命令 Influence: 1. 测试在屏幕已通过DPMS关闭时按下电源键，验证屏幕状态不会改变且没有多余 操作 2. 测试在屏幕开启时按下电源键，验证屏幕能正确关闭 3. 测试从挂起状态唤醒（开盖）后的行为，确保屏幕状态能正确报告和处理 4. 验证/tmp/dpms-state文件在屏幕状态变化时能被正确创建和更新 5. 测试电源键与其他屏幕关闭机制（如空闲超时）的交互，确保没有冲突 PMS: BUG-364835
deepin pr auto review★ 总体评分：60分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // common/fileutil/fileutil.go
package fileutil
import (
"fmt"
"io"
"os"
"golang.org/x/sys/unix"
)
// SafeWriteFile 安全写入文件，拒绝符号链接和硬链接
func SafeWriteFile(filename string, content []byte, perm os.FileMode) error {
fi, err := os.Lstat(filename)
if err != nil && !os.IsNotExist(err) {
return err
}
if err == nil {
// 文件已存在，必须是普通文件且无硬链接
if fi.Mode()&os.ModeSymlink != 0 {
return fmt.Errorf("file %q is a symlink, refusing to write", filename)
}
if !fi.Mode().IsRegular() {
return fmt.Errorf("file %q is not a regular file", filename)
}
// 检查硬链接数，防止硬链接截断攻击
stat, ok := fi.Sys().(*unix.Stat_t)
if !ok {
return fmt.Errorf("file %q failed to get underlying stat info", filename)
}
if stat.Nlink > 1 {
return fmt.Errorf("file %q has hard links (%d), refusing to truncate", filename, stat.Nlink)
}
f, err := os.OpenFile(filename, unix.O_WRONLY|unix.O_TRUNC|unix.O_NOFOLLOW, perm)
if err != nil {
return err
}
defer f.Close()
_, err = f.Write(content)
return err
}
// 文件不存在，安全创建
f, err := os.OpenFile(filename, unix.O_WRONLY|unix.O_CREAT|unix.O_EXCL, perm)
if err != nil {
return err
}
defer f.Close()
_, err = f.Write(content)
return err
} |
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.
Log: Optimized power button logic to avoid redundant screen-off commands
Influence:
fix: 如果屏幕已处于DPMS关闭状态则跳过屏幕关闭处理
Log: 优化电源键逻辑，避免重复发送屏幕关闭命令
Influence:
PMS: BUG-364835
Summary by Sourcery
Skip redundant DPMS screen-off operations when the display is already off and align suspend state constants ordering
Bug Fixes:
Enhancements: