backport: refactor: reimplement unescapeExec #263
Closed
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
adjusted patch from master branch (originally 2f0f34a) Signed-off-by: ComixHe <heyuming@deepin.org>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia 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
Author
|
/integrate |
deepin pr auto review关键摘要:
是否建议立即修改: |
Reviewer's Guide by SourceryThis pull request refactors the Sequence diagram for unescapeExec with resource processingsequenceDiagram
participant AS as ApplicationService
participant UE as unescapeExec
participant PU as processUrl
participant QProcess as QProcess
AS->UE: unescapeExec(str, fields)
UE->UE: Parse str for field codes (%f, %u, %F, %U)
alt Field code found
UE->PU: processUrl(field)
PU->PU: Check if URL is valid and local
alt URL is valid and local
PU-->UE: Return local file path
else URL is invalid or remote
PU-->UE: Return original URL
end
UE->UE: Append processed resource to command
end
UE-->AS: Return LaunchTask
AS->QProcess: start(LaunchBin, command)
QProcess-->AS: Exit code
AS-->AS: Handle exit code
Updated class diagram for ApplicationServiceclassDiagram
class ApplicationService {
-m_applicationPath: QDBusObjectPath
-m_desktopSource: DesktopSource
-m_launcher: QString
+Launch(action: QString, fields: QStringList, res: QVariant) : QVariant
+unescapeExec(str: QString, fields: QStringList) : LaunchTask
+unescapeExecArgs(str: QString) : std::optional<QStringList>
+unescapeEens(options: QVariantMap) : void
+autoRemoveFromDesktop() : void
+findEntryValue(group: QString, valueKey: QString, type: EntryValueType, locale: QLocale) : QVariant
}
class LaunchTask {
+LaunchBin: QString
+command: QStringList
+Resources: QStringList
}
ApplicationService -- LaunchTask : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
AutoIntegrationPr Bot |
|
TAG Bot TAG: 1.2.16.1 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @BLumia - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes to
unescapeExecare quite large; could you add some more comments to explain the logic, especially around the field code handling? - Consider using
QStringViewinstead ofQStringfor parsing inunescapeExecto avoid unnecessary string copies.
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.
| m_applicationPath.path(), | ||
| [this, binary = std::move(bin), commands = std::move(cmds)](const QVariant &variantValue) -> QVariant { | ||
| auto resourceFile = variantValue.toString(); | ||
| [this, binary = std::move(bin), commands = std::move(cmds)](const QVariant &value) -> QVariant { |
There was a problem hiding this comment.
issue (complexity): Consider extracting resource replacement and command adjustment logic into helper functions to improve the lambda's readability and maintainability, and refactor nested loops or string operations into dedicated functions to keep the lambda concise while maintaining functionality .
Consider extracting the embedded resource-replacement and command‐adjustment logic into small helper functions so that the lambda stays focused. For example, you might create a helper like this:
QStringList processResourceInsertion(QStringList cmds, const QString &resource) {
auto it = std::find_if(cmds.begin(), cmds.end(), [](const QString &cmd) {
return cmd.contains("%f") || cmd.contains("%F");
});
if (it != cmds.end()) {
auto index = (it->contains("%f") ? it->indexOf("%f") : it->indexOf("%F"));
// Replace the placeholder with resource (adjust length accordingly)
it->replace(index, 2, resource);
} else {
qCritical() << "internal logic error, can't find %f or %F in exec command, abort.";
}
return cmds;
}Then, in your lambda, simply call this helper:
// Existing setup of newCommands and determination of resource
newCommands = processResourceInsertion(newCommands, resourceFile);Similarly, consider refactoring any nested loops or string operations (for handling %f/%F/%u types) into dedicated functions. This keeps the lambda concise while maintaining all functionality.
Member
Author
|
backport 不足够修复遇到的问题，决定使用主干并在主干修复。见：#264 |
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.
Summary by Sourcery
Refactor and improve the unescapeExec method in the ApplicationService class, enhancing the handling of desktop entry launch commands and field codes
New Features:
Enhancements:
Chores: