feat: add support for passing extra arguments to the SPICE viewer#1860
feat: add support for passing extra arguments to the SPICE viewer#1860lvillani wants to merge 3 commits intoquickemu-project:masterfrom
Conversation
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 4/5
- Mild risk: in
quickemu,--viewer-extra-argsvalues containing spaces will be split in the desktop shortcut Exec line, which can break viewer invocation with multi-word args. - This looks contained to shortcut generation and is a moderate usability issue rather than a core functionality break, so it seems safe to merge with awareness.
- Pay close attention to
quickemu- quote or escape Exec line args to preserve spaces.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="quickemu">
<violation number="1" location="quickemu:2720">
P2: `--viewer-extra-args` values containing spaces will break the generated desktop shortcut because the Exec line is not quoted, so additional viewer args are split into separate CLI options. Quote/escape the value when adding it to `SHORTCUT_OPTIONS` so it stays a single argument in the desktop file.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
2a39d1c to
c9f50eb
Compare
|
I updated commit messages to follow the current style a bit more closely (namely, I added the I'm a bit unsure about the remark from cubic since the feature is implemented nearly the same was as |
c9f50eb to
5663769
Compare
|
@lvillani Thanks for contributing to the project 🙇 I've been reflecting on the growing number of arguments in quickemu. I'm planning to remove most of the advanced configuration from arguments and expose it exclusively through options in the VM configuration. This is to simplify the code a little and also focus the command execution on common use cases. So, I won't be merging this pull request as it stands, but if you want to refactor it to be automatically enabled when appropriate or exposed only via a configuration option, I'd welcome that 👍 |
|
Thanks for the feedback! I completely agree with the goal of simplifying the arguments and expose this setting only in the VM configuration file instead. I'll update the PR to implement it that way in a few days. |
5663769 to
554c0aa
Compare
|
I've updated the implementation so that viewer_extra_args is now exclusively read from the configuration file. I also realized I missed documenting this option in I'm converting this PR to a draft while I perform more thorough testing. I'll mark it as ready for review once that's complete. |
554c0aa to
5f6aff1
Compare
|
I have rebased my branch against |
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Small documentation updates (clarifying SPICE viewer) and minor feature addition (viewer_extra_args config option). 28 lines across 3 files. No destructive operations, credentials, or critical path a
5f6aff1 to
f5f9a04
Compare
f5f9a04 to
31a197c
Compare
|
Rebased against the |
|
@flexiondotorg Just bumping this since it's now ready for review! The changes reflect your feedback to move this directly to the VM configuration. Let me know what you think. |
Description
This PR adds support for passing additional arguments to the SPICE viewer. The feature was added mainly to support passing
--spice-usbredir-redirect-on-connectin order to support automatic USB device redirection. For example (in a configuration file):viewer_extra_args="--spice-usbredir-redirect-on-connect=-1,0x1234,0x5678,-1,1"Where
0x1234and0x5678are example USB VID and PID, respectively.The string format is described at https://www.spice-space.org/usbredir.html#filter-string-format.
This PR also contains a minor style commit (removing trailing whitespace) and another commit clarifying that "viewer" actually refers to the SPICE viewer.
Type of change
Checklist: