feat(displaycontrol): let a server handler override its capabilities

[Greg Lamberson (glamberson)]

Summary

• DisplayControlServer::start() sends DisplayControlCapabilities::new(1, 3840, 2400) unconditionally, so every server using this crate is capped at one monitor with no way to advertise more.
• Added a default trait method on DisplayControlHandler, capabilities(), that start() now calls instead of the hardcoded literal.
• The default returns the same (1, 3840, 2400) values, so any existing implementation that does not override it keeps its current behavior unchanged.
• A handler serving more than one monitor can override capabilities() to report the real count.

Validation

cargo xtask check fmt/lints/tests/typos/locks all pass.

Notes

• Additive only: one new trait method with a default body, one call-site change in start(). No existing implementor is affected unless it opts in.

[Copilot]

🟢 Approval recommended

The implementation is sound; the only unresolved comment is a non-blocking documentation nit.

Pull request overview

Enables display-control handlers to customize advertised monitor capabilities while preserving existing defaults.

Changes:

• Adds an overridable capabilities() method.
• Uses handler-provided capabilities during startup.

File summaries

File	Review
crates/ironrdp-displaycontrol/src/server.rs	Capability customization is additive. Minor documentation issue: the example must handle the DecodeResult returned by DisplayControlCapabilities::new(...).

Review details

Suppressed comments (1)

crates/ironrdp-displaycontrol/src/server.rs:20

• DisplayControlCapabilities::new(...) returns a DecodeResult, so the documented expression cannot be returned directly from this method. Avoid presenting it as a valid return expression unless the validation result is handled explicitly.

    /// A handler serving more than one monitor should override this to
    /// return `DisplayControlCapabilities::new(monitor_count, 3840, 2400)`.

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Balanced

[github-actions]

Additive, well-scoped change: DisplayControlServer::start() now takes the DISPLAYCONTROL_CAPS_PDU contents from a new defaulted DisplayControlHandler::capabilities() instead of a hardcoded (1, 3840, 2400) literal. I verified the default stays within the crate's internal limits (MAX_SUPPORTED_MONITORS 1024, MAX_MONITOR_AREA_FACTOR 16384 in pdu/mod.rs), so its expect never fires and the encoded PDU is byte-identical to before; non-overriding implementors are unaffected, and the hook mirrors the existing monitor_layout() and egfx preferred_capabilities() patterns. One merged issue (three duplicate specialist findings): the infallible signature deletes start()'s decode_err! error path and the doc example encourages .expect, so an override built from runtime state that exceeds the crate limits panics during channel start; a Result-returning hook would preserve error propagation. No other correctness, protocol, or maintainability concerns found.

[github-actions]

Additive and correct. DisplayControlHandler::capabilities() defaults to DisplayControlCapabilities::new(1, 3840, 2400), reproducing the previous hardcoded start() output exactly, so the sole in-repo implementor (ironrdp-server's DisplayControlBackend, which does not override) is unaffected. start() propagates handler errors via the same decode_err! bridge process() uses; validation bounds in calculate_monitor_area are unchanged and one spec-conformant CAPS PDU is still sent at channel start. The doc comment accurately explains the fallibility and panic-abort risk. The added error-path test compiles against existing test dependencies and asserts real behavior. Changelog updates are automated via release-plz/git-cliff, so no manual CHANGELOG edit is expected. One low-severity gap remains: no test verifies a valid override's values reach the encoded start message, the error assertion does not check the error kind, and the default output stays untested.

[Benoît Cortier (CBenoit)]

Hi Greg Lamberson (@glamberson)
This is looking good to me, but may I confirm with you if you pushed everything on your side? I see you answered a comment above as addressed with more tests, but I don’t see them.
Thanks!

[Greg Lamberson (glamberson)]

Oops! I hadn't pushed that commit yet. Just pushed 3bb21ee, which adds the two tests: One confirming a multi-monitor override actually encodes into the DISPLAYCONTROL_CAPS_PDU, the other pinning the default (1, 3840, 2400) capabilities that predate this PR. All xtask gates pass.

Thanks for catching it.

[github-actions]

This pull request may overlap with #1918.

Both PRs center on the same DISPLAYCONTROL capabilities extension point: this PR adds the fallible, overridable DisplayControlHandler::capabilities() in ironrdp-displaycontrol, while #1918 implements that override in ironrdp-server's DisplayControlBackend backed by a new RdpServerDisplay::monitor_count() to advertise real monitor counts. Shared scope around what DISPLAYCONTROL_CAPS_PDU values are sent at channel start; flagged for human assessment, not a redundancy verdict.

This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide.

Note

LLM-assisted content (no human feedback).