Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion Sources/tart/Commands/Set.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ struct Set: AsyncParsableCommand {
@Option(help: "VM memory size in megabytes")
var memory: UInt64?

@Option(help: "VM display resolution in a format of WIDTHxHEIGHT[pt|px]. For example, 1200x800, 1200x800pt or 1920x1080px. Units are treated as hints and default to \"pt\" (points) for macOS VMs and \"px\" (pixels) for Linux VMs when not specified.")
@Option(help: "VM display resolution in a format of WIDTHxHEIGHT[pt|px][@PPI]. For example, 1200x800, 1200x800pt, 1920x1080px or 3200x1800px@220. Units are treated as hints and default to \"pt\" (points) for macOS VMs and \"px\" (pixels) for Linux VMs when not specified. The optional @PPI (pixels-per-inch) hint applies to pixel-unit displays and, at a Retina-class value such as 220, makes a macOS guest expose a HiDPI (2x) mode regardless of the host display; it defaults to 72 (non-Retina) when omitted.")
var display: VMDisplayConfig?

@Flag(inversion: .prefixedNo, help: ArgumentHelp("Whether to automatically reconfigure the VM's display to fit the window"))
Expand Down Expand Up @@ -57,6 +57,7 @@ struct Set: AsyncParsableCommand {
vmConfig.display.height = display.height
}
vmConfig.display.unit = display.unit
vmConfig.display.ppi = display.ppi

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject nonpositive PPI hints

When a user mistypes the new suffix as @0 or @-1, the parser produces a non-nil ppi and this assignment persists it even though width and height have positive guards. The macOS pixel path later feeds effectivePixelsPerInch straight into VZMacGraphicsDisplayConfiguration, so the saved config no longer falls back to 72 and can fail to construct or validate the display until the user manually resets it; reject or ignore ppi <= 0 before saving.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch — fixed in 60bf256. The --display parser now keeps only a positive PPI (@0/@-1 are ignored like non-numeric input), and effectivePixelsPerInch also treats a non-positive stored value (e.g. a hand-edited config) as unset, so a non-positive value can never reach VZMacGraphicsDisplayConfiguration — it falls back to 72. Covered by two new tests in VMConfigTests.

}

vmConfig.displayRefit = displayRefit
Expand Down Expand Up @@ -91,6 +92,16 @@ extension VMDisplayConfig: ExpressibleByArgument {
public init(argument: String) {
var argument = argument
var unit: Unit? = nil
var ppi: Int? = nil

// Optional "@PPI" pixels-per-inch hint, e.g. "3200x1800px@220". Parsed
// before the unit suffix since it always trails the whole spec. Only a
// positive value is kept; zero/negative (or non-numeric) is ignored so the
// display falls back to the 72 default rather than a broken configuration.
if let atIndex = argument.lastIndex(of: "@") {
ppi = Int(argument[argument.index(after: atIndex)...]).flatMap { $0 > 0 ? $0 : nil }
argument = String(argument[..<atIndex])
}

if argument.hasSuffix(Unit.pixel.rawValue) {
argument = String(argument.dropLast(Unit.pixel.rawValue.count))
Expand All @@ -107,6 +118,7 @@ extension VMDisplayConfig: ExpressibleByArgument {
width: parts[safe: 0] ?? 0,
height: parts[safe: 1] ?? 0,
unit: unit,
ppi: ppi,
)
}
}
6 changes: 4 additions & 2 deletions Sources/tart/Platform/Darwin.swift
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,11 @@ struct UnsupportedHostOSError: Error, CustomStringConvertible {
VZMacGraphicsDisplayConfiguration(
widthInPixels: vmConfig.display.width,
heightInPixels: vmConfig.display.height,
// A reasonable guess according to Apple's documentation[1]
// Defaults to 72 — a reasonable guess according to Apple's
// documentation[1] — unless a --display "@PPI" hint asks for a
// Retina-class density (which yields a HiDPI guest mode).
// [1]: https://developer.apple.com/documentation/coregraphics/1456599-cgdisplayscreensize
pixelsPerInch: 72
pixelsPerInch: vmConfig.display.effectivePixelsPerInch
)
]

Expand Down
23 changes: 20 additions & 3 deletions Sources/tart/VMConfig.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,32 @@ struct VMDisplayConfig: Codable, Equatable {
var width: Int = 1024
var height: Int = 768
var unit: Unit?
var ppi: Int?

// Pixels-per-inch handed to the guest's pixel-unit framebuffer. Defaults to
// 72 (non-Retina) when no positive hint is given, matching
// Virtualization.framework's historical assumption; a Retina-class value
// (e.g. 220) makes the guest expose a HiDPI (scaling:on) mode regardless of
// the host display. A non-positive stored value (e.g. from a hand-edited
// config) is treated as unset so it never reaches the display configuration.
var effectivePixelsPerInch: Int {
if let ppi, ppi > 0 {
return ppi
}
return 72
}
}

extension VMDisplayConfig: CustomStringConvertible {
var description: String {
var result = "\(width)x\(height)"
if let unit {
"\(width)x\(height)\(unit.rawValue)"
} else {
"\(width)x\(height)"
result += unit.rawValue
}
if let ppi {
result += "@\(ppi)"
}
return result
}
}

Expand Down
57 changes: 57 additions & 0 deletions Tests/TartTests/VMConfigTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,61 @@ final class VMConfigTests: XCTestCase {
vmDisplayConfig = VMDisplayConfig.init(argument: "1234x5678px")
XCTAssertEqual(VMDisplayConfig(width: 1234, height: 5678, unit: .pixel), vmDisplayConfig)
}

func testEffectivePixelsPerInchUsesConfiguredValue() throws {
let config = VMDisplayConfig(width: 3200, height: 1800, unit: .pixel, ppi: 220)
XCTAssertEqual(220, config.effectivePixelsPerInch)
}

func testEffectivePixelsPerInchDefaultsTo72WhenUnset() throws {
// Upstream behavior: a pixel display with no PPI hint stays non-Retina (72 PPI).
let config = VMDisplayConfig(width: 1234, height: 5678, unit: .pixel, ppi: nil)
XCTAssertEqual(72, config.effectivePixelsPerInch)
}

func testParsesPixelsPerInchSuffix() throws {
let config = VMDisplayConfig(argument: "3200x1800px@220")
XCTAssertEqual(VMDisplayConfig(width: 3200, height: 1800, unit: .pixel, ppi: 220), config)
}

func testWithoutSuffixHasNilPixelsPerInch() throws {
// Backward compatibility: existing "WIDTHxHEIGHT[pt|px]" strings carry no PPI.
XCTAssertEqual(
VMDisplayConfig(width: 1234, height: 5678, unit: .pixel, ppi: nil),
VMDisplayConfig(argument: "1234x5678px"))
}

func testDescriptionRoundTripsPixelsPerInch() throws {
let config = VMDisplayConfig(width: 3200, height: 1800, unit: .pixel, ppi: 220)
XCTAssertEqual("3200x1800px@220", config.description)
XCTAssertEqual(config, VMDisplayConfig(argument: config.description))
}

func testParsesPixelsPerInchWithoutExplicitUnit() throws {
let config = VMDisplayConfig(argument: "3200x1800@220")
XCTAssertEqual(VMDisplayConfig(width: 3200, height: 1800, unit: nil, ppi: 220), config)
}

func testMalformedPixelsPerInchDegradesToNil() throws {
// A non-numeric PPI is ignored (falls back to the 72 default) rather than
// failing the parse — consistent with the parser's lenient dimensions.
let config = VMDisplayConfig(argument: "3200x1800px@notanumber")
XCTAssertEqual(VMDisplayConfig(width: 3200, height: 1800, unit: .pixel, ppi: nil), config)
XCTAssertEqual(72, config.effectivePixelsPerInch)
}

func testRejectsNonPositivePixelsPerInchWhenParsing() throws {
// A zero or negative PPI is meaningless and must not persist — it would
// otherwise reach VZMacGraphicsDisplayConfiguration and fail to build the
// display instead of falling back to the 72 default.
XCTAssertNil(VMDisplayConfig(argument: "3200x1800px@0").ppi)
XCTAssertNil(VMDisplayConfig(argument: "3200x1800px@-5").ppi)
}

func testEffectivePixelsPerInchIgnoresNonPositiveStoredValue() throws {
// Defense for a hand-edited config.json: a stored value <= 0 falls back to
// 72 rather than being handed to VZMacGraphicsDisplayConfiguration.
XCTAssertEqual(72, VMDisplayConfig(width: 100, height: 100, unit: .pixel, ppi: 0).effectivePixelsPerInch)
XCTAssertEqual(72, VMDisplayConfig(width: 100, height: 100, unit: .pixel, ppi: -5).effectivePixelsPerInch)
}
}