fix: optimize shortcut launch performance by replacing subprocess spawning with in-process handlers#1118
Open
robertkill wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: robertkill 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 GuideReplaces slow, subprocess-based implementations of system shortcuts with in-process actions using GIO/GSettings and direct D-Bus calls, enabled via new action types and an optional customAction on SystemShortcut, to significantly improve shortcut launch performance. Sequence diagram for in-process fileManager launch shortcutsequenceDiagram
actor User
participant Manager
participant SystemShortcut
participant gio
User->>Manager: KeyEvent (Win+E)
Manager->>SystemShortcut: GetAction()
SystemShortcut-->>Manager: Action{Type:ActionTypeLaunchMimeType, Arg:mimeType}
Manager->>Manager: handlers[ActionTypeLaunchMimeType](ev)
Manager->>gio: AppInfoGetDefaultForType(mimeType, false)
gio-->>Manager: AppInfo
Manager->>gio: ToDesktopAppInfo(AppInfo)
gio-->>Manager: DesktopAppInfo
Manager->>Manager: runDesktopFile(desktopFile)
Manager-->>User: Default file manager window opened
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 the lock screen handler, consider replacing the
exec.Command("sh", "-c", "setxkbmap -query | grep ...")shell pipeline with directexec.Command("setxkbmap", "-query")plus Go-side parsing to avoid relying on the shell and external text utilities. - For the terminal launch handler, it may be worth explicitly handling the case where
settings.GetString(gsKeyTerminalAppId)returns an empty string (e.g., log and return early) to avoid passing an invalid desktop file ID intorunDesktopFile.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the lock screen handler, consider replacing the `exec.Command("sh", "-c", "setxkbmap -query | grep ...")` shell pipeline with direct `exec.Command("setxkbmap", "-query")` plus Go-side parsing to avoid relying on the shell and external text utilities.
- For the terminal launch handler, it may be worth explicitly handling the case where `settings.GetString(gsKeyTerminalAppId)` returns an empty string (e.g., log and return early) to avoid passing an invalid desktop file ID into `runDesktopFile`.
## Individual Comments
### Comment 1
<location path="keybinding1/manager_handlers.go" line_range="201" />
<code_context>
+ }()
+ }
+
+ m.handlers[ActionTypeLockScreen] = func(ev *KeyEvent) {
+ go func() {
+ // 1. 获取当前 keyboard option
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new lock-screen and app-launch logic from `initHandlers` into small helper methods and replacing the shell pipeline with Go parsing to keep the function focused on simple wiring.
You can keep all the new functionality while reducing complexity in `initHandlers` by:
### 1. Extracting the lock screen flow into helpers
Move the multi-step script from the inline goroutine into small helper methods on `Manager`. This makes `initHandlers` just wiring, and keeps responsibilities separated.
**Before (simplified):**
```go
m.handlers[ActionTypeLockScreen] = func(ev *KeyEvent) {
go func() {
out, err := exec.Command("sh", "-c",
"setxkbmap -query | grep option | awk -F ' ' '{print $2}'").Output()
origOption := strings.TrimSpace(string(out))
logger.Debug("lockScreen: orig option:", origOption)
exec.Command("setxkbmap", "-option", "grab:break_actions").Run()
if !_useWayland {
exec.Command("xdotool", "key", "XF86Ungrab").Run()
}
lockObj := m.sessionSigLoop.Conn().Object(
"org.deepin.dde.LockFront1",
"/org/deepin/dde/LockFront1")
err = lockObj.Call("org.deepin.dde.LockFront1.Show", 0).Err
if err != nil {
logger.Warning("lockScreen: failed to show lock front:", err)
}
if origOption != "" {
exec.Command("setxkbmap", "-option", origOption).Run()
}
}()
}
```
**After (example refactor):**
```go
m.handlers[ActionTypeLockScreen] = func(ev *KeyEvent) {
go m.lockScreen()
}
```
With helpers like:
```go
func (m *Manager) lockScreen() {
origOption := m.getCurrentXkbOption()
logger.Debug("lockScreen: orig option:", origOption)
m.breakXGrab()
defer m.restoreXkbOption(origOption)
m.showLockFront()
}
func (m *Manager) breakXGrab() {
exec.Command("setxkbmap", "-option", "grab:break_actions").Run()
if !_useWayland {
exec.Command("xdotool", "key", "XF86Ungrab").Run()
}
}
func (m *Manager) restoreXkbOption(orig string) {
if orig != "" {
exec.Command("setxkbmap", "-option", orig).Run()
}
}
func (m *Manager) showLockFront() {
lockObj := m.sessionSigLoop.Conn().Object(
"org.deepin.dde.LockFront1",
"/org/deepin/dde/LockFront1")
if err := lockObj.Call("org.deepin.dde.LockFront1.Show", 0).Err; err != nil {
logger.Warning("lockScreen: failed to show lock front:", err)
}
}
```
### 2. Replace the shell pipeline with simple Go parsing
The `sh | grep | awk` pipeline is brittle and harder to read/test than a small parsing helper:
**Before:**
```go
out, err := exec.Command("sh", "-c",
"setxkbmap -query | grep option | awk -F ' ' '{print $2}'").Output()
origOption := strings.TrimSpace(string(out))
```
**After:**
```go
func (m *Manager) getCurrentXkbOption() string {
out, err := exec.Command("setxkbmap", "-query").Output()
if err != nil {
logger.Warning("lockScreen: setxkbmap -query failed:", err)
return ""
}
for _, line := range strings.Split(string(out), "\n") {
line = strings.TrimSpace(line)
if strings.HasPrefix(line, "options:") {
// "options: grp:alt_shift_toggle"
fields := strings.Fields(line)
if len(fields) >= 2 {
return fields[1]
}
}
}
return ""
}
```
This keeps the behavior (read current option, ignore errors by returning empty string) but makes it explicit and testable without invoking a shell.
### 3. Keep `initHandlers` focused on wiring for new actions
You can similarly extract the logic for launching default apps so `initHandlers` only wires actions:
**Before:**
```go
m.handlers[ActionTypeLaunchMimeType] = func(ev *KeyEvent) {
action := ev.Shortcut.GetAction()
mimeType, ok := action.Arg.(string)
if !ok {
logger.Warning(ErrTypeAssertionFail)
return
}
go func() {
appInfo := gio.AppInfoGetDefaultForType(mimeType, false)
if appInfo == nil {
logger.Warning("no default app for mime type:", mimeType)
return
}
defer appInfo.Unref()
dAppInfo := gio.ToDesktopAppInfo(appInfo)
desktopFile := filepath.Base(dAppInfo.GetFilename())
err := m.runDesktopFile(desktopFile)
if err != nil {
logger.Warning("launch mime type error:", err)
}
}()
}
```
**After:**
```go
m.handlers[ActionTypeLaunchMimeType] = func(ev *KeyEvent) {
action := ev.Shortcut.GetAction()
mimeType, ok := action.Arg.(string)
if !ok {
logger.Warning(ErrTypeAssertionFail)
return
}
go m.launchDefaultForMimeType(mimeType)
}
```
```go
func (m *Manager) launchDefaultForMimeType(mimeType string) {
appInfo := gio.AppInfoGetDefaultForType(mimeType, false)
if appInfo == nil {
logger.Warning("no default app for mime type:", mimeType)
return
}
defer appInfo.Unref()
dAppInfo := gio.ToDesktopAppInfo(appInfo)
desktopFile := filepath.Base(dAppInfo.GetFilename())
if err := m.runDesktopFile(desktopFile); err != nil {
logger.Warning("launch mime type error:", err)
}
}
```
And for the terminal:
```go
m.handlers[ActionTypeLaunchTerminal] = func(ev *KeyEvent) {
go m.launchDefaultTerminal()
}
func (m *Manager) launchDefaultTerminal() {
settings := gio.NewSettings(gsSchemaDefaultTerminal)
if settings == nil {
logger.Warning("failed to get terminal settings")
return
}
defer settings.Unref()
appId := settings.GetString(gsKeyTerminalAppId)
if err := m.runDesktopFile(appId); err != nil {
logger.Warning("launch terminal error:", err)
}
}
```
These changes keep all behavior intact but reduce complexity in `initHandlers`, isolate concerns, and make the new logic easier to test and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
robertkill
force-pushed
the
fix-361803-optimize-shortcut-launch
branch
from
327a517 to
ca45cf0
Compare
deepin pr auto review这份代码的主要目标是将部分快捷键（文件管理器、终端、锁屏）的处理方式从“派生子进程”重构为“进程内调用（in-process）”，以提升性能和安全性。整体思路清晰，但在代码健壮性、安全性、错误处理和并发控制方面存在一些需要改进的地方。 以下是对该 Git Diff 的详细代码审查意见： 1. 代码安全
2. 代码逻辑与健壮性
3. 代码性能
4. 代码规范与风格
改进后的代码建议针对上述主要问题，以下是修改后的关键代码片段：
func (m *Manager) launchDefaultForMimeType(mimeType string) {
appInfo := gio.AppInfoGetDefaultForType(mimeType, false)
if appInfo == nil {
logger.Warning("no default app for mime type:", mimeType)
return
}
defer appInfo.Unref()
dAppInfo := gio.ToDesktopAppInfo(appInfo)
if dAppInfo == nil {
logger.Warning("failed to cast AppInfo to DesktopAppInfo for mime type:", mimeType)
return
}
desktopFile := filepath.Base(dAppInfo.GetFilename())
err := m.runDesktopFile(desktopFile)
if err != nil {
logger.Warning("launch mime type error:", err)
}
}
func (m *Manager) breakXGrab() {
err := exec.Command("setxkbmap", "-option", "grab:break_actions").Run()
if err != nil {
logger.Warning("breakXGrab: setxkbmap failed:", err)
}
if !_useWayland {
err = exec.Command("xdotool", "key", "XF86Ungrab").Run()
if err != nil {
logger.Warning("breakXGrab: xdotool failed:", err)
}
}
}
func (m *Manager) restoreXkbOption(orig string) {
if orig != "" {
err := exec.Command("setxkbmap", "-option", orig).Run()
if err != nil {
logger.Warning("restoreXkbOption: setxkbmap failed:", err)
}
}
}
func (sm *ShortcutManager) AddSystemById(wmObj wm.Wm, id string) {
// ... 前置代码 ...
// 使用 switch 优化逻辑
switch id {
case "fileManager":
// fileManager 走 in-process AM Launch，不 spawn 子进程
sysShortcut.customAction = NewLaunchMimeTypeAction("inode/directory")
case "terminal":
// terminal 走 in-process AM Launch，不 spawn 子进程
sysShortcut.customAction = NewLaunchTerminalAction()
case "lockScreen", "lockScreen-wayland":
// lockScreen 走 in-process DBus，不 spawn dbus-send
sysShortcut.customAction = NewLockScreenAction()
}
sm.addWithoutLock(sysShortcut)
}
func (ss *SystemShortcut) SetAction(newAction *Action) error {
if newAction == nil {
return ErrNilAction
}
// 建议增加对允许的 Action 类型的白名单校验
allowedTypes := map[ActionType]bool{
ActionTypeExecCmd: true,
ActionTypeLaunchMimeType: true,
ActionTypeLaunchTerminal: true,
ActionTypeLockScreen: true,
// 其他允许的类型...
}
if !allowedTypes[newAction.Type] {
return ErrInvalidActionType
}
if newAction.Type == ActionTypeExecCmd {
arg, ok := newAction.Arg.(*ActionExecCmdArg)
if !ok {
return ErrTypeAssertionFail
}
ss.arg = arg
ss.customAction = nil
} else {
ss.customAction = newAction
// 清理旧的 arg，避免内存泄漏或状态不一致
ss.arg = nil
}
return nil
} |
robertkill
force-pushed
the
fix-361803-optimize-shortcut-launch
branch
from
ca45cf0 to
16c10f7
Compare
…wning with in-process handlers 1. Add ActionTypeLaunchMimeType handler: use GIO AppInfoGetDefaultForType + AM Launch in-process, removes default-file-manager Go binary subprocess 2. Add ActionTypeLaunchTerminal handler: use GSettings to read terminal app-id + AM Launch in-process, removes default-terminal Go binary subprocess 3. Add ActionTypeLockScreen handler: in-process DBus LockFront1.Show + X11 grab cleanup, removes dbus-send subprocess from shell pipeline 4. Add customAction field to SystemShortcut: allows per-shortcut override of GetAction() return value without changing storage/serialization Influence: 1. fileManager shortcut (Win+E): ~1,011ms -> ~176ms 2. terminal shortcut (Ctrl+Alt+T): ~678ms -> ~608ms 3. lockScreen shortcut (Win+L): shell pipeline eliminated, DBus call now in-process fix: 优化快捷键启动性能，用 in-process handler 替换子进程 1. 新增 ActionTypeLaunchMimeType handler：GIO 查 mime + AM Launch 进程内完成，消除 default-file-manager Go 子进程 2. 新增 ActionTypeLaunchTerminal handler：GSettings 读终端配置 + AM Launch 进程内完成，消除 default-terminal Go 子进程 3. 新增 ActionTypeLockScreen handler：in-process DBus LockFront1.Show + X11 抓取清理，消除 shell 链中的 dbus-send 子进程 4. SystemShortcut 新增 customAction 字段：允许按快捷键覆写 GetAction() 返回值，不改存储/序列化 Influence: 1. fileManager 快捷键 (Win+E)：~1,011ms -> ~176ms 2. terminal 快捷键 (Ctrl+Alt+T)：~678ms -> ~608ms 3. lockScreen 快捷键 (Win+L)：消除 shell 链，DBus 调用改为进程内直连 PMS: BUG-361803
robertkill
force-pushed
the
fix-361803-optimize-shortcut-launch
branch
from
16c10f7 to
4e187eb
Compare
Contributor
|
TAG Bot New tag: 6.1.91 |
Contributor
|
TAG Bot New tag: 6.1.92 |
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.
Summary
Fix BUG-361803: shortcut-triggered app launches are significantly slower than clicking on the dock.
Root cause: The keybinding daemon spawned separate Go binaries (
default-file-manager,default-terminal) and complex shell pipelines (lockScreen) as subprocesses for certain system shortcuts. Go binary startup (~200-300ms runtime init) and subprocess overhead caused 500-900ms extra delay compared to the click path.Fix: Replace subprocess spawning with in-process handlers that use the daemon's existing session bus and GIO/GSettings APIs directly.
Changes
AppInfoGetDefaultForType→runDesktopFile()(AM Launch in-process). Removesdefault-file-managerGo binary subprocess.com.deepin.desktop.default-applications.terminal→runDesktopFile()(AM Launch in-process). Removesdefault-terminalGo binary subprocess.setxkbmap+xdotool) + in-process DBusLockFront1.Show. Removesdbus-sendsubprocess from shell pipeline.GetAction()override without changing storage or serialization.Performance
Modified Files
keybinding1/shortcuts/action.go— 3 new ActionType constants + factory functionskeybinding1/shortcuts/system_shortcut.go—customActionfield on SystemShortcutkeybinding1/shortcuts/shortcut_manager.go— Apply customAction for fileManager/terminal/lockScreenkeybinding1/manager_handlers.go— 3 new handlers for the new ActionTypesPMS: BUG-361803
Summary by Sourcery
Optimize shortcut-triggered app launches by handling key system shortcuts in-process instead of spawning external subprocesses.
New Features:
Bug Fixes:
Enhancements: