feat: update dde-am tool#280
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
Adds enhanced command-line functionality to dde-am for launching applications using app IDs, desktop file paths, or URIs, with support for environment variables and improved logging.
- Introduces new CLI options (
-d/--desktop-path,-u/--uri,-e/--env) - Implements
getAppIdFromURIhelper and fallback logic - Extends
Launcherto accept and forward environment variables
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/dde-am-usage.md | Updates usage guide to include new options and examples |
| apps/dde-am/src/main.cpp | Adds URI/desktop-path handling, env option parsing, and getAppIdFromURI |
| apps/dde-am/src/launcher.h | Declares setEnvironmentVariables |
| apps/dde-am/src/launcher.cpp | Passes environment variables into D-Bus call options |
Comments suppressed due to low confidence (2)
apps/dde-am/src/main.cpp:18
- New function
getAppIdFromURIlacks unit tests; adding tests for valid URIs, non-file URIs, and missing files will ensure correct behavior.
QString getAppIdFromURI(const QString &uri)
apps/dde-am/src/launcher.h:19
- [nitpick] Public method
setEnvironmentVariableslacks a docstring; adding a brief description will improve API documentation and clarify expected input format.
void setEnvironmentVariables(const QStringList &envVars);
apps/dde-am/src/main.cpp
Outdated
| if (appId.isEmpty() && parser.value(desktopPathOption).endsWith(".desktop")) { | ||
| QFileInfo fileInfo(parser.value(desktopPathOption)); | ||
| appId = fileInfo.baseName(); // Use filename without .desktop suffix |
There was a problem hiding this comment.
[nitpick] Duplicate fallback logic for deriving app IDs from .desktop paths appears in multiple branches; consider extracting this into a shared helper function to reduce duplication.
| if (appId.isEmpty() && parser.value(desktopPathOption).endsWith(".desktop")) { | |
| QFileInfo fileInfo(parser.value(desktopPathOption)); | |
| appId = fileInfo.baseName(); // Use filename without .desktop suffix | |
| if (appId.isEmpty()) { | |
| appId = getAppIdFromDesktopPathFallback(parser.value(desktopPathOption)); |
deepin pr auto review代码审查意见：
综上所述，代码在功能实现上基本正确，但在代码质量、安全性和可维护性方面还有改进的空间。 |
|
TAG Bot New tag: 1.2.32 |
apps/dde-am/src/main.cpp
Outdated
| } | ||
|
|
||
| // Handle legacy environment variable as third argument | ||
| if (!arguments.isEmpty() && !parser.isSet(envOption)) { |
There was a problem hiding this comment.
这个地方的arguments是位置参数，不是选项参数，应该不需要判断arguments.isEmpty()，envVars也不是从这里获取的，
BLumia
left a comment
BLumia
left a comment
There was a problem hiding this comment.
还是兼容一下之前以第二个位置参数作为 action 的场景吧。两三行的事
dde-am support env now! Log:
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, xzl01 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 |
|
备注一下私下沟通的关于后续潜在支持传递参数给程序的需求的结论： 根据惯用做法，使用 $ dde-am deepin-editor -e LANG=zh_CN -- file:///path/to/textfile1.txt file:///path/to/textfile2.txt
$ dde-am firefox -- https://www.bing.com |
Add am-open tool for launching applications via command line
Log:
Summary by Sourcery
Add a new command-line tool 'am-open' for launching desktop applications via the ApplicationManager D-Bus interface.
New Features:
Enhancements:
Build: