Skip to content

Introduce IProcessRunner and migrate actions to centralized process execution; refactor camera capture and face authorizer#13

Open
Programmer-MrWang wants to merge 2 commits intomainfrom
codex/review-code-for-improvement-points
Open

Introduce IProcessRunner and migrate actions to centralized process execution; refactor camera capture and face authorizer#13
Programmer-MrWang wants to merge 2 commits intomainfrom
codex/review-code-for-improvement-points

Conversation

@Programmer-MrWang
Copy link
Owner

Motivation

  • Centralize and standardize external process execution to provide consistent timeout, exit-code handling and structured logging instead of ad-hoc Process.Start calls across actions.
  • Improve robustness of camera capture and face recognition by making the capture loop cancellable, clarifying frame ownership and reducing risk of leaked unmanaged resources.
  • Register a reusable process runner in DI to make actions testable and reduce duplication.
  • Capture code-review findings in a new CODE_REVIEW_REPORT.md to record higher-priority issues and recommendations.

Description

  • Added a new IProcessRunner interface and concrete ProcessRunner implementation (Services/IProcessRunner.cs, Services/ProcessRunner.cs) that run processes with timeouts, expected success exit codes and structured logging, returning a ProcessRunResult.
  • Registered the runner in DI via services.AddSingleton<IProcessRunner, ProcessRunner>(); in Plugin.cs and migrated multiple action classes to accept IProcessRunner and call RunAsync instead of invoking Process.Start directly (examples: CopyAction, MoveAction, DeleteAction, DisableMouseAction, EnableMouseAction, SimulateMouseAction, and the display actions such as CloneDisplayAction, ExtendDisplayAction, ExternalDisplayAction, InternalDisplayAction).
  • Standardized ProcessStartInfo settings (e.g. WindowStyle = ProcessWindowStyle.Hidden) and introduced explicit success exit code handling and longer timeouts for potentially long-running operations (robocopy, rmdir, batch files).
  • Refactored CameraCaptureService to an asynchronous cancellable capture loop (async Task CaptureLoop), use Task.Delay with cancellation, make FrameCaptured invocation safer by creating and passing a managed Mat snapshot and catching subscriber exceptions, and improve stop/dispose handling.
  • Updated FaceRecognitionAuthorizer to clone frames where appropriate, improve exception logging while extracting encodings and during verification cancellation handling, and ensure proper resource disposal around camera/face services.
  • Removed several duplicated local process-handling blocks in favor of the centralized runner and added CODE_REVIEW_REPORT.md containing review findings and recommendations.

Testing

  • No automated tests were run for this change.

Codex Task

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3578b9def

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

private async Task CaptureLoop()
{
using var frame = new Mat();
var token = _cts?.Token ?? CancellationToken.None;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Pass capture token explicitly to prevent uncancellable loop

Avoid falling back to CancellationToken.None here: if Stop() cancels, waits up to 1s, and then sets _cts = null before CaptureLoop actually begins, this line captures None and the while condition can run indefinitely, leaving a background task alive after shutdown/restart. This regression was introduced by switching from _cts?.IsCancellationRequested to a local token fallback; pass the token into CaptureLoop at task creation time so cancellation is always bound to the correct session.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant