fix: auto-restart fcitx5 in ProcessMonitor constructor#125
Merged
wyu71 merged 1 commit intolinuxdeepin:masterfrom Jan 4, 2026
Merged
fix: auto-restart fcitx5 in ProcessMonitor constructor#125wyu71 merged 1 commit intolinuxdeepin:masterfrom
wyu71 merged 1 commit intolinuxdeepin:masterfrom
Conversation
- Kill existing fcitx5 processes for current user before starting - Start fcitx5 in detached mode during constructor Log: auto-restart fcitx5 in ProcessMonitor constructor pms: BUG-339241
|
TAG Bot TAG: 6.0.23 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe ProcessMonitor constructor now proactively restarts fcitx5 by killing any existing fcitx5 processes for the current user and launching a new detached fcitx5 instance, with logging around these actions. Sequence diagram for ProcessMonitor constructor auto-restart of fcitx5sequenceDiagram
participant Caller
participant ProcessMonitor
participant QTimer
participant exeCommand
participant QProcess
Caller->>ProcessMonitor: constructor(parent)
activate ProcessMonitor
ProcessMonitor->>QTimer: connect(m_timer.timeout, checkFcitx5Process)
ProcessMonitor->>exeCommand: exeCommand(pgrep, [-u, getCurrentUserName(), -x, fcitx5], output, error)
exeCommand-->>ProcessMonitor: output, error
alt fcitx5 processes found
ProcessMonitor->>ProcessMonitor: pids = output.trimmed().split('\n')
loop for each pid in pids
ProcessMonitor->>exeCommand: exeCommand(kill, [pid.trimmed()], killOutput, killError)
exeCommand-->>ProcessMonitor: killOutput, killError
end
end
ProcessMonitor->>QProcess: startDetached(PROCESS_NAME, [-d])
QProcess-->>ProcessMonitor: started
alt started is true
ProcessMonitor->>ProcessMonitor: log success
else started is false
ProcessMonitor->>ProcessMonitor: log failure
end
deactivate ProcessMonitor
Class diagram for updated ProcessMonitor constructor behaviorclassDiagram
class ProcessMonitor {
- QTimer m_timer
+ ProcessMonitor(parent: QObject*)
+ startMonitoring() void
+ checkFcitx5Process() void
}
class QTimer {
+ timeout() void
}
class QProcess {
+ startDetached(program: QString, arguments: QStringList) bool
}
class HelperFunctions {
+ exeCommand(command: QString, arguments: QStringList, output: QString, error: QString) void
+ getCurrentUserName() QString
}
ProcessMonitor --> QTimer : uses
ProcessMonitor --> QProcess : uses
ProcessMonitor --> HelperFunctions : uses
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:
- The constructor now performs synchronous
pgrep/killcalls and starts a new process, which may block the main thread during object construction; consider moving this logic to an explicit initialization/start method or a background task instead of doing it in the constructor. - The return values and stderr (
error,killError) fromexeCommandare currently ignored; consider checking and logging failures so that issues withpgreporkillare visible and can be diagnosed. - You can simplify the detached start by using
QProcess::startDetached(PROCESS_NAME, {"-d"})directly instead of creating a localQProcess processinstance that is otherwise unused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The constructor now performs synchronous `pgrep`/`kill` calls and starts a new process, which may block the main thread during object construction; consider moving this logic to an explicit initialization/start method or a background task instead of doing it in the constructor.
- The return values and stderr (`error`, `killError`) from `exeCommand` are currently ignored; consider checking and logging failures so that issues with `pgrep` or `kill` are visible and can be diagnosed.
- You can simplify the detached start by using `QProcess::startDetached(PROCESS_NAME, {"-d"})` directly instead of creating a local `QProcess process` instance that is otherwise unused.
## Individual Comments
### Comment 1
<location> `src/fcitx5helper/processmonitor.cpp:92-99` </location>
<code_context>
+ for (const QString &pid : pids) {
+ qCDebug(procMon) << "Killing existing fcitx5 process with PID:" << pid;
+ QString killOutput, killError;
+ exeCommand("kill", QStringList() << pid.trimmed(), killOutput, killError);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Kill failures and errors are ignored, which may hide problems in process cleanup.
`exeCommand`’s return value and `killOutput` / `killError` are ignored, so a failed `kill` (permissions, process already gone, invalid PID, etc.) is silently treated as success. Please check the result and/or `killError` and log or otherwise handle failures, especially since this is terminating user processes.
```suggestion
if (!output.isEmpty()) {
QStringList pids = output.trimmed().split('\n', Qt::SkipEmptyParts);
for (const QString &pid : pids) {
const QString trimmedPid = pid.trimmed();
qCDebug(procMon) << "Killing existing fcitx5 process with PID:" << trimmedPid;
QString killOutput, killError;
const bool killSucceeded =
exeCommand("kill", QStringList() << trimmedPid, killOutput, killError);
if (!killSucceeded || !killError.trimmed().isEmpty()) {
qCWarning(procMon)
<< "Failed to kill existing fcitx5 process"
<< "PID:" << trimmedPid
<< "success:" << killSucceeded
<< "stderr:" << killError.trimmed()
<< "stdout:" << killOutput.trimmed();
}
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
deepin pr auto review我来对这个diff进行详细的代码审查：
安全性问题：
建议改进： // 在kill之前验证进程
exeCommand("ps", QStringList() << "-p" << pid.trimmed() << "-o" << "comm=", output, error);
if (output.trimmed() == "fcitx5") {
exeCommand("kill", QStringList() << pid.trimmed(), killOutput, killError);
}代码质量问题：
建议将这些操作分离到单独的方法中： ProcessMonitor::ProcessMonitor(QObject *parent) : QObject(parent)
{
connect(&m_timer, &QTimer::timeout, this, &ProcessMonitor::checkFcitx5Process);
initializeFcitx5();
}
void ProcessMonitor::initializeFcitx5()
{
terminateExistingFcitx5();
startNewFcitx5();
}性能问题：
建议改进： QProcess process;
process.setProgram("pgrep");
process.setArguments(QStringList() << "-u" << getCurrentUserName() << "-x" << "fcitx5");
process.start();
if (!process.waitForFinished(5000)) { // 设置5秒超时
qCWarning(procMon) << "pgrep command timeout";
process.kill();
return;
}错误处理问题：
建议改进： if (!killError.isEmpty()) {
qCWarning(procMon) << "Failed to kill process" << pid << ":" << killError;
}其他建议：
总的来说，这个修改虽然实现了功能，但在安全性、可靠性和代码结构方面还有改进空间。建议重构代码，提高其健壮性和可维护性。 |
lzwind
approved these changes
Jan 4, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, wyu71 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 |
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
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.
Log: auto-restart fcitx5 in ProcessMonitor constructor
pms: BUG-339241
Summary by Sourcery
Ensure the ProcessMonitor proactively restarts the fcitx5 process when constructed.
Bug Fixes: