fix: Resolve the issue where the shortcut key for selected desktop fi…#3008
Open
Fire-dtx wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: Resolve the issue where the shortcut key for selected desktop fi…#3008Fire-dtx wants to merge 1 commit intolinuxdeepin:masterfrom
Fire-dtx wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Fire-dtx 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 GuideAdds a helper in KeyboardController to detect when a custom shortcut command targets a .desktop file and, if the file exists, automatically prefixes the command with Sequence diagram for adding a custom shortcut with desktop command detectionsequenceDiagram
actor User
participant KeyboardController
participant KeyboardWorker
participant QFileInfo
User->>KeyboardController: addCustomShortcut(name, cmd, accels)
KeyboardController->>KeyboardWorker: onDisableShortcut(conflict) (for each conflict)
KeyboardController->>KeyboardController: checkDesktopCmd(cmd)
KeyboardController->>QFileInfo: QFileInfo(cmd)
QFileInfo-->>KeyboardController: exists()
alt desktop file exists and path or .desktop detected
KeyboardController-->>KeyboardController: newCmd = dde-am + cmd
else
KeyboardController-->>KeyboardController: newCmd = cmd
end
KeyboardController->>KeyboardWorker: addCustomShortcut(name, newCmd, accels)
Class diagram for updated KeyboardController desktop command handlingclassDiagram
class KeyboardController {
+formatKeys(shortcuts QString) QStringList
+addCustomShortcut(name QString, cmd QString, accels QString) void
+modifyCustomShortcut(id QString, name QString, cmd QString, accels QString) void
+keyboardEnabledChanged() void
-checkDesktopCmd(cmd QString) QString
-m_repeatInterval uint
-m_repeatDelay uint
-m_numLock bool
}
class KeyboardWorker {
+onDisableShortcut(conflict QString) void
+addCustomShortcut(name QString, cmd QString, accels QString) void
+modifyCustomShortcut(shortcut ShortcutPtr) void
}
class Shortcut {
+id QString
+name QString
+command QString
+accels QString
}
KeyboardController --> KeyboardWorker : uses
KeyboardController --> Shortcut : modifies
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 2 issues, and left some high level feedback:
- The
checkDesktopCmdlogic usesQFileInfoon the fullcmdstring, which will fail if arguments are appended to the desktop file path (e.g./usr/share/applications/foo.desktop --option); consider parsing out the actual path before checkingexists(). - Relying on
QFileInfo::exists()for commands that only contain a.desktopfilename (without a full or relative path) likely won't detect desktop files in standard application directories; you may want to resolve such names against known desktop file locations before deciding whether to prependdde-am.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `checkDesktopCmd` logic uses `QFileInfo` on the full `cmd` string, which will fail if arguments are appended to the desktop file path (e.g. `/usr/share/applications/foo.desktop --option`); consider parsing out the actual path before checking `exists()`.
- Relying on `QFileInfo::exists()` for commands that only contain a `.desktop` filename (without a full or relative path) likely won't detect desktop files in standard application directories; you may want to resolve such names against known desktop file locations before deciding whether to prepend `dde-am`.
## Individual Comments
### Comment 1
<location> `src/plugin-keyboard/operation/keyboardcontroller.cpp:232-242` </location>
<code_context>
+QString KeyboardController::checkDesktopCmd(const QString &cmd)
+{
+ // 检查并处理desktop命令，如果desktop文件存在则添加dde-am前缀
+ if (!cmd.isEmpty() && (cmd.startsWith("/") || cmd.contains(".desktop"))) {
+ QFileInfo fileInfo(cmd);
+ if (fileInfo.exists()) {
</code_context>
<issue_to_address>
**suggestion:** Trim the command string before checks to avoid whitespace-related misdetection
Leading/trailing spaces in `cmd` (e.g. from copy-paste) will cause `startsWith("/")` to fail and `QFileInfo` to look up a path with whitespace, so an existing desktop file may not be detected. Consider `auto trimmed = cmd.trimmed();` and performing the checks on `trimmed` (and returning `trimmed` or the original as appropriate) to handle this case reliably.
```suggestion
QString KeyboardController::checkDesktopCmd(const QString &cmd)
{
// 检查并处理desktop命令，如果desktop文件存在则添加dde-am前缀
const auto trimmed = cmd.trimmed();
if (!trimmed.isEmpty() && (trimmed.startsWith("/") || trimmed.contains(".desktop"))) {
QFileInfo fileInfo(trimmed);
if (fileInfo.exists()) {
return "dde-am " + trimmed;
}
}
return cmd;
}
```
</issue_to_address>
### Comment 2
<location> `src/plugin-keyboard/operation/keyboardcontroller.cpp:236-237` </location>
<code_context>
+{
+ // 检查并处理desktop命令，如果desktop文件存在则添加dde-am前缀
+ if (!cmd.isEmpty() && (cmd.startsWith("/") || cmd.contains(".desktop"))) {
+ QFileInfo fileInfo(cmd);
+ if (fileInfo.exists()) {
+ return "dde-am " + cmd;
+ }
</code_context>
<issue_to_address>
**suggestion:** Limit `.desktop` existence checks to the command token rather than the full string
Because `QFileInfo` is constructed with the full `cmd` string, any arguments or shell wrappers (e.g. `"/usr/share/applications/foo.desktop --flag"`, `"env FOO=1 foo.desktop"`) will cause `exists()` to fail even when the underlying `.desktop` file is present. Extract the executable/desktop-file token (up to the first whitespace) and pass only that to `QFileInfo` so the existence check reflects the actual desktop file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Fire-dtx
force-pushed
the
master
branch
3 times, most recently
from
de2a6e8 to
784defc
Compare
…les does not work Optimizing: Missing the dde-am parameter Log: When detecting a desktop file during command execution, automatically prepend the dde-am parameter to the command pms: BUG-337781
deepin pr auto review这段代码主要实现了在添加或修改自定义快捷键时，自动检测 1. 语法逻辑审查
2. 代码质量审查
3. 代码性能审查
4. 代码安全审查
5. 改进后的代码建议以下是综合了上述建议的改进代码片段： // keyboardcontroller.h
private:
// 处理desktop命令，如果desktop文件存在则添加dde-am前缀
QString processDesktopCmd(const QString &cmd);
void disableConflictShortcut(const QString &accels); // 提取公共逻辑
static const QString DESKTOP_CMD_PREFIX;
uint m_repeatInterval;
// ... 其他成员// keyboardcontroller.cpp
const QString KeyboardController::DESKTOP_CMD_PREFIX = "dde-am ";
QString KeyboardController::processDesktopCmd(const QString &cmd)
{
// 检查是否为绝对路径且以.desktop结尾
if (!cmd.isEmpty() && cmd.startsWith("/") && cmd.endsWith(".desktop")) {
QFileInfo fileInfo(cmd);
// exists() 会自动处理符号链接的目标检查
if (fileInfo.exists()) {
// 注意：这里假设 m_worker 能安全处理包含空格的路径。
// 如果是通过 shell 执行，建议对 cmd 进行转义或使用引号包裹。
return DESKTOP_CMD_PREFIX + cmd;
}
}
return cmd;
}
void KeyboardController::disableConflictShortcut(const QString &accels)
{
if (auto conflict = m_shortcutModel->getInfo(accels)) {
m_worker->onDisableShortcut(conflict);
}
}
void KeyboardController::addCustomShortcut(const QString &name, const QString &cmd, const QString &accels)
{
// 清理冲突快捷键
disableConflictShortcut(accels);
// 处理命令前缀
QString newCmd = processDesktopCmd(cmd);
m_worker->addCustomShortcut(name, newCmd, accels);
}
void KeyboardController::modifyCustomShortcut(const QString &id, const QString &name, const QString &cmd, const QString &accels)
{
auto shortcut = m_shortcutModel->getCustomInfo(id);
if (!shortcut) {
return;
}
// 清理冲突快捷键
disableConflictShortcut(accels);
shortcut->name = name;
shortcut->command = processDesktopCmd(cmd);
shortcut->accels = accels;
m_worker->modifyCustomShortcut(shortcut);
// ...
}总结这段代码实现了预期的功能，逻辑清晰。主要改进点在于：
|
|
TAG Bot New tag: 6.1.72 |
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.
Optimizing: Missing the dde-am parameter
Log: When detecting a desktop file during command execution, automatically prepend the dde-am parameter to the command
pms: BUG-337781
Summary by Sourcery
Bug Fixes: