fix: convert file:// to local path #264
Merged
Conversation
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
Reviewer's Guide by SourceryThis pull request modifies how the application service handles file paths and URLs passed to applications launched via desktop entries. It converts Sequence diagram for launching application with local file path conversionsequenceDiagram
participant ApplicationService
participant LaunchTask
participant QUrl
ApplicationService->>LaunchTask: unescapeExec(str, fields)
activate LaunchTask
LaunchTask->>LaunchTask: Process %F or %f
LaunchTask->>QUrl: QUrl::fromUserInput(str)
activate QUrl
QUrl-->>LaunchTask: url
deactivate QUrl
LaunchTask->>LaunchTask: url.isLocalFile()
alt isLocalFile() == true
LaunchTask->>LaunchTask: str = url.toLocalFile()
end
LaunchTask-->>ApplicationService: task
deactivate LaunchTask
ApplicationService->>ApplicationService: Launch(action, fields, task)
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 @ComixHe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding some comments to explain the logic within the
std::for_eachlambda inApplicationService::Launch. - It might be helpful to introduce some more descriptive variable names to improve readability.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }); | ||
| } | ||
|
|
||
| auto newCmds = task.command; |
There was a problem hiding this comment.
issue (complexity): Consider replacing lambdas and reverse-iterator insertions with explicit loops to improve readability and maintainability of the code.
Consider replacing these lambdas and reverse-iterator insertions with explicit, simple loops. For example, instead of this:
std::for_each(
rawRes.rbegin(), rawRes.rend(),
[&newCmds, &it](QString &str) { it = newCmds.insert(it, str); }
);
you could simplify it to a clear reverse loop:
if (task.fieldLocation == -1) {
for (int i = rawRes.size() - 1; i >= 0; --i) {
newCmds.insert(task.argNum + 1, rawRes.at(i));
}
} else {
auto arg = newCmds.begin() + task.argNum + 1;
arg->insert(task.fieldLocation, rawRes.takeFirst());
++arg;
for (int i = rawRes.size() - 1; i >= 0; --i) {
newCmds.insert(arg, rawRes.at(i));
}
}
Similarly, for URL processing, replace the lambda with a range-based loop:
if (task.local) {
for (auto &str : rawRes) {
QUrl url = QUrl::fromUserInput(str);
if (url.isLocalFile()) {
str = url.toLocalFile();
}
// Optionally handle remote files in a separate function.
}
}
These changes preserve functionality while reducing layered abstractions and nested lambdas, improving readability and maintainability.
BLumia
previously approved these changes
Apr 10, 2025
|
TAG Bot TAG: 1.2.27 |
passing local path to application instead of url when desktop Exec specify %F or %f. other change: - respect the original url, passing it to application directly - correct the application command generating process Signed-off-by: ComixHe <heyuming@deepin.org>
Signed-off-by: ComixHe <heyuming@deepin.org>
deepin pr auto review代码审查意见：
|
BLumia
approved these changes
Apr 10, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, ComixHe 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 |
Member
|
/integrate |
|
AutoIntegrationPr Bot |
This was referenced Apr 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
passing local path to application instead of url when desktop Exec specify %F or %f.
other change:
Summary by Sourcery
Improve handling of file URLs and local paths when launching applications through desktop entry files
Bug Fixes:
Enhancements: