From 0348d2990fb42c7b50b29abe97bffc13a493a1ac Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 01:35:49 +0000 Subject: [PATCH 01/13] print: give the job ticket the driver's own prefix for its vendor namespace Windows' PTMergeAndValidatePrintTicket names a vendor namespace with a generated prefix (ns0000) as soon as the merged ticket uses a name from it. driver_ticket merges the driver's own paper option from its capabilities, which carries vendor properties, so a Brother HL-L2390DW's ticket came back with ns0000 where the driver's own default ticket says brpsk. XML-wise the two are the same; the driver's print pipeline reads the prefix as text. The job was accepted and then ID2D1PrintControl::AddPage (or Close, in older builds) failed with 0x80040003, reported by winprint as "Render error" / "Failed to close print control: No such interface supported". The same ticket sent raw with brpsk prints; with only the prefix changed to ns0000 it fails the same way, so the prefix is the whole difference. driver_ticket now re-spells the validated ticket with the prefixes of the driver's own default ticket (an empty ticket, validated), through ticket::respell: pure XML, tested on every platform, and a no-op unless a vendor namespace is spelled differently. It rewrites only the root declarations and the vendor names in name / constrained attributes and QName values, handles two namespaces trading prefixes, and leaves the ticket as it is when a rename could be ambiguous or leave a prefix undeclared. Verified on a Brother HL-L2390DW: the job spools, and pdfcraft head's doc_print that failed at AddPage prints. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01FQ5PzHVUcntEHU97znim5u --- crates/print/README.md | 7 +- crates/print/src/ticket.rs | 197 ++++++++++++++++++++++++++++++++++++ crates/print/src/windows.rs | 22 +++- 3 files changed, 223 insertions(+), 3 deletions(-) diff --git a/crates/print/README.md b/crates/print/README.md index 00b126893..5ccfce058 100644 --- a/crates/print/README.md +++ b/crates/print/README.md @@ -23,8 +23,11 @@ opens Windows printer preferences and submits jobs with copies, collation, duple orientation and "no scaling" travel in a PrintTicket that the driver validates first; a driver that would change the paper, orientation or scaling is refused before anything is sent. Paper is the driver's own option for the sheet's size (to 0.25 mm), preferring the one - named like the standard size ("ARCH D" over a custom 24 × 36 in entry). This backend comes - from storytold/pdfcraft#599 and #1132. + named like the standard size ("ARCH D" over a custom 24 × 36 in entry). Windows' ticket merge + renames a driver's vendor namespace prefix (`brpsk` to `ns0000`) once the ticket uses one of + its names, and a Brother driver then fails every page with `0x80040003`; the validated ticket + is given the prefixes of the driver's own default ticket back (`ticket::respell`). This backend + comes from storytold/pdfcraft#599 and #1132. A job has one ticket, so one paper and orientation. Sheets of the other orientation are turned back with `/Rotate`, so portrait and landscape sheets still print as one job. Exact size refuses sheets of different sizes and paper the driver lacks. Shrink-to-fit uses the diff --git a/crates/print/src/ticket.rs b/crates/print/src/ticket.rs index 5f8c914bd..955ed3430 100644 --- a/crates/print/src/ticket.rs +++ b/crates/print/src/ticket.rs @@ -206,6 +206,152 @@ pub(crate) fn verify_job_options(xml: &str, job: &Job, pages: usize) -> Result<( Ok(()) } +const NS_XSI: &str = "http://www.w3.org/2001/XMLSchema-instance"; +const NS_XSD: &str = "http://www.w3.org/2001/XMLSchema"; +const NS_XML: &str = "http://www.w3.org/XML/1998/namespace"; + +/// A PrintTicket that sets nothing. Merged and validated by a driver it comes back as the +/// driver's own default ticket, which spells the driver's vendor namespaces the way the driver does. +pub(crate) fn empty_ticket() -> String { + format!(r#""#) +} + +/// The (prefix, namespace) pairs the root element declares for namespaces other than the Print +/// Schema's: the driver's vendor namespaces. +fn vendor_prefixes(document: &roxmltree::Document<'_>) -> Vec<(String, String)> { + document + .root_element() + .namespaces() + .filter_map(|n| n.name().map(|prefix| (prefix.to_string(), n.uri().to_string()))) + .filter(|(_, uri)| ![NS_PSF, NS_PSK, NS_XSI, NS_XSD, NS_XML].contains(&uri.as_str())) + .collect() +} + +/// Whether every prefix in a `name` or `constrained` attribute or a QName value of the ticket is +/// declared: the XML parser does not check a prefix inside an attribute value. +fn prefixes_resolve(document: &roxmltree::Document<'_>) -> bool { + let unresolved = |element: roxmltree::Node<'_, '_>, value: &str| { + value.split_once(':').is_some_and(|(prefix, _)| element.lookup_namespace_uri(Some(prefix)).is_none()) + }; + !document.descendants().any(|node| { + if node.is_element() { + node.attributes().any(|a| a.namespace().is_none() && matches!(a.name(), "name" | "constrained") && unresolved(node, a.value())) + } else { + node.is_text() && node.parent().is_some_and(|p| is_qname_value(p) && node.text().is_some_and(|t| unresolved(p, t.trim()))) + } + }) +} + +/// A `Value` whose text is a qualified name (`xsi:type="xsd:QName"`): its prefix is a namespace's. +fn is_qname_value(node: roxmltree::Node<'_, '_>) -> bool { + node.attribute((NS_XSI, "type")).is_some_and(|t| t.rsplit(':').next() == Some("QName")) +} + +/// The start of every `xmlns:prefix` declaration's prefix in the start tag that begins at +/// `start`, with the prefix. +fn declared_prefixes(xml: &str, start: usize) -> Vec<(usize, String)> { + let mut found = Vec::new(); + let mut quote = None; + let mut at = start; + let mut previous = None; + for c in xml.get(start..).unwrap_or_default().chars() { + match quote { + Some(q) if c == q => quote = None, + Some(_) => {} + None if c == '"' || c == '\'' => quote = Some(c), + None if c == '>' => break, + None => {} + } + if quote.is_none() + && c == 'x' + && previous.is_some_and(char::is_whitespace) + && let Some(name) = xml.get(at..).and_then(|rest| rest.strip_prefix("xmlns:")) + { + let prefix: String = name.chars().take_while(|c| !c.is_whitespace() && *c != '=').collect(); + found.push((at + "xmlns:".len(), prefix)); + } + previous = Some(c); + at += c.len_utf8(); + } + found +} + +/// `ticket` with the driver's own prefix for each of the driver's vendor namespaces, where the +/// ticket spells one differently; `None` when nothing needs changing, or when it can't be done +/// safely (then the ticket stays as it is). `driver` is the driver's own ticket +/// ([`empty_ticket`] validated). +/// +/// Windows' ticket merge names a vendor namespace with a prefix of its own (`ns0000`) as soon as the +/// merged ticket uses a name from it, such as the driver's own paper option from its capabilities. +/// The prefix means nothing in XML, but some drivers' print pipelines read it as text: the job is +/// accepted, then every page fails in the XPS print API (`0x80040003`, "No such interface +/// supported"). A Brother driver names its namespace `brpsk` and fails with `ns0000`. Only the +/// declarations and the vendor names in `name`/`constrained` attributes and QName values change. +pub(crate) fn respell(ticket: &str, driver: &str) -> Option { + let document = roxmltree::Document::parse(ticket).ok()?; + let own = vendor_prefixes(&roxmltree::Document::parse(driver).ok()?); + let declared = vendor_prefixes(&document); + let renames: Vec<(&str, &str)> = declared + .iter() + .filter_map(|(old, uri)| own.iter().find(|(_, u)| u == uri).map(|(new, _)| (old.as_str(), new.as_str()))) + .filter(|(old, new)| old != new) + .collect(); + if renames.is_empty() { + return None; + } + let renamed = |old: &str| renames.iter().find(|(o, _)| *o == old).map(|(_, new)| *new); + // Each prefix must still name one namespace; a swap of two (ns0000 and ns0001) is fine. + let mut after: Vec<&str> = document.root_element().namespaces().filter_map(|n| n.name()).map(|p| renamed(p).unwrap_or(p)).collect(); + let declared_count = after.len(); + after.sort_unstable(); + after.dedup(); + if after.len() != declared_count { + return None; + } + let respelled = |at: usize| { + let rest = ticket.get(at..)?; + renames.iter().find(|(old, _)| rest.strip_prefix(old).is_some_and(|r| r.starts_with(':'))).map(|(old, new)| (at, at + old.len(), *new)) + }; + let mut edits: Vec<(usize, usize, &str)> = Vec::new(); + for (at, prefix) in declared_prefixes(ticket, document.root_element().range().start) { + if let Some(new) = renamed(&prefix) { + edits.push((at, at + prefix.len(), new)); + } + } + let uses_renamed = |ns: Option<&str>| ns.is_some_and(|ns| declared.iter().any(|(old, uri)| uri == ns && renamed(old).is_some())); + for node in document.descendants() { + if node.is_element() { + // A vendor namespace used for an element or attribute is not re-spelled here. + if uses_renamed(node.tag_name().namespace()) || node.attributes().any(|a| uses_renamed(a.namespace())) { + return None; + } + for a in + node.attributes().filter(|a| (a.namespace().is_none() && matches!(a.name(), "name" | "constrained")) || a.namespace() == Some(NS_XSI)) + { + edits.extend(respelled(a.range_value().start)); + } + } else if node.is_text() && node.parent().is_some_and(is_qname_value) { + edits.extend(respelled(node.range().start)); + } + } + edits.sort_unstable_by_key(|(start, ..)| *start); + let mut out = String::with_capacity(ticket.len()); + let mut copied = 0; + for (start, end, new) in edits { + out.push_str(ticket.get(copied..start)?); + out.push_str(new); + copied = end; + } + out.push_str(ticket.get(copied..)?); + // The result must declare each namespace under the prefix wanted and resolve every prefix. + let check = roxmltree::Document::parse(&out).ok()?; + let mut now = vendor_prefixes(&check); + let mut wanted: Vec<(String, String)> = declared.iter().map(|(old, uri)| (renamed(old).unwrap_or(old).to_string(), uri.clone())).collect(); + now.sort_unstable(); + wanted.sort_unstable(); + (now == wanted && prefixes_resolve(&check)).then_some(out) +} + /// The widest unprintable margin of the paper, in micrometres, from PrintCapabilities computed /// for the job's ticket (`PageImageableSize`). The largest of the four, so content centred inside /// it is clear of every edge whichever way the driver measures. `None` when the driver doesn't say. @@ -297,6 +443,57 @@ mod tests { assert!(verify_ticket(" String { + format!( + r#"215900279400mm{prefix}:Fastns0000:text, not a name"# + ) + } + + #[test] + fn a_vendor_namespace_gets_the_prefix_the_driver_declares_it_with() { + let merged = brother("ns0000"); + assert_eq!(respell(&merged, &brother("brpsk")), Some(brother("brpsk")), "declaration, names and QName values; not the text"); + assert_eq!(respell(&brother("brpsk"), &brother("brpsk")), None, "already the driver's"); + assert_eq!(respell(&brother("brpsk"), &merged), Some(merged.clone()), "whatever the driver calls it"); + // What the job's checks read is the same either way. + let letter = (215_900, 279_400); + assert_eq!(verify_ticket(&merged, Some(letter), false), Ok(letter)); + assert_eq!(verify_ticket(&respell(&merged, &brother("brpsk")).unwrap(), Some(letter), false), Ok(letter)); + } + + #[test] + fn two_vendor_namespaces_can_trade_prefixes() { + let other = "http://example.com/other"; + let ticket = |a: &str, b: &str| { + format!( + r#""# + ) + }; + assert_eq!(respell(&ticket("ns0000", "ns0001"), &ticket("ns0001", "ns0000")), Some(ticket("ns0001", "ns0000"))); + } + + #[test] + fn a_ticket_that_cannot_be_respelled_safely_is_left_alone() { + let merged = brother("ns0000"); + assert_eq!(respell(&merged, &empty_ticket()), None, "the driver has no such namespace"); + assert_eq!(respell("", ""); + assert_eq!(respell(&element, &brother("brpsk")), None); + // A name whose prefix is not declared stays refused after the change. + let dangling = merged.replace("name=\"ns0000:Mode\"", "name=\"ns0001:Mode\""); + assert_eq!(respell(&dangling, &brother("brpsk")), None); + } + #[test] fn a_driver_that_drops_explicit_job_options_is_refused() { let job = Job { copies: 2, duplex: Some(Duplex::LongEdge), grayscale: true, ..Job::default() }; diff --git a/crates/print/src/windows.rs b/crates/print/src/windows.rs index 45a23aa7b..a4fad8937 100644 --- a/crates/print/src/windows.rs +++ b/crates/print/src/windows.rs @@ -103,12 +103,27 @@ fn driver_ticket(device: &PrinterDevice, job: &Job, sheet: (f64, f64), landscape } let mut builder = PrintTicketBuilder::new(device).map_err(spool_error)?; builder.merge(PrintTicket::from(document)).map_err(spool_error)?; - let validated = builder.build().map_err(spool_error)?; + let validated = in_drivers_spelling(device, builder.build().map_err(spool_error)?); let xml = std::str::from_utf8(validated.get_xml()).map_err(spool_error)?; let paper = ticket::verify_ticket(xml, wanted, landscape)?; Ok((validated, paper)) } +/// The ticket with the driver's vendor namespaces under the prefixes the driver declares them with +/// ([`ticket::respell`]). Windows' merge renames them (`brpsk` to `ns0000`) when the merged ticket +/// uses one of the driver's names, such as its own paper option, and a Brother driver then fails +/// every page of the job with 0x80040003. The driver's prefixes are those of its own default ticket +/// (an empty one, validated). The ticket as it is when they can't be read. +fn in_drivers_spelling(device: &PrinterDevice, ticket: PrintTicket) -> PrintTicket { + let spelled = (|| { + let mut builder = PrintTicketBuilder::new(device).ok()?; + builder.merge(PrintTicket::from_xml(ticket::empty_ticket())).ok()?; + let own = builder.build().ok()?; + ticket::respell(std::str::from_utf8(ticket.get_xml()).ok()?, std::str::from_utf8(own.get_xml()).ok()?) + })(); + spelled.map_or(ticket, PrintTicket::from_xml) +} + /// The widest unprintable margin of the ticket's paper, in points; 0 when the driver doesn't say. fn imageable_margin(device: &PrinterDevice, ticket: &PrintTicket) -> f64 { PrintCapabilities::fetch_xml_for_ticket(device, Some(ticket)) @@ -201,6 +216,11 @@ mod tests { let (ticket, paper) = driver_ticket(&device, &job, (612.0, 792.0), false).unwrap(); ticket::verify_job_options(std::str::from_utf8(ticket.get_xml()).unwrap(), &job, 1).unwrap(); assert_eq!(paper, (215_900, 279_400)); + // The ticket names the driver's vendor namespaces as the driver does (the merge renamed them). + let mut own = PrintTicketBuilder::new(&device).unwrap(); + own.merge(PrintTicket::from_xml(ticket::empty_ticket())).unwrap(); + let own = own.build().unwrap(); + assert_eq!(ticket::respell(std::str::from_utf8(ticket.get_xml()).unwrap(), std::str::from_utf8(own.get_xml()).unwrap()), None); eprintln!("unprintable margin: {} pt", imageable_margin(&device, &ticket)); if let Ok(path) = std::env::var("CRAFT_PRINT_TEST_TICKET_OUTPUT") { std::fs::write(path, ticket.get_xml()).unwrap(); From fb59c587dfb1e26f385ca385cd6acf5f8b8d572c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 02:06:11 +0000 Subject: [PATCH 02/13] print: respell refuses a nested redeclaration; test the vendor-element case for real Review of #307: - respell rewrites the root's namespace declarations, so an element below the root that declares either prefix of a rename again (ns0000 or brpsk) would make a name resolve to another namespace once the prefixes change. It now returns None for such a ticket, and a test covers both prefixes. - The vendor-element test replaced only an opening tag, left the closing tag behind and so never parsed: it passed because respell refused the broken XML, not because of what it was meant to check. Every refusal fixture is now asserted well-formed, and the element is replaced whole. Doing so showed the element guard was redundant: an element or attribute name in a renamed namespace leaves an undeclared prefix, which the final parse of the result already refuses. The guard is removed and the test now exercises that check. - Doc comments on the new constants and tests. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01FQ5PzHVUcntEHU97znim5u --- crates/print/src/ticket.rs | 58 +++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/crates/print/src/ticket.rs b/crates/print/src/ticket.rs index 955ed3430..4f385a209 100644 --- a/crates/print/src/ticket.rs +++ b/crates/print/src/ticket.rs @@ -206,8 +206,11 @@ pub(crate) fn verify_job_options(xml: &str, job: &Job, pages: usize) -> Result<( Ok(()) } +/// XML Schema instance namespace (`xsi:type`). const NS_XSI: &str = "http://www.w3.org/2001/XMLSchema-instance"; +/// XML Schema datatypes namespace (`xsd:integer`, `xsd:QName`). const NS_XSD: &str = "http://www.w3.org/2001/XMLSchema"; +/// The namespace XML itself binds to the `xml` prefix. const NS_XML: &str = "http://www.w3.org/XML/1998/namespace"; /// A PrintTicket that sets nothing. Merged and validated by a driver it comes back as the @@ -286,7 +289,8 @@ fn declared_prefixes(xml: &str, start: usize) -> Vec<(usize, String)> { /// The prefix means nothing in XML, but some drivers' print pipelines read it as text: the job is /// accepted, then every page fails in the XPS print API (`0x80040003`, "No such interface /// supported"). A Brother driver names its namespace `brpsk` and fails with `ns0000`. Only the -/// declarations and the vendor names in `name`/`constrained` attributes and QName values change. +/// declarations and the vendor names in `name`/`constrained` attributes and QName values change; +/// a ticket that redeclares one of the prefixes below its root is left as it is. pub(crate) fn respell(ticket: &str, driver: &str) -> Option { let document = roxmltree::Document::parse(ticket).ok()?; let own = vendor_prefixes(&roxmltree::Document::parse(driver).ok()?); @@ -308,6 +312,16 @@ pub(crate) fn respell(ticket: &str, driver: &str) -> Option { if after.len() != declared_count { return None; } + // Only the root's declarations are rewritten: an element below it that declares either prefix + // again would make a name mean something else once the prefixes change. + let touched = |prefix: &str| renames.iter().any(|(old, new)| prefix == *old || prefix == *new); + let redeclared = document + .descendants() + .filter(|n| n.is_element() && *n != document.root_element()) + .any(|n| declared_prefixes(ticket, n.range().start).iter().any(|(_, prefix)| touched(prefix))); + if redeclared { + return None; + } let respelled = |at: usize| { let rest = ticket.get(at..)?; renames.iter().find(|(old, _)| rest.strip_prefix(old).is_some_and(|r| r.starts_with(':'))).map(|(old, new)| (at, at + old.len(), *new)) @@ -318,13 +332,8 @@ pub(crate) fn respell(ticket: &str, driver: &str) -> Option { edits.push((at, at + prefix.len(), new)); } } - let uses_renamed = |ns: Option<&str>| ns.is_some_and(|ns| declared.iter().any(|(old, uri)| uri == ns && renamed(old).is_some())); for node in document.descendants() { if node.is_element() { - // A vendor namespace used for an element or attribute is not re-spelled here. - if uses_renamed(node.tag_name().namespace()) || node.attributes().any(|a| uses_renamed(a.namespace())) { - return None; - } for a in node.attributes().filter(|a| (a.namespace().is_none() && matches!(a.name(), "name" | "constrained")) || a.namespace() == Some(NS_XSI)) { @@ -343,7 +352,8 @@ pub(crate) fn respell(ticket: &str, driver: &str) -> Option { copied = end; } out.push_str(ticket.get(copied..)?); - // The result must declare each namespace under the prefix wanted and resolve every prefix. + // The result must parse (a vendor element or attribute name would be left with an undeclared + // prefix), declare each namespace under the prefix wanted and resolve every prefix. let check = roxmltree::Document::parse(&out).ok()?; let mut now = vendor_prefixes(&check); let mut wanted: Vec<(String, String)> = declared.iter().map(|(old, uri)| (renamed(old).unwrap_or(old).to_string(), uri.clone())).collect(); @@ -443,6 +453,7 @@ mod tests { assert!(verify_ticket("", ""); - assert_eq!(respell(&element, &brother("brpsk")), None); + let feature = ""; + assert!(merged.contains(feature)); + assert_eq!(refused(&merged.replace(feature, "")), None); // A name whose prefix is not declared stays refused after the change. - let dangling = merged.replace("name=\"ns0000:Mode\"", "name=\"ns0001:Mode\""); - assert_eq!(respell(&dangling, &brother("brpsk")), None); + assert_eq!(refused(&merged.replace("name=\"ns0000:Mode\"", "name=\"ns0001:Mode\"")), None); + // A nested element that declares either prefix again would change what a name resolves to. + for redeclared in ["ns0000", "brpsk"] { + let nested = merged.replace( + "", + &format!(""), + ); + assert_ne!(nested, merged); + assert_eq!(refused(&nested), None, "{redeclared} declared again below the root"); + } + // Control: the same ticket without the nested declaration is respelled. + assert!(refused(&merged).is_some()); } #[test] From 766f1c6f2d1b7e7b543442fe81d7a1e86f53884a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 02:58:18 +0000 Subject: [PATCH 03/13] print: respell leaves a ticket alone when an element or attribute name would move Review follow-up. When two vendor namespaces trade prefixes, both prefixes stay declared, so a name written with one of them as an element or attribute name (``, `ns0000:flag="1"`) still parsed and resolved after the root declarations changed, but to the other namespace. respell only rewrites `name`/`constrained` values and QName values, so such a name would not follow. respell now returns None when any element or attribute is named in a namespace whose prefix changes. Names in a namespace that keeps its prefix do not block the respelling, and the real Brother tickets (vendor names only in `name` attributes and QName values) are respelled as before. Tests: regression cases for element and attribute names in a swap and in a prefix chain, and a control for names in an untouched namespace. Each ticket is asserted well-formed with every prefix resolving, so the case is refused for the name, not for a parse failure. The refusal tests fail with the guard disabled. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01FQ5PzHVUcntEHU97zniM5U --- crates/print/src/ticket.rs | 90 ++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 8 deletions(-) diff --git a/crates/print/src/ticket.rs b/crates/print/src/ticket.rs index 4f385a209..269623dee 100644 --- a/crates/print/src/ticket.rs +++ b/crates/print/src/ticket.rs @@ -289,8 +289,9 @@ fn declared_prefixes(xml: &str, start: usize) -> Vec<(usize, String)> { /// The prefix means nothing in XML, but some drivers' print pipelines read it as text: the job is /// accepted, then every page fails in the XPS print API (`0x80040003`, "No such interface /// supported"). A Brother driver names its namespace `brpsk` and fails with `ns0000`. Only the -/// declarations and the vendor names in `name`/`constrained` attributes and QName values change; -/// a ticket that redeclares one of the prefixes below its root is left as it is. +/// declarations and the vendor names in `name`/`constrained` attributes and QName values change. +/// A ticket is left as it is when it redeclares one of the prefixes below its root, or has an +/// element or attribute named in a namespace whose prefix would change. pub(crate) fn respell(ticket: &str, driver: &str) -> Option { let document = roxmltree::Document::parse(ticket).ok()?; let own = vendor_prefixes(&roxmltree::Document::parse(driver).ok()?); @@ -322,6 +323,19 @@ pub(crate) fn respell(ticket: &str, driver: &str) -> Option { if redeclared { return None; } + // Changing the root's declarations moves every name written with one of these prefixes, and only + // the `name`/`constrained` values and QName values are rewritten to follow. An element or attribute + // name (``, `ns0000:flag="1"`) would not follow: it would be left with an undeclared + // prefix, or, when two namespaces trade prefixes, quietly resolve to the other namespace. Names + // in a namespace whose prefix stays as it is are not affected. + let moved: Vec<&str> = document.root_element().namespaces().filter(|n| n.name().is_some_and(touched)).map(|n| n.uri()).collect(); + let names_moved = document.descendants().filter(|n| n.is_element()).any(|n| { + n.tag_name().namespace().is_some_and(|uri| moved.contains(&uri)) + || n.attributes().any(|a| a.namespace().is_some_and(|uri| moved.contains(&uri))) + }); + if names_moved { + return None; + } let respelled = |at: usize| { let rest = ticket.get(at..)?; renames.iter().find(|(old, _)| rest.strip_prefix(old).is_some_and(|r| r.starts_with(':'))).map(|(old, new)| (at, at + old.len(), *new)) @@ -352,8 +366,8 @@ pub(crate) fn respell(ticket: &str, driver: &str) -> Option { copied = end; } out.push_str(ticket.get(copied..)?); - // The result must parse (a vendor element or attribute name would be left with an undeclared - // prefix), declare each namespace under the prefix wanted and resolve every prefix. + // A last check on the result: it must parse, declare each namespace under the prefix wanted and + // resolve every prefix. let check = roxmltree::Document::parse(&out).ok()?; let mut now = vendor_prefixes(&check); let mut wanted: Vec<(String, String)> = declared.iter().map(|(old, uri)| (renamed(old).unwrap_or(old).to_string(), uri.clone())).collect(); @@ -478,16 +492,76 @@ mod tests { assert_eq!(verify_ticket(&respell(&merged, &brother("brpsk")).unwrap(), Some(letter), false), Ok(letter)); } + /// A second vendor namespace, next to [`BROTHER`]. + const OTHER: &str = "http://example.com/other"; + + /// A ticket that names [`BROTHER`] with the prefix `a` and [`OTHER`] with `b` in its root, uses + /// each in a `name`, and ends with `extra`. + fn two_namespaces(a: &str, b: &str, extra: &str) -> String { + format!( + r#"{extra}"# + ) + } + /// Prefixes are renamed together, so two namespaces that swap prefixes still end up distinct. #[test] fn two_vendor_namespaces_can_trade_prefixes() { - let other = "http://example.com/other"; - let ticket = |a: &str, b: &str| { + let ticket = |a: &str, b: &str| two_namespaces(a, b, ""); + assert_eq!(respell(&ticket("ns0000", "ns0001"), &ticket("ns0001", "ns0000")), Some(ticket("ns0001", "ns0000"))); + } + + /// When two namespaces trade prefixes both prefixes stay declared, so a name that is only + /// written with a prefix (an element or an attribute, unlike a `name` value) would still parse and + /// resolve, but to the other namespace. Each ticket here is well-formed and has every prefix + /// declared, so `respell` leaves it alone for the name, not because the result didn't parse. + #[test] + fn a_swap_does_not_move_an_element_or_attribute_name_to_the_other_namespace() { + let driver = two_namespaces("ns0001", "ns0000", ""); + let refused = |extra: &str| { + let ticket = two_namespaces("ns0000", "ns0001", extra); + let document = roxmltree::Document::parse(&ticket).expect("the fixture must stay well-formed"); + assert!(prefixes_resolve(&document)); + respell(&ticket, &driver) + }; + assert!(refused("").is_some(), "control: the same ticket without such a name is respelled"); + + // An element: `ns0000:Feature` is BROTHER's until the declarations swap, then OTHER's. + assert_eq!(refused(""), None, "element in the first namespace"); + assert_eq!(refused(""), None, "element in the second namespace"); + assert_eq!(refused(""), None, "nested element"); + + // An attribute, whichever element carries it. + assert_eq!(refused(""), None, "attribute in the first namespace"); + assert_eq!(refused(""), None, "attribute in the second namespace"); + assert_eq!(refused(""), None, "both at once"); + } + + /// The same when the prefixes form a chain rather than a swap: `brpsk` is OTHER's, is about to + /// become BROTHER's, and OTHER moves on to `ns0005`. + #[test] + fn a_name_in_a_namespace_whose_prefix_is_taken_over_is_not_respelled() { + let driver = two_namespaces("brpsk", "ns0005", ""); + let refused = |extra: &str| { + let ticket = two_namespaces("ns0000", "brpsk", extra); + let document = roxmltree::Document::parse(&ticket).expect("the fixture must stay well-formed"); + assert!(prefixes_resolve(&document)); + respell(&ticket, &driver) + }; + assert!(refused("").is_some(), "control"); + assert_eq!(refused(""), None, "element: OTHER's now, BROTHER's after"); + assert_eq!(refused(""), None, "attribute"); + } + + /// Only names in a namespace whose prefix changes block the respelling. + #[test] + fn names_in_a_namespace_that_keeps_its_prefix_do_not_block_the_respelling() { + let keep = "http://example.com/keep"; + let ticket = |prefix: &str| { format!( - r#""# + r#""# ) }; - assert_eq!(respell(&ticket("ns0000", "ns0001"), &ticket("ns0001", "ns0000")), Some(ticket("ns0001", "ns0000"))); + assert_eq!(respell(&ticket("ns0000"), &ticket("brpsk")), Some(ticket("brpsk"))); } /// Each ticket here is well-formed XML, so `respell` leaves it alone for the reason named, not From 23bd96375a547f3205d9c5083cb2feacf20d3a92 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 06:34:58 +0000 Subject: [PATCH 04/13] print: the live driver test compares the vendor prefixes instead of re-running respell Review follow-up. The test asserted `respell(validated, driver_default) == None`. respell also returns None when it refuses a ticket, and in_drivers_spelling then keeps the ticket as it is, so a ticket still spelled `ns0000` passed. The test now compares the vendor namespace declarations of the ticket driver_ticket returns with those of the driver's own default ticket and fails when a namespace the driver declares is under another prefix. The comparison is `ticket::prefixes_unlike_the_drivers` (test-only), unit-tested here: it flags the ticket as the merge returns it, passes the respelled one and the driver's own, and flags a ticket respell refused, where `respell(..) == None` can't tell it from a correct one. Checked against a Brother HL-L2390DW: the test passes, and fails with `(brother namespace, "ns0000", "brpsk")` when in_drivers_spelling is made to return the merged ticket. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01FQ5PzHVUcntEHU97zniM5U --- crates/print/src/ticket.rs | 58 +++++++++++++++++++++++++++++++++++++ crates/print/src/windows.rs | 5 +++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/crates/print/src/ticket.rs b/crates/print/src/ticket.rs index 269623dee..9b76290e4 100644 --- a/crates/print/src/ticket.rs +++ b/crates/print/src/ticket.rs @@ -376,6 +376,29 @@ pub(crate) fn respell(ticket: &str, driver: &str) -> Option { (now == wanted && prefixes_resolve(&check)).then_some(out) } +/// Each vendor namespace that `ticket` declares under a prefix the driver does not declare it with, +/// as (namespace, prefix in the ticket, prefix of the driver); empty when the ticket is spelled the +/// way the driver spells it. `driver` is the driver's own ticket ([`empty_ticket`] validated). +/// `None` when either does not parse. +/// +/// For checking the ticket a job ends up with. [`respell`] returning `None` for it does not: that +/// is also what it returns when it refused to change a ticket, which then stays as it is. +#[cfg(test)] +pub(crate) fn prefixes_unlike_the_drivers(ticket: &str, driver: &str) -> Option> { + let own = vendor_prefixes(&roxmltree::Document::parse(driver).ok()?); + let declared = vendor_prefixes(&roxmltree::Document::parse(ticket).ok()?); + Some( + declared + .into_iter() + .filter(|pair| !own.contains(pair)) + .filter_map(|(prefix, uri)| { + let (theirs, _) = own.iter().find(|(_, u)| *u == uri)?; + Some((uri, prefix, theirs.clone())) + }) + .collect(), + ) +} + /// The widest unprintable margin of the paper, in micrometres, from PrintCapabilities computed /// for the job's ticket (`PageImageableSize`). The largest of the four, so content centred inside /// it is clear of every edge whichever way the driver measures. `None` when the driver doesn't say. @@ -552,6 +575,41 @@ mod tests { assert_eq!(refused(""), None, "attribute"); } + /// What the live driver test checks. A ticket `respell` refused keeps the prefix the merge gave + /// it, but `respell` says `None` for it, as it does for a ticket that is already right: only the + /// prefixes tell the two apart. + #[test] + fn the_prefixes_tell_a_refused_ticket_from_one_in_the_drivers_spelling() { + let driver = brother("brpsk"); + let unlike = |ticket: &str| prefixes_unlike_the_drivers(ticket, &driver).expect("both tickets parse"); + let merged = brother("ns0000"); + let mismatch = (BROTHER.to_string(), "ns0000".to_string(), "brpsk".to_string()); + + assert_eq!(unlike(&merged), std::slice::from_ref(&mismatch), "as the merge returns it"); + assert_eq!(unlike(&respell(&merged, &driver).unwrap()), [], "respelled"); + assert_eq!(unlike(&driver), [], "already the driver's"); + + // `respell` refuses this one (`ns0000` is declared again below the root), so it stays as the + // merge left it, and `respell` says `None` for it just as it does for the driver's own. + let refused = merged.replace( + "", + "", + ); + assert_ne!(refused, merged); + assert_eq!(respell(&refused, &driver), None); + assert_eq!(respell(&driver, &driver), None); + assert_eq!(unlike(&refused), [mismatch], "refused, and still wrong"); + + // Two namespaces that trade prefixes are both reported. + let traded = prefixes_unlike_the_drivers(&two_namespaces("ns0000", "ns0001", ""), &two_namespaces("ns0001", "ns0000", "")).unwrap(); + assert_eq!(traded.len(), 2, "{traded:?}"); + // A namespace the driver doesn't declare has no prefix of the driver's to differ from. + assert_eq!(unlike(&empty_ticket()), [], "no vendor namespace to compare"); + assert_eq!(unlike(&two_namespaces("ns0000", "ns0001", "")).len(), 1, "only BROTHER is the driver's"); + assert_eq!(prefixes_unlike_the_drivers(" Date: Tue, 27 Oct 2026 09:21:01 -0400 Subject: [PATCH 05/13] craft-ui: ComboBox, egui's combo box with a menu that grows back (#387) * craft-ui: combo boxes whose menu grows back with its contents egui's ComboBox lays its menu out in the height it had on the previous pass, and its scroll area takes no more than that. A filtered list that shrinks (a search with no matches) never grows back when the search is cleared (emilk/egui#5225, #5138). craft_ui::combo::menu offers its contents the full height on every pass; craft_ui::combo::combo draws the combo box button around it. The Font Manager's Language, Script and Sample menus use it. * craft-ui: combo and menu take the close behaviour A menu with a search field that closes itself on a pick (DesignCraft's font menu) closes on a click outside only. * craft-ui: ComboBox, a drop-in for egui's combo box, with header menus craft_ui::ComboBox has egui 0.37's ComboBox builder and button (same atoms, style classes and icon, pixel-identical in a test) with a menu that grows back. show_ui_with_header and show_rows_with_header keep a search field in place and scroll only the list; combo::Menu is the same menu under a field an app draws itself. clippy.toml disallows egui::ComboBox. The Font Manager's Language menu, the keymap dialog's combo boxes and the print dialog's printer and duplex choices use it. * craft-ui: combo menus keep to the room on their side, and footers A menu is never taller than the room on the side of its field it opens on, so an unbounded height fits on screen and scrolls. The side is settled once the menu is measured and kept while it is open, so a menu that shrinks or grows while typing doesn't jump to the other side of its field. Footer forms keep a row under the scrolling list. Menu::new takes the field's Ui for its default height and frame, and Menu::open shows the menu under a text field that keeps the focus while typing. * craft-ui: combo menus open by the app keep their side, and take picks Menu::open takes &mut bool: the menu clears it when it closes (close behavior, Escape, ui.close(), the field losing the focus to anything but the menu), so a click on a row under a focused text field is picked. The side settles only once the menu was shown and measured, also for menus the app opens (Popup::open_id). A footer that changes height remeasures the menu before it is shown. The popup frame's margins come from the popup style; only above/below alignments are tried. * craft-ui: a resting pointer over a combo menu doesn't keep it open The menu counts the pointer as having moved into it only on a press or click. A footer height change also requests a repaint, in case the discard is denied. * craft-ui: a footer menu opens without a second sizing pass A footer height change no longer discards the pass or forces another sizing pass: the menu measures again on the next pass and settles its side then. A forced sizing pass ran a search field's focus request in a pass where the field can't take the focus, so a footer menu opened with the search unfocused. * craft-ui: tests use Ui::scope_id * craft-ui: hide the measuring pass of a footer menu, cap remeasures The pass that measures a menu before its side is settled is laid out but drawn fully transparent, so no frame shows a side or height the menu then leaves. A footer that changes height on every pass remeasures twice per open, then settles. * craft-ui: tests satisfy clippy --- clippy.toml | 3 + crates/font-manager/src/panel/browse.rs | 26 +- crates/font-manager/src/panel/widgets.rs | 11 +- crates/keymap/src/ui/review.rs | 3 +- crates/keymap/src/ui/sets.rs | 3 +- crates/keymap/src/ui/view.rs | 5 +- crates/ui/src/combo.rs | 657 +++++++++++++++++++++++ crates/ui/src/combo/tests.rs | 526 ++++++++++++++++++ crates/ui/src/lib.rs | 3 + crates/ui/src/printing.rs | 4 +- 10 files changed, 1220 insertions(+), 21 deletions(-) create mode 100644 crates/ui/src/combo.rs create mode 100644 crates/ui/src/combo/tests.rs diff --git a/clippy.toml b/clippy.toml index 4b6c79f55..287e14edc 100644 --- a/clippy.toml +++ b/clippy.toml @@ -2,3 +2,6 @@ allow-unwrap-in-tests = true allow-expect-in-tests = true allow-panic-in-tests = true allow-indexing-slicing-in-tests = true +disallowed-types = [ + { path = "egui::ComboBox", reason = "use craft_ui::ComboBox: egui's combo box menu can't grow back while open (egui#5225)" }, +] diff --git a/crates/font-manager/src/panel/browse.rs b/crates/font-manager/src/panel/browse.rs index 9973ae3fb..e7112c1e4 100644 --- a/crates/font-manager/src/panel/browse.rs +++ b/crates/font-manager/src/panel/browse.rs @@ -119,25 +119,27 @@ impl FontsPanel { let search = &c.search; let language_text = &mut self.language_text; let query = &mut self.query; - widgets::select(ui, "craft-fonts.language", tl!("Language"), &selected, width, |ui| { + let row = ui.spacing().interact_size.y; + let header = |ui: &mut Ui| { let field = widgets::field(ui, s, language_text, tl!("Name in English or in the language"), tl!("Find a language"), ui.available_width(), true); if field.gained_focus() || ui.memory(|m| m.focused().is_none()) { field.request_focus(); } ui.add_space(4.0); - let matches = search.languages(language_text); - let row = ui.spacing().interact_size.y; - egui::ScrollArea::vertical().max_height(300.0).auto_shrink([false, true]).show_rows(ui, row, matches.len() + 1, |ui, range| { - for n in range { - if n == 0 { - ui.selectable_value(&mut query.language, None, tl!("Any language")); - } else if let Some(&i) = matches.get(n - 1) { - ui.selectable_value(&mut query.language, Some(i), language_label(i)); - } + search.languages(language_text) + }; + let rows = |ui: &mut Ui, range: std::ops::Range, matches: Vec| { + for n in range { + if n == 0 { + ui.selectable_value(&mut query.language, None, tl!("Any language")); + } else if let Some(&i) = matches.get(n - 1) { + ui.selectable_value(&mut query.language, Some(i), language_label(i)); } - }); - }); + } + }; + let inner = widgets::select_box("craft-fonts.language", &selected, width).show_rows_with_header(ui, header, row, |m| m.len() + 1, rows); + widgets::name(ui, &inner.response, tl!("Language")); if self.query.language != before { self.focus_row = None; } diff --git a/crates/font-manager/src/panel/widgets.rs b/crates/font-manager/src/panel/widgets.rs index bdd593584..960ed8b85 100644 --- a/crates/font-manager/src/panel/widgets.rs +++ b/crates/font-manager/src/panel/widgets.rs @@ -5,8 +5,8 @@ use std::sync::Arc; use egui::text::{LayoutJob, TextWrapping}; use egui::{ - Align2, Color32, ComboBox, CursorIcon, FontFamily, FontId, Galley, Painter, Pos2, Rect, Response, Role, Sense, Stroke, StrokeKind, TextEdit, - TextFormat, Ui, Vec2, WidgetInfo, pos2, vec2, + Align2, Color32, CursorIcon, FontFamily, FontId, Galley, Painter, Pos2, Rect, Response, Role, Sense, Stroke, StrokeKind, TextEdit, TextFormat, + Ui, Vec2, WidgetInfo, pos2, vec2, }; use crate::panel::look::{Look, Palette}; @@ -378,11 +378,16 @@ pub fn field(ui: &mut Ui, s: &Skin, text: &mut String, hint: &str, label: &str, /// A drop-down `width` wide showing `selected`, named `label`. pub fn select(ui: &mut Ui, id_salt: &str, label: &str, selected: &str, width: f32, add: impl FnOnce(&mut Ui) -> R) -> Option { - let inner = ComboBox::from_id_salt(id_salt).selected_text(selected).width(width).height(380.0).truncate().show_ui(ui, add); + let inner = select_box(id_salt, selected, width).show_ui(ui, add); name(ui, &inner.response, label); inner.inner } +/// The combo box of [`select`], for a menu with a header; name its response with [`name`]. +pub fn select_box<'a>(id_salt: &str, selected: &'a str, width: f32) -> craft_ui::ComboBox<'a> { + craft_ui::ComboBox::from_id_salt(id_salt).selected_text(selected).width(width).height(380.0).truncate() +} + /// An inline note in a sunken, hairline box: an info glyph, then `add`. pub fn note(ui: &mut Ui, s: &Skin, add: impl FnOnce(&mut Ui) -> R) -> R { egui::Frame::new() diff --git a/crates/keymap/src/ui/review.rs b/crates/keymap/src/ui/review.rs index 231bafeb6..2610d8a94 100644 --- a/crates/keymap/src/ui/review.rs +++ b/crates/keymap/src/ui/review.rs @@ -2,8 +2,9 @@ //! detail pane: what the set will change, what did not match and why, suggestions to pick from, //! and after the import the report of what was skipped, taken or failed. +use craft_ui::ComboBox; use egui::text::{LayoutJob, TextFormat}; -use egui::{ComboBox, Label, Rect, RichText, ScrollArea, TextEdit, TextStyle, Ui}; +use egui::{Label, Rect, RichText, ScrollArea, TextEdit, TextStyle, Ui}; use super::{Draw, FIELD_W, fill}; use crate::chord::foreign::{ForeignError, ReadNote}; diff --git a/crates/keymap/src/ui/sets.rs b/crates/keymap/src/ui/sets.rs index 7b4cd479b..f0b1502d0 100644 --- a/crates/keymap/src/ui/sets.rs +++ b/crates/keymap/src/ui/sets.rs @@ -2,8 +2,9 @@ //! Revert Set, Export, Import… and Summarize…; actions that do not fit go into a More menu. New //! Set… and Rename… open an inline form under the row. +use craft_ui::ComboBox; use craft_ui::layout::fit_toolbar; -use egui::{ComboBox, Key as EguiKey, RichText, TextEdit, Ui}; +use egui::{Key as EguiKey, RichText, TextEdit, Ui}; use super::{Draw, FIELD_W}; use crate::profiles::{Profiles, set_name}; diff --git a/crates/keymap/src/ui/view.rs b/crates/keymap/src/ui/view.rs index 56b65c6dc..7b46245a1 100644 --- a/crates/keymap/src/ui/view.rs +++ b/crates/keymap/src/ui/view.rs @@ -8,6 +8,7 @@ use std::ops::Range; use std::sync::Arc; +use craft_ui::ComboBox; use craft_ui::buttons::{IconButton, IconButtonStyle}; use craft_ui::layout::{ResizeHandle, SplitAxis, SplitSize, split_rect}; use craft_ui::tabs::{self, Tab}; @@ -15,8 +16,8 @@ use egui::Galley; use egui::accesskit::Live; use egui::text::{LayoutJob, TextFormat, TextWrapping}; use egui::{ - Align, Button, Color32, ComboBox, EventFilter, Id, Key as EguiKey, Label, Layout, Modifiers, Painter, Rect, Response, RichText, Role, ScrollArea, - Sense, Stroke, StrokeKind, TextEdit, TextStyle, Ui, UiBuilder, Vec2, WidgetInfo, WidgetText, pos2, vec2, + Align, Button, Color32, EventFilter, Id, Key as EguiKey, Label, Layout, Modifiers, Painter, Rect, Response, RichText, Role, ScrollArea, Sense, + Stroke, StrokeKind, TextEdit, TextStyle, Ui, UiBuilder, Vec2, WidgetInfo, WidgetText, pos2, vec2, }; use crate::chord::Trigger; diff --git a/crates/ui/src/combo.rs b/crates/ui/src/combo.rs new file mode 100644 index 000000000..d0138b7d8 --- /dev/null +++ b/crates/ui/src/combo.rs @@ -0,0 +1,657 @@ +//! Combo boxes and drop-down menus whose menu follows its contents' height while it is open. +//! +//! egui lays a popup out in the size it had on the previous pass, and [`egui::ComboBox`] puts its +//! menu in a [`ScrollArea`] that takes no more than that height. Contents that shrink (a search +//! with no matches) shrink the menu; contents that grow again (the search cleared) only scroll +//! inside it, and the menu stays small until it is closed (emilk/egui#5225). [`ComboBox`] is +//! egui's combo box with a menu that offers its contents the full height on every pass, so the +//! menu grows back. [`Menu`] is that menu under a field an app draws itself. +//! +//! A searchable menu keeps its search field in place and scrolls only its list: +//! [`ComboBox::show_ui_with_header`] and [`ComboBox::show_rows_with_header`], and the same on +//! [`Menu`]. + +use std::cell::RefCell; +use std::ops::Range; + +use egui::{ + Align, Align2, AsIdSalt, Atom, AtomExt as _, Atoms, Context, Direction, Frame, Id, IdSalt, InnerResponse, IntoAtoms, LayerId, NumExt as _, Order, + Painter, Popup, PopupCloseBehavior, Rect, RectAlign, Response, Role, ScrollArea, Sense, Shape, TextStyle, TextWrapMode, Ui, Vec2, WidgetAtom, + WidgetInfo, + class::{ClassName, Classes, HasClasses}, + style::{StyleModifier, WidgetVisuals}, + vec2, + widget_style::ButtonStyle, +}; + +pub use egui::IconPainter; + +/// egui's [`egui::ComboBox`] with a menu that grows when its contents do. +/// +/// The builder, the button and what the methods return are egui 0.37's. The one difference is +/// the menu: its scroll area is offered the full [`Self::height`] on every pass instead of the +/// menu's height on the previous pass, so a menu whose contents shrink and grow again while it +/// is open (a filtered list) grows back. It adds [`Self::show_ui_with_header`] and +/// [`Self::show_rows_with_header`] for menus with a search field. +/// +/// ``` +/// # egui::__run_test_ui(|ui| { +/// let mut selected = 0; +/// craft_ui::ComboBox::from_label("Size").show_index(ui, &mut selected, 3, |i| ["Small", "Medium", "Large"][i]); +/// # }); +/// ``` +#[must_use = "You should call .show*"] +pub struct ComboBox<'a> { + id_salt: IdSalt, + label: Option>, + selected_text: Atoms<'a>, + width: Option, + height: Option, + icon: Option, + wrap_mode: Option, + close_behavior: Option, + popup_style: StyleModifier, + classes: Classes, +} + +impl<'a> ComboBox<'a> { + /// Present on the button of every combo box, as on egui's. + #[expect(clippy::disallowed_types, reason = "egui's class names, so themes style both alike")] + pub const CLASS: ClassName = egui::ComboBox::CLASS; + + /// Present on the button of a combo box while its menu is open, as on egui's. + #[expect(clippy::disallowed_types, reason = "egui's class names, so themes style both alike")] + pub const CLASS_OPEN: ClassName = egui::ComboBox::CLASS_OPEN; + + fn with(id_salt: IdSalt, label: Option>) -> Self { + let mut combo = Self { + id_salt, + label, + selected_text: Atoms::default(), + width: None, + height: None, + icon: None, + wrap_mode: None, + close_behavior: None, + popup_style: StyleModifier::default(), + classes: Classes::default(), + }; + combo.add_class(Self::CLASS); + combo + } + + /// A combo box with `label` next to it, its id from `id_salt`. + pub fn new(id_salt: impl AsIdSalt, label: impl IntoAtoms<'a>) -> Self { + Self::with(IdSalt::new(id_salt), Some(label.into_atoms())) + } + + /// A combo box with `label` next to it, its id from the label's contents. + pub fn from_label(label: impl IntoAtoms<'a>) -> Self { + let label = label.into_atoms(); + Self::with(label.salt(), Some(label)) + } + + /// A combo box without a label. + pub fn from_id_salt(id_salt: impl AsIdSalt) -> Self { + Self::with(IdSalt::new(id_salt), None) + } + + /// The button's minimum width. Default: [`egui::Spacing::combo_width`]. + #[inline] + pub fn width(mut self, width: f32) -> Self { + self.width = Some(width); + self + } + + /// The maximum height of the menu's contents, inside its frame. Default: + /// [`egui::Spacing::combo_height`]. + #[inline] + pub fn height(mut self, height: f32) -> Self { + self.height = Some(height); + self + } + + /// What the button shows as the selected value. + #[inline] + pub fn selected_text(mut self, selected_text: impl IntoAtoms<'a>) -> Self { + self.selected_text = selected_text.into_atoms(); + self + } + + /// Paints the icon in place of the down-pointing triangle. + #[inline] + pub fn icon(mut self, icon_fn: impl FnOnce(&Ui, Rect, &WidgetVisuals, bool) + 'static) -> Self { + self.icon = Some(Box::new(icon_fn)); + self + } + + /// How the selected text wraps. Default: the [`Ui`]'s. + #[inline] + pub fn wrap_mode(mut self, wrap_mode: TextWrapMode) -> Self { + self.wrap_mode = Some(wrap_mode); + self + } + + /// Wraps the selected text: [`TextWrapMode::Wrap`]. + #[inline] + pub fn wrap(mut self) -> Self { + self.wrap_mode = Some(TextWrapMode::Wrap); + self + } + + /// Truncates the selected text: [`TextWrapMode::Truncate`]. + #[inline] + pub fn truncate(mut self) -> Self { + self.wrap_mode = Some(TextWrapMode::Truncate); + self + } + + /// When the menu closes. Default: [`PopupCloseBehavior::CloseOnClick`]. + #[inline] + pub fn close_behavior(mut self, close_behavior: PopupCloseBehavior) -> Self { + self.close_behavior = Some(close_behavior); + self + } + + /// The menu's style. Default: the [`Ui`]'s. + #[inline] + pub fn popup_style(mut self, popup_style: StyleModifier) -> Self { + self.popup_style = popup_style; + self + } + + /// Shows the button, and `menu_contents` in the menu while it is open. `inner` is what + /// `menu_contents` returned, or `None` while the menu is closed. + pub fn show_ui(self, ui: &mut Ui, menu_contents: impl FnOnce(&mut Ui) -> R) -> InnerResponse> { + self.show(ui, |menu| menu.show_ui(menu_contents)) + } + + /// Like [`Self::show_ui`], with `header` (a search field, filter chips) fixed at the top of + /// the menu and only `list` scrolling under it. See [`Menu::show_ui_with_header`]. + pub fn show_ui_with_header( + self, + ui: &mut Ui, + header: impl FnOnce(&mut Ui) -> H, + list: impl FnOnce(&mut Ui, H) -> R, + ) -> InnerResponse> { + self.show(ui, |menu| menu.show_ui_with_header(header, list)) + } + + /// Like [`Self::show_ui_with_header`], with a list of rows of one height that only lays out + /// the rows in view. See [`Menu::show_rows_with_header`]. + pub fn show_rows_with_header( + self, + ui: &mut Ui, + header: impl FnOnce(&mut Ui) -> H, + row_height: f32, + total_rows: impl FnOnce(&H) -> usize, + rows: impl FnOnce(&mut Ui, Range, H) -> R, + ) -> InnerResponse> { + self.show(ui, |menu| menu.show_rows_with_header(header, row_height, total_rows, rows)) + } + + /// Like [`Self::show_ui_with_header`], with `footer` (an "Add…" row) fixed under the list. + /// See [`Menu::show_ui_with_header_footer`]. + pub fn show_ui_with_header_footer( + self, + ui: &mut Ui, + header: impl FnOnce(&mut Ui) -> H, + list: impl FnOnce(&mut Ui, H) -> R, + footer: impl FnOnce(&mut Ui) -> F, + ) -> InnerResponse> { + self.show(ui, |menu| menu.show_ui_with_header_footer(header, list, footer)) + } + + /// Like [`Self::show_rows_with_header`], with `footer` fixed under the list. See + /// [`Menu::show_ui_with_header_footer`]. + pub fn show_rows_with_header_footer( + self, + ui: &mut Ui, + header: impl FnOnce(&mut Ui) -> H, + row_height: f32, + total_rows: impl FnOnce(&H) -> usize, + rows: impl FnOnce(&mut Ui, Range, H) -> R, + footer: impl FnOnce(&mut Ui) -> F, + ) -> InnerResponse> { + self.show(ui, |menu| menu.show_rows_with_header_footer(header, row_height, total_rows, rows, footer)) + } + + fn show(self, ui: &mut Ui, open: impl FnOnce(Menu<'_>) -> Option) -> InnerResponse> { + let button_id = ui.make_persistent_id(self.id_salt); + show(ui, button_id, self, open) + } + + /// Shows `get(i)` for `i` in `0..len` as the choices, with `*selected` chosen. The response is + /// marked changed when a choice is picked. + pub fn show_index>(self, ui: &mut Ui, selected: &mut usize, len: usize, get: impl Fn(usize) -> Text) -> Response { + let combo = self.selected_text(get(*selected)); + let mut changed = false; + let mut response = combo + .show_ui(ui, |ui| { + for i in 0..len { + if ui.selectable_label(i == *selected, get(i)).clicked() { + *selected = i; + changed = true; + } + } + }) + .response; + if changed { + response.mark_changed(); + } + response + } + + /// Whether the menu of the combo box `id` (its response's id) is open. + pub fn is_open(ctx: &Context, id: Id) -> bool { + Popup::is_id_open(ctx, popup_id(id)) + } +} + +impl HasClasses for ComboBox<'_> { + fn classes(&self) -> &Classes { + &self.classes + } + + fn classes_mut(&mut self) -> &mut Classes { + &mut self.classes + } +} + +fn popup_id(button_id: Id) -> Id { + button_id.with("popup") +} + +// The button is egui 0.37's `combo_box_dyn`, so a combo box looks the same as egui's. +fn show(ui: &mut Ui, button_id: Id, mut combo: ComboBox<'_>, open: impl FnOnce(Menu<'_>) -> Option) -> InnerResponse> { + let is_popup_open = Popup::is_id_open(ui.ctx(), popup_id(button_id)); + let wrap_mode = combo.wrap_mode.unwrap_or_else(|| ui.wrap_mode()); + let close_behavior = combo.close_behavior.unwrap_or(PopupCloseBehavior::CloseOnClick); + combo.set_class(ComboBox::CLASS_OPEN, is_popup_open); + let ComboBox { label, selected_text, width, height, icon, popup_style, classes, .. } = combo; + + // Built from the same atoms and style as a `Button`, so the two line up when put side by side. + let ButtonStyle { atom_layout: atom_layout_style } = ui.widget_style(button_id, &classes); + + // The visuals passed to the icon painter, from the interaction state at the start of the pass. + let visuals = if is_popup_open { + ui.visuals().widgets.open + } else if let Some(response) = ui.ctx().read_response(button_id) { + *ui.style().interact(&response) + } else { + ui.visuals().widgets.inactive + }; + + let min_width = width.unwrap_or_else(|| ui.spacing().combo_width); + let min_size = Vec2::new(min_width, 0.0).at_least(atom_layout_style.min_size); + + let icon = RefCell::new(icon); + let rtl = ui.style().right_to_left; + let icon_atom = Atom::paint(Vec2::splat(ui.spacing().icon_width), move |ui, args| { + let rect = args.rect.expand(visuals.expansion); + if let Some(icon) = icon.borrow_mut().take() { + icon(ui, rect, &visuals, is_popup_open); + } else { + paint_default_icon(ui.painter(), rect, &visuals); + } + }) + .atom_grow(true) + .atom_align(if rtl { Align2::LEFT_CENTER } else { Align2::RIGHT_CENTER }); + let direction = if rtl { Direction::RightToLeft } else { Direction::LeftToRight }; + + let accessible_text = selected_text.text().map(String::from).unwrap_or_default(); + let button = atom_layout_style + .apply( + WidgetAtom::new((selected_text, icon_atom)) + .id(button_id) + .sense(Sense::click()) + .fallback_font(TextStyle::Button) + .wrap_mode(wrap_mode) + .direction(direction), + ) + .min_size(min_size); + + let (button_response, label_response) = match label { + Some(label) => { + let label_text = label.text().map(String::from).unwrap_or_default(); + let outer_response = + WidgetAtom::new((Atom::widget(button), label)).gap(ui.spacing().item_spacing.x).direction(direction).show(ui).response; + outer_response.widget_info(|| WidgetInfo::labeled(Role::Label, ui.is_enabled(), &label_text)); + let button_response = ui.ctx().read_response(button_id).unwrap_or_else(|| outer_response.clone()); + (button_response, Some((outer_response, !label_text.is_empty()))) + } + None => (button.show(ui).response, None), + }; + + button_response.widget_info(|| { + let mut info = WidgetInfo::new(Role::ComboBox); + info.enabled = ui.is_enabled(); + info.current_text_value = Some(accessible_text.clone()); + info + }); + + let popup = Popup::menu(&button_response).id(popup_id(button_id)); + let mut menu = Menu::with_popup(popup, ui, &button_response).style(popup_style).close_behavior(close_behavior); + if let Some(height) = height { + menu = menu.height(height); + } + let inner = open(menu); + + InnerResponse { + inner, + response: match label_response { + // An empty label names nothing, so the caller names the combo box. + Some((label_response, true)) => button_response.labelled_by(label_response.id) | label_response, + Some((label_response, false)) => button_response | label_response, + None => button_response, + }, + } +} + +fn paint_default_icon(painter: &Painter, rect: Rect, visuals: &WidgetVisuals) { + let rect = Rect::from_center_size(rect.center(), vec2(rect.width() * 0.7, rect.height() * 0.45)); + painter.add(Shape::rotated_triangle(rect, 0.0, visuals.fg_stroke.color)); +} + +/// The menu of a combo box, under a field an app draws itself: [`ComboBox`]'s menu without the +/// button. A click on the field opens and closes it, or the caller does with [`Self::open`]; the +/// menu is at least as wide as the field. +/// +/// The menu opens below the field, or above it when there is more room there, and stays on that +/// side while it is open. It is never taller than the room on that side: a long list scrolls. +/// +/// Searchable menus use the header forms ([`Self::show_ui_with_header`] and the rest): the header +/// stays in place and only the list scrolls. Anything fixed above the list goes in the header, +/// such as a "none" row above the search field; anything fixed below it goes in the footer. +/// [`Self::show_rows_with_header`] is for rows of one height; a list of rows of different +/// heights, or a tree, uses [`Self::show_ui_with_header`]. +/// +/// ``` +/// # egui::__run_test_ui(|ui| { +/// let field = ui.button("Font"); +/// craft_ui::combo::Menu::new(ui, &field).height(400.0).show_ui(|ui| ui.label("Arial")); +/// # }); +/// ``` +#[must_use = "You should call .show*"] +pub struct Menu<'a> { + popup: Popup<'a>, + ctx: Context, + field: Rect, + field_id: Id, + height: f32, + style: StyleModifier, + close_behavior: PopupCloseBehavior, + open: Option<&'a mut bool>, +} + +impl<'a> Menu<'a> { + /// The menu under `field`, a widget in `ui`. + pub fn new(ui: &Ui, field: &Response) -> Self { + Self::with_popup(Popup::menu(field), ui, field) + } + + fn with_popup(popup: Popup<'a>, ui: &Ui, field: &Response) -> Self { + // Below the field or above it, never beside it. + let flipped: &'static [RectAlign] = if ui.ctx().global_style().right_to_left { &[RectAlign::TOP_END] } else { &[RectAlign::TOP_START] }; + Self { + popup: popup.align_alternatives(flipped), + ctx: field.ctx.clone(), + field: field.rect, + field_id: field.id, + height: ui.spacing().combo_height, + style: StyleModifier::from(egui::containers::menu::menu_style), + close_behavior: PopupCloseBehavior::CloseOnClick, + open: None, + } + } + + /// The maximum height of the menu's contents, inside its frame, kept to the room on its side + /// of the field. Default: [`egui::Spacing::combo_height`]. + #[inline] + pub fn height(mut self, height: f32) -> Self { + self.height = height; + self + } + + /// When the menu closes. Default: [`PopupCloseBehavior::CloseOnClick`]. + #[inline] + pub fn close_behavior(mut self, close_behavior: PopupCloseBehavior) -> Self { + self.close_behavior = close_behavior; + self + } + + /// Shows the menu while `*open`, for a field that isn't a button, such as a text field that + /// searches the menu while it keeps the keyboard focus. The caller sets `*open` to open the + /// menu, and the menu clears it when it closes: on its close behavior, Escape, + /// [`Ui::close`] inside it, or the field losing the focus to anything but the menu. Clicks on + /// the field neither open nor close it. The menu doesn't take the focus. + /// + /// ``` + /// # egui::__run_test_ui(|ui| { + /// # let (mut text, mut open) = (String::new(), false); + /// let field = ui.text_edit_singleline(&mut text); + /// open |= field.gained_focus() || field.changed(); + /// craft_ui::combo::Menu::new(ui, &field).open(&mut open).show_ui(|ui| ui.label("Arial")); + /// # }); + /// ``` + #[inline] + pub fn open(mut self, open: &'a mut bool) -> Self { + self.open = Some(open); + self + } + + /// The menu's style. Default: egui's menu style. + #[inline] + pub fn style(mut self, style: impl Into) -> Self { + self.style = style.into(); + self + } + + /// Shows `add` in the menu while it is open, scrolling when it is taller than the menu. + /// Returns what `add` returned, or `None` while the menu is closed. + pub fn show_ui(self, add: impl FnOnce(&mut Ui) -> R) -> Option { + self.show(|_| (), |scroll, ui, ()| scroll.show(ui, add).inner, None::).map(|(r, _)| r) + } + + /// Shows `header` at the top of the menu and `list` under it while the menu is open, and + /// returns what `list` returned. `list` gets what `header` returned, such as the rows a + /// search field's text matches. Only `list` scrolls; the header stays in place. The menu is + /// as tall as both, up to its height. `list` lays out in a vertical scroll area: give it + /// rows, not a scroll area of its own. + pub fn show_ui_with_header(self, header: impl FnOnce(&mut Ui) -> H, list: impl FnOnce(&mut Ui, H) -> R) -> Option { + self.show(header, |scroll, ui, h| scroll.show(ui, |ui| list(ui, h)).inner, None::).map(|(r, _)| r) + } + + /// Like [`Self::show_ui_with_header`], with a list of `total_rows(&header's)` rows, each + /// `row_height` tall without the item spacing, that only lays out the rows in view: `rows` + /// gets the range to add, as in [`ScrollArea::show_rows`], and what `header` returned. For + /// rows of one height only. + pub fn show_rows_with_header( + self, + header: impl FnOnce(&mut Ui) -> H, + row_height: f32, + total_rows: impl FnOnce(&H) -> usize, + rows: impl FnOnce(&mut Ui, Range, H) -> R, + ) -> Option { + self.show(header, rows_list(row_height, total_rows, rows), None::).map(|(r, _)| r) + } + + /// Like [`Self::show_ui_with_header`], with `footer` (an "Add…" row) fixed under the list. + /// Returns what `list` and `footer` returned. + pub fn show_ui_with_header_footer( + self, + header: impl FnOnce(&mut Ui) -> H, + list: impl FnOnce(&mut Ui, H) -> R, + footer: impl FnOnce(&mut Ui) -> F, + ) -> Option<(R, F)> { + self.show(header, |scroll, ui, h| scroll.show(ui, |ui| list(ui, h)).inner, Some(footer)).and_then(|(r, f)| Some((r, f?))) + } + + /// Like [`Self::show_rows_with_header`], with `footer` fixed under the list. + pub fn show_rows_with_header_footer( + self, + header: impl FnOnce(&mut Ui) -> H, + row_height: f32, + total_rows: impl FnOnce(&H) -> usize, + rows: impl FnOnce(&mut Ui, Range, H) -> R, + footer: impl FnOnce(&mut Ui) -> F, + ) -> Option<(R, F)> { + self.show(header, rows_list(row_height, total_rows, rows), Some(footer)).and_then(|(r, f)| Some((r, f?))) + } + + fn show( + self, + header: impl FnOnce(&mut Ui) -> H, + list: impl FnOnce(ScrollArea, &mut Ui, H) -> R, + footer: Option F>, + ) -> Option<(R, Option)> { + let Self { popup, ctx, field, field_id, height, style, mut close_behavior, open } = self; + let id = popup.get_id(); + let popup = match open { + Some(open) => { + let focus_key = id.with("craft-field-focus"); + let focused = ctx.memory(|m| m.has_focus(field_id)); + let had_focus = ctx.data_mut(|d| std::mem::replace(d.get_temp_mut_or_default::(focus_key), focused)); + if had_focus && !focused && !moved_into(&ctx, id) { + *open = false; + } + // A click in the field (moving a text field's cursor) leaves the menu open. + if ctx.input(|i| i.pointer.interact_pos()).is_some_and(|pos| field.contains(pos)) { + close_behavior = PopupCloseBehavior::IgnoreClicks; + } + popup.open_bool(open) + } + None => popup, + }; + let footer_id = id.with("craft-footer-height"); + let remeasure_id = id.with("craft-remeasure"); + let remeasure = ctx.data_mut(|d| d.remove_temp::(remeasure_id)).unwrap_or(false); + let (align, settled) = side(&popup, remeasure); + // At most two remeasures per open, so a footer that changes height on every pass settles. + let count_id = id.with("craft-remeasures"); + if ctx.read_response(id).is_none() { + ctx.data_mut(|d| d.remove::(count_id)); + } + let remeasures = ctx.data(|d| d.get_temp::(count_id)).unwrap_or(0); + // Until the side is settled, the menu is measured against the whole screen. + let room = match popup.get_anchor_rect() { + Some(anchor) if settled => room(align, anchor, ctx.content_rect()), + _ => ctx.content_rect().height(), + }; + let mut popup_style = (*ctx.global_style()).clone(); + style.apply(&mut popup_style); + let frame = Frame::popup(&popup_style).total_margin().sum().y; + let height = height.min(room - frame).max(0.0); + let inner = popup + .align(align) + .align_alternatives(&[]) + .width(field.width()) + .style(style) + .close_behavior(close_behavior) + .show(|ui| { + // The pass that measures the menu before its side is settled: drawn at a side that + // may change and at a height the room doesn't cap. Laid out and interactive (a + // search field takes the focus), but not seen. + if remeasure && !settled { + ui.multiply_opacity(0.0); + } + ui.set_min_width(ui.available_width()); + // The popup offers last pass's height. Offer the full height instead: the scroll + // area takes what the header and footer leave of it, then shrinks to the list. + ui.set_max_height(height); + let header = header(ui); + // The footer comes after the list; keep the room it took last pass. + let footer_height = ui.data(|d| d.get_temp::(footer_id)); + let footer_room = if footer.is_some() { footer_height.unwrap_or(0.0) + ui.spacing().item_spacing.y } else { 0.0 }; + // Labels widen a narrow menu rather than wrap early, as in egui's combo box. A + // list left with no room keeps ScrollArea's minimum scrolled height. + ui.style_mut().wrap_mode = Some(TextWrapMode::Extend); + let list = list(ScrollArea::vertical().max_height((ui.available_height() - footer_room).max(0.0)), ui, header); + let footer = footer.map(|footer| { + let top = ui.cursor().top(); + let f = footer(ui); + let height = ui.min_rect().bottom() - top; + if footer_height.is_none_or(|h| (h - height).abs() > 0.5) && remeasures < 2 { + // The list took the wrong room: lay the menu out again next pass. Not a + // discarded pass: the header's side effects (focusing a search field on + // open) would run twice. + ui.data_mut(|d| { + d.insert_temp(footer_id, height); + d.insert_temp(remeasure_id, true); + d.insert_temp(count_id, remeasures + 1); + }); + ui.ctx().request_repaint(); + } + f + }); + (list, footer) + }) + .map(|r| r.inner); + if inner.is_some() { + ctx.accesskit_node_builder(id, |node| node.set_role(Role::ListBox)); + } + inner + } +} + +fn rows_list( + row_height: f32, + total_rows: impl FnOnce(&H) -> usize, + rows: impl FnOnce(&mut Ui, Range, H) -> R, +) -> impl FnOnce(ScrollArea, &mut Ui, H) -> R { + move |scroll, ui, h| { + let total = total_rows(&h); + scroll.show_rows(ui, row_height, total, |ui, range| rows(ui, range, h)).inner + } +} + +/// Whether the pointer or the keyboard focus went into the menu `popup` this pass. +fn moved_into(ctx: &Context, popup: Id) -> bool { + // The pointer counts only on a press or click: a resting mouse says nothing about focus. + let pointer = ctx.input(|i| (i.pointer.any_pressed() || i.pointer.any_click()).then(|| i.pointer.interact_pos())).flatten(); + let in_menu = ctx.memory(|m| m.area_rect(popup)).zip(pointer).is_some_and(|(menu, pos)| menu.contains(pos)); + let focused = ctx.memory(|m| m.focused()).and_then(|id| ctx.read_response(id)); + in_menu || focused.is_some_and(|r| r.layer_id == LayerId::new(Order::Foreground, popup)) +} + +/// The side of its field `popup` shows on, and whether it is settled. The side is settled on the +/// first pass after the menu was shown and measured (`remeasure` when that measure is stale): the +/// side egui picks when the menu fits there, else the one with more room. It is kept while the +/// menu stays open, so a menu that shrinks or grows while open doesn't jump to the other side. +fn side(popup: &Popup<'_>, remeasure: bool) -> (RectAlign, bool) { + let ctx = popup.ctx(); + let key = popup.get_id().with("craft-side"); + if !popup.is_open() || ctx.read_response(popup.get_id()).is_none() { + ctx.data_mut(|d| d.remove::(key)); + return (popup.get_best_align(), false); + } + if let Some(align) = ctx.data(|d| d.get_temp::(key)) { + return (align, true); + } + if remeasure { + return (popup.get_best_align(), false); + } + // Outside the data lock: these read the popup's state from memory. + let mut align = popup.get_best_align(); + if let (Some(size), Some(anchor)) = (popup.get_expected_size(), popup.get_anchor_rect()) { + let screen = ctx.content_rect(); + let flipped = align.flipped_y(); + if room(align, anchor, screen) < size.y && room(flipped, anchor, screen) > room(align, anchor, screen) { + align = flipped; + } + } + ctx.data_mut(|d| d.insert_temp(key, align)); + (align, true) +} + +/// The height a menu on the `align` side of `anchor` has inside `screen`. +fn room(align: RectAlign, anchor: Rect, screen: Rect) -> f32 { + match (align.parent.y(), align.child.y()) { + (Align::Max, Align::Min) => screen.bottom() - anchor.bottom(), + (Align::Min, Align::Max) => anchor.top() - screen.top(), + _ => screen.height(), + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/ui/src/combo/tests.rs b/crates/ui/src/combo/tests.rs new file mode 100644 index 000000000..48e02da2e --- /dev/null +++ b/crates/ui/src/combo/tests.rs @@ -0,0 +1,526 @@ +use egui_kittest::{Harness, kittest::Queryable as _}; + +use super::ComboBox; + +struct Menu { + filter: String, + button: Option, +} + +fn popup(h: &Harness<'_, Menu>) -> egui::Id { + super::popup_id(h.state().button.unwrap_or(egui::Id::NULL)) +} + +/// The menu's height as egui keeps it between passes. +fn height(h: &Harness<'_, Menu>) -> f32 { + egui::AreaState::load(&h.ctx, popup(h)).and_then(|s| s.size).map_or(0.0, |s| s.y) +} + +#[test] +fn a_filtered_menu_grows_back_when_the_filter_is_cleared() { + let mut h = Harness::builder().with_size(egui::vec2(600.0, 800.0)).build_ui_state( + |ui, m: &mut Menu| { + let inner = ComboBox::new("language", "Language").selected_text("Any language").width(200.0).height(380.0).show_ui(ui, |ui| { + let name = ui.label("Find a language").id; + ui.text_edit_singleline(&mut m.filter).labelled_by(name); + let rows = if m.filter.is_empty() { 60 } else { 0 }; + let row = ui.spacing().interact_size.y; + egui::ScrollArea::vertical().max_height(300.0).auto_shrink([false, true]).show_rows(ui, row, rows, |ui, range| { + for n in range { + let _ = ui.selectable_label(false, format!("Language {n}")); + } + }); + }); + m.button = Some(inner.response.id); + }, + Menu { filter: String::new(), button: None }, + ); + h.run(); + egui::Popup::open_id(&h.ctx, popup(&h)); + h.run(); + let full = height(&h); + assert!(full > 300.0, "the open menu shows the list: {full}"); + + h.state_mut().filter = "xxxx".into(); + h.run(); + let none = height(&h); + assert!(none < 100.0, "no matches shrink the menu: {none}"); + + h.state_mut().filter.clear(); + h.run(); + let again = height(&h); + assert!((again - full).abs() < 1.0, "cleared, the menu is {again} tall, not {full} as when it opened"); +} + +struct Search { + filter: String, + scroll: bool, + field: egui::Rect, + first_row: usize, + button: Option, +} + +fn search_menu() -> Harness<'static, Search> { + Harness::builder().with_size(egui::vec2(600.0, 800.0)).build_ui_state( + |ui, m: &mut Search| { + let row = ui.spacing().interact_size.y; + let header = |ui: &mut egui::Ui| { + let name = ui.label("Find a language").id; + m.field = ui.text_edit_singleline(&mut m.filter).labelled_by(name).rect; + if m.filter.is_empty() { 60 } else { 0 } + }; + let rows = |ui: &mut egui::Ui, range: std::ops::Range, _: usize| { + if std::mem::take(&mut m.scroll) { + ui.scroll_with_delta(egui::vec2(0.0, -200.0)); + } + m.first_row = range.start; + for n in range { + let _ = ui.selectable_label(false, format!("Language {n}")); + } + }; + let inner = ComboBox::new("language", "Language").selected_text("Any language").width(200.0).height(380.0).show_rows_with_header( + ui, + header, + row, + |&n| n, + rows, + ); + m.button = Some(inner.response.id); + }, + Search { filter: String::new(), scroll: false, field: egui::Rect::NOTHING, first_row: 0, button: None }, + ) +} + +fn search_height(h: &Harness<'_, Search>) -> f32 { + let popup = super::popup_id(h.state().button.unwrap_or(egui::Id::NULL)); + egui::AreaState::load(&h.ctx, popup).and_then(|s| s.size).map_or(0.0, |s| s.y) +} + +#[test] +fn a_menu_header_stays_put_while_the_list_scrolls_and_the_menu_grows_back() { + let mut h = search_menu(); + h.run(); + h.get_by_role_and_label(egui::Role::ComboBox, "Language").click(); + h.run(); + let (field, full) = (h.state().field, search_height(&h)); + assert!(full > 300.0, "the open menu shows the list: {full}"); + assert!(full < 400.0, "the menu's contents keep to its height of 380, inside its frame: {full}"); + + h.state_mut().scroll = true; + h.run(); + assert!(h.state().first_row > 0, "the list scrolled"); + assert_eq!(h.state().field, field, "the search field stays put"); + + h.state_mut().filter = "xxxx".into(); + h.run(); + let none = search_height(&h); + assert!(none < 100.0, "no matches shrink the menu: {none}"); + + h.state_mut().filter.clear(); + h.run(); + let again = search_height(&h); + assert!((again - full).abs() < 1.0, "cleared, the menu is {again} tall, not {full} as when it opened"); +} + +const SIZES: [&str; 3] = ["Small", "Medium", "Large"]; + +#[test] +fn show_index_picks_a_choice_and_marks_the_response_changed() { + let mut h = Harness::new_ui_state( + |ui, s: &mut (usize, bool)| { + let response = ComboBox::from_label("Size").show_index(ui, &mut s.0, SIZES.len(), |i| SIZES[i]); + s.1 |= response.changed(); + }, + (0, false), + ); + h.run(); + h.get_by_role_and_label(egui::Role::ComboBox, "Size").click(); + h.run(); + h.get_by_label("Large").click(); + h.run(); + assert_eq!(*h.state(), (2, true)); + assert!(h.query_by_label("Medium").is_none(), "picking closes the menu"); +} + +fn sizes(ui: &mut egui::Ui, rows: usize) { + for n in 0..rows { + let size = SIZES.get(n).map_or_else(|| format!("Row {n}"), |s| (*s).to_owned()); + let _ = ui.selectable_label(n == 1, size); + } +} + +/// Combo boxes in the states a user sees them in, drawn with egui's combo box or ours. +#[expect(clippy::disallowed_types, reason = "the reference our combo box is compared with")] +fn scene(ui: &mut egui::Ui, ours: bool) { + macro_rules! both { + ($ui:ident, $rows:expr; $ctor:ident ( $($arg:expr),* ) $(. $m:ident ( $($a:expr),* ))*) => { + if ours { + ComboBox::$ctor($($arg),*)$(.$m($($a),*))*.show_ui($ui, |ui| sizes(ui, $rows)).response + } else { + egui::ComboBox::$ctor($($arg),*)$(.$m($($a),*))*.show_ui($ui, |ui| sizes(ui, $rows)).response + } + }; + } + let _ = both!(ui, 3; from_label("Size").selected_text("Medium")); + let _ = both!(ui, 3; from_id_salt("narrow").selected_text("A long selected value that is cut short").width(120.0).truncate()); + let _ = both!(ui, 3; new("wrapped", "Wrapped").selected_text("A long selected value that wraps onto a second line").width(140.0).wrap()); + let _ = ui.add_enabled_ui(false, |ui| both!(ui, 3; new("disabled", "Disabled").selected_text("Medium"))); + let _ = both!(ui, 3; new("open", "Open").selected_text("Medium").width(160.0)); + let _ = both!(ui, 40; new("long", "Long").selected_text("Row 1").height(300.0)); +} + +fn render(ours: bool, hover: Option<&str>, open: Option<&str>) -> image::RgbaImage { + // Two of the combo boxes have no label: the scene compares looks, not names. + let mut h = Harness::builder().with_size(egui::vec2(480.0, 700.0)).with_accessibility_check(false).build_ui(move |ui| scene(ui, ours)); + h.run(); + if let Some(label) = open { + h.get_by_role_and_label(egui::Role::ComboBox, label).click(); + h.run(); + } + if let Some(label) = hover { + h.get_by_role_and_label(egui::Role::ComboBox, label).hover(); + } + h.run(); + h.render().unwrap_or_else(|e| panic!("render: {e}")) +} + +#[test] +fn the_button_and_menu_look_like_eguis() { + for (hover, open) in [(None, None), (Some("Size"), None), (None, Some("Open")), (None, Some("Long"))] { + let (egui, ours) = (render(false, hover, open), render(true, hover, open)); + let differ = egui.pixels().zip(ours.pixels()).filter(|(a, b)| a != b).count(); + assert_eq!(differ, 0, "hover {hover:?}, open {open:?}: {differ} pixels differ"); + } +} + +/// A combo box at `top` in a window `screen` tall, with `rows` rows (none while filtered). +struct Long { + rows: usize, + filtered: bool, + to_end: bool, + last_row: egui::Rect, + button: egui::Rect, + popup: egui::Id, +} + +fn long_menu(top: f32, screen: f32, height: f32, rows: usize) -> Harness<'static, Long> { + Harness::builder().with_size(egui::vec2(400.0, screen)).build_ui_state( + move |ui, m: &mut Long| { + ui.add_space(top); + let inner = ComboBox::new("long", "Long").selected_text("Row 0").height(height).show_ui(ui, |ui| { + let shown = if m.filtered { 2 } else { m.rows }; + for n in 0..shown { + m.last_row = ui.selectable_label(false, format!("Row {n}")).rect; + } + if std::mem::take(&mut m.to_end) { + ui.scroll_to_cursor(Some(egui::Align::BOTTOM)); + } + }); + m.button = inner.response.rect; + m.popup = super::popup_id(inner.response.id); + }, + Long { rows, filtered: false, to_end: false, last_row: egui::Rect::NOTHING, button: egui::Rect::NOTHING, popup: egui::Id::NULL }, + ) +} + +fn menu_rect(h: &Harness<'_, Long>) -> egui::Rect { + h.ctx.memory(|m| m.area_rect(h.state().popup)).unwrap_or(egui::Rect::NOTHING) +} + +#[test] +fn an_unbounded_menu_fits_on_screen_and_scrolls_to_its_last_row() { + let mut h = long_menu(0.0, 300.0, f32::INFINITY, 100); + h.run(); + h.get_by_role_and_label(egui::Role::ComboBox, "Long").click(); + h.run(); + let menu = menu_rect(&h); + assert!(menu.bottom() <= 300.0 + 0.5, "the menu ends on screen: {menu:?}"); + h.state_mut().to_end = true; + h.run(); + let last = h.state().last_row; + assert!(last.bottom() <= menu_rect(&h).bottom() && last.top() >= menu_rect(&h).top(), "the last row {last:?} scrolls into the menu {menu:?}"); +} + +#[test] +fn a_menu_that_shrinks_and_grows_while_open_stays_on_its_side() { + // Too little room below for the full list: the menu opens above. Filtered, it would fit + // below too. + let mut h = long_menu(480.0, 600.0, 380.0, 60); + h.run(); + h.get_by_role_and_label(egui::Role::ComboBox, "Long").click(); + h.run(); + let above = |h: &Harness<'_, Long>| menu_rect(h).bottom() <= h.state().button.top() + 0.5; + assert!(above(&h), "opens above: {:?}", menu_rect(&h)); + h.state_mut().filtered = true; + h.run(); + assert!(above(&h), "stays above when it shrinks: {:?}", menu_rect(&h)); + h.state_mut().filtered = false; + h.run(); + assert!(above(&h), "and when it grows back: {:?}", menu_rect(&h)); + assert!(menu_rect(&h).top() >= -0.5, "keeping to the room there: {:?}", menu_rect(&h)); +} + +#[test] +fn a_menu_opened_by_the_app_settles_its_side_once_measured() { + // Opened with `Popup::open_id`, as photocraft's press-and-drag menus are, so it is open on + // the pass it is first shown. + let mut h = long_menu(480.0, 600.0, 380.0, 60); + h.run(); + egui::Popup::open_id(&h.ctx, h.state().popup); + h.run(); + let menu = menu_rect(&h); + assert!(menu.bottom() <= h.state().button.top() + 0.5, "opens above, where there is room: {menu:?}"); + assert!(menu.height() > 300.0, "with the whole list: {menu:?}"); +} + +#[test] +fn a_footer_stays_under_the_list() { + let mut h = Harness::builder().with_size(egui::vec2(400.0, 600.0)).build_ui_state( + |ui, s: &mut (bool, egui::Rect, egui::Rect)| { + let _ = ComboBox::new("buses", "Send").selected_text("Bus 1").height(300.0).show_ui_with_header_footer( + ui, + |ui| ui.label("Buses"), + |ui, _| { + for n in 0..50 { + let _ = ui.selectable_label(false, format!("Bus {n}")); + } + if std::mem::take(&mut s.0) { + ui.scroll_with_delta(egui::vec2(0.0, -200.0)); + } + s.1 = ui.clip_rect(); + }, + |ui| s.2 = ui.button("New bus…").rect, + ); + }, + (false, egui::Rect::NOTHING, egui::Rect::NOTHING), + ); + h.run(); + h.get_by_role_and_label(egui::Role::ComboBox, "Send").click(); + h.run(); + let (list, footer) = (h.state().1, h.state().2); + assert!(footer.top() >= list.bottom() - 0.5, "the footer {footer:?} is under the list {list:?}"); + h.state_mut().0 = true; + h.run(); + assert_eq!(h.state().2, footer, "the footer stays put while the list scrolls"); + assert!(h.query_by_label("New bus…").is_some()); +} + +struct Typed { + text: String, + open: bool, + picked: Option, +} + +/// A text field that searches its menu: the menu shows while `open`, which the field sets. +fn font_field() -> Harness<'static, Typed> { + Harness::new_ui_state( + |ui, t: &mut Typed| { + let name = ui.label("Font").id; + let field = ui.text_edit_singleline(&mut t.text).labelled_by(name); + t.open |= field.gained_focus() || field.changed(); + let text = t.text.clone(); + let menu = super::Menu::new(ui, &field).open(&mut t.open); + if let Some(Some(n)) = menu.show_ui(|ui| (0..3).find(|n| ui.selectable_label(false, format!("{text} {n}")).clicked())) { + t.picked = Some(n); + } + }, + Typed { text: String::new(), open: false, picked: None }, + ) +} + +#[test] +fn a_menu_under_a_text_field_leaves_it_the_focus_and_takes_a_pick() { + let mut h = font_field(); + h.run(); + h.get_by_role_and_label(egui::Role::TextInput, "Font").click(); + h.run(); + assert!(h.state().open, "focusing the field opens the menu"); + h.get_by_role_and_label(egui::Role::TextInput, "Font").type_text("ar"); + h.run(); + assert_eq!(h.state().text, "ar"); + assert!(h.get_by_role_and_label(egui::Role::TextInput, "Font").is_focused(), "the field keeps the focus"); + assert!(h.query_by_label("ar 1").is_some(), "the menu follows the text"); + + h.get_by_role_and_label(egui::Role::TextInput, "Font").click(); + h.run(); + assert!(h.state().open, "a click in the field leaves the menu open"); + + h.get_by_label("ar 1").click(); + h.run(); + assert_eq!(h.state().picked, Some(1), "a click on a row picks it"); + assert!(!h.state().open && h.query_by_label("ar 1").is_none(), "and closes the menu"); + + h.get_by_role_and_label(egui::Role::TextInput, "Font").click(); + h.run(); + assert!(h.state().open && h.query_by_label("ar 1").is_some(), "focusing the field opens it again"); + h.key_press(egui::Key::Escape); + h.run(); + assert!(!h.state().open && h.query_by_label("ar 1").is_none(), "Escape closes it"); +} + +#[test] +fn a_menu_under_a_text_field_closes_when_the_focus_leaves_it() { + for mouse_over_menu in [false, true] { + let mut h = Harness::new_ui_state( + |ui, t: &mut Typed| { + let name = ui.label("Font").id; + let field = ui.text_edit_singleline(&mut t.text).labelled_by(name); + t.open |= field.gained_focus() || field.changed(); + let _ = ui.button("Other"); + let menu = super::Menu::new(ui, &field).open(&mut t.open); + let _ = menu.show_ui(|ui| ui.label("Fonts")); + }, + Typed { text: String::new(), open: false, picked: None }, + ); + h.run(); + h.get_by_role_and_label(egui::Role::TextInput, "Font").click(); + h.run(); + assert!(h.state().open); + if mouse_over_menu { + h.get_by_label("Fonts").hover(); + h.run(); + assert!(h.state().open, "hovering the menu keeps it open"); + } + h.key_press(egui::Key::Tab); + h.run(); + h.run(); + assert!(!h.state().open, "Tab to another widget closes the menu (mouse over menu {mouse_over_menu})"); + } +} + +#[test] +fn a_click_on_the_canvas_closes_a_menu_under_a_text_field() { + let mut h = font_field(); + h.run(); + h.get_by_role_and_label(egui::Role::TextInput, "Font").click(); + h.run(); + assert!(h.state().open); + h.hover_at(egui::pos2(380.0, 280.0)); + h.run(); + for pressed in [true, false] { + h.event(egui::Event::PointerButton { + pos: egui::pos2(380.0, 280.0), + button: egui::PointerButton::Primary, + pressed, + modifiers: egui::Modifiers::NONE, + }); + h.run(); + } + h.run(); + h.run(); + assert!(!h.state().open, "a click elsewhere closes the menu"); +} + +#[test] +fn a_click_opened_menu_under_a_text_field_leaves_it_the_focus_while_typing() { + let mut h = Harness::new_ui_state( + |ui, shown: &mut (String, bool)| { + let name = ui.label("Font").id; + let field = ui.text_edit_singleline(&mut shown.0).labelled_by(name); + let text = shown.0.clone(); + let menu = super::Menu::new(ui, &field).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside); + shown.1 = menu.show_ui(|ui| ui.label(format!("Fonts matching {text:?}"))).is_some(); + }, + (String::new(), false), + ); + h.run(); + h.get_by_role_and_label(egui::Role::TextInput, "Font").click(); + h.run(); + h.get_by_role_and_label(egui::Role::TextInput, "Font").type_text("ar"); + h.run(); + assert_eq!(h.state().0, "ar"); + assert!(h.get_by_role_and_label(egui::Role::TextInput, "Font").is_focused(), "the field keeps the focus"); + assert!(h.state().1, "the menu stays open"); + assert!(h.query_by_label("Fonts matching \"ar\"").is_some(), "the menu follows the text"); +} + +#[test] +fn a_menu_with_a_footer_shrinks_when_its_list_empties_and_grows_back() { + let mut h = Harness::builder().with_size(egui::vec2(400.0, 800.0)).build_ui_state( + |ui, (filter, seen): &mut (String, Vec)| { + ui.add_space(700.0); + let row = ui.spacing().interact_size.y; + let field = ui.button("Send"); + let _ = super::Menu::new(ui, &field) + .height(260.0) + .close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside) + .show_rows_with_header_footer( + |ui| { + ui.label("Title"); + let name = ui.label("Search").id; + // A search field that takes the focus when the menu opens, as apps do. + let (id, pass) = (ui.scope_id().with("search"), ui.ctx().cumulative_pass_nr()); + let opened = ui.data(|d| d.get_temp::(id)).is_none_or(|last| last + 1 < pass); + ui.data_mut(|d| d.insert_temp(id, pass)); + let field = ui.text_edit_singleline(filter).labelled_by(name); + if opened { + field.request_focus(); + } + if filter.is_empty() { 30 } else { 0 } + }, + row, + |&n| n, + |ui, range, _| { + for n in range { + let _ = ui.selectable_label(false, format!("Bus {n}")); + } + }, + |ui| { + let _ = ui.button("new bus..."); + let remove = ui.button("Remove send"); + // Where the footer is in each frame the user sees in full. + if ui.opacity() >= 1.0 { + seen.push(remove.rect.bottom()); + } + }, + ); + }, + (String::new(), Vec::new()), + ); + h.run(); + h.get_by_label("Send").click(); + h.run(); + // The menu opens above its field, so it shrinks from the top: the search field moves down. + let footer = |h: &Harness<'_, (String, Vec)>| h.get_by_role(egui::Role::TextInput).rect(); + let full = footer(&h); + // Opening shows the menu where it stays: no visible frame at a side or height it then leaves. + let seen = &h.state().1; + assert!(!seen.is_empty() && seen.iter().all(|b| (b - seen[0]).abs() < 0.5), "visible frames moved: {seen:?}"); + h.get_by_role(egui::Role::TextInput).type_text("zzzz"); + h.run(); + let none = footer(&h); + assert_eq!(h.state().0, "zzzz"); + assert!(none.top() > full.top() + 100.0, "no matches shrink the menu: {none:?} vs {full:?}"); + for _ in 0..4 { + h.key_press(egui::Key::Backspace); + h.run(); + } + assert!((footer(&h).top() - full.top()).abs() < 1.0, "cleared, the footer is back: {:?} vs {full:?}", footer(&h)); +} + +#[test] +fn a_footer_that_changes_height_every_pass_settles() { + let mut h = Harness::builder().with_size(egui::vec2(400.0, 600.0)).build_ui_state( + |ui, passes: &mut u32| { + let field = ui.button("Send"); + let _ = super::Menu::new(ui, &field).show_ui_with_header_footer( + |ui| ui.label("Title"), + |ui, _| { + let _ = ui.label("Row"); + }, + |ui| { + *passes += 1; + ui.add_space(if (*passes).is_multiple_of(2) { 10.0 } else { 30.0 }); + }, + ); + }, + 0, + ); + h.run(); + h.get_by_label("Send").click(); + h.run(); + let before = *h.state(); + h.run(); + h.run(); + assert!(*h.state() - before <= 4, "the menu keeps remeasuring: {} passes in two frames", *h.state() - before); +} diff --git a/crates/ui/src/lib.rs b/crates/ui/src/lib.rs index b37b23dd8..3c98f5f0c 100644 --- a/crates/ui/src/lib.rs +++ b/crates/ui/src/lib.rs @@ -7,6 +7,7 @@ pub mod buttons; pub mod capture; +pub mod combo; pub mod direction; pub mod docking; mod focus; @@ -22,3 +23,5 @@ pub mod tabs; pub mod testing; pub mod text; pub mod theme; + +pub use combo::ComboBox; diff --git a/crates/ui/src/printing.rs b/crates/ui/src/printing.rs index bdcf55277..8ac771696 100644 --- a/crates/ui/src/printing.rs +++ b/crates/ui/src/printing.rs @@ -213,7 +213,7 @@ impl State { let printer_label = ui.label(labels.printer); let shown = shown_names(&self.printers); let selected = self.printers.iter().zip(&shown).find(|(p, _)| p.name == self.settings.printer).map(|(_, text)| text.as_str()); - egui::ComboBox::from_id_salt("printer") + crate::ComboBox::from_id_salt("printer") .width(width) .wrap_mode(egui::TextWrapMode::Truncate) .selected_text(if self.settings.printer.is_empty() { labels.select_printer } else { selected.unwrap_or(&self.settings.printer) }) @@ -238,7 +238,7 @@ impl State { (Some(Duplex::ShortEdge), labels.short_edge), ]; let duplex_label = ui.label(labels.duplex); - egui::ComboBox::from_id_salt("duplex") + crate::ComboBox::from_id_salt("duplex") .width(width) .wrap_mode(egui::TextWrapMode::Truncate) .selected_text( From d40c2be47d36e6d25747ab5665c4f174b03df5b7 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Tue, 27 Oct 2026 10:26:11 -0400 Subject: [PATCH 06/13] craft-ui: combo footer remeasures reset once the footer is stable (#394) * craft-ui: combo footer remeasures reset once the footer is stable * craft-ui: test combo footer recovery after the remeasure cap --- crates/ui/src/combo.rs | 14 +++++-- crates/ui/src/combo/tests.rs | 74 ++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/crates/ui/src/combo.rs b/crates/ui/src/combo.rs index d0138b7d8..91d70f52b 100644 --- a/crates/ui/src/combo.rs +++ b/crates/ui/src/combo.rs @@ -527,7 +527,8 @@ impl<'a> Menu<'a> { let remeasure_id = id.with("craft-remeasure"); let remeasure = ctx.data_mut(|d| d.remove_temp::(remeasure_id)).unwrap_or(false); let (align, settled) = side(&popup, remeasure); - // At most two remeasures per open, so a footer that changes height on every pass settles. + // At most two remeasures in a row, so a footer that changes height on every pass settles. + // A pass where the footer keeps its height clears the count. let count_id = id.with("craft-remeasures"); if ctx.read_response(id).is_none() { ctx.data_mut(|d| d.remove::(count_id)); @@ -571,12 +572,19 @@ impl<'a> Menu<'a> { let top = ui.cursor().top(); let f = footer(ui); let height = ui.min_rect().bottom() - top; - if footer_height.is_none_or(|h| (h - height).abs() > 0.5) && remeasures < 2 { + let changed = footer_height.is_none_or(|h| (h - height).abs() > 0.5); + if !changed && remeasures > 0 { + ui.data_mut(|d| d.remove::(count_id)); + } + if changed { + // Recorded even past the cap, so "unchanged" means unchanged since the last pass. + ui.data_mut(|d| d.insert_temp(footer_id, height)); + } + if changed && remeasures < 2 { // The list took the wrong room: lay the menu out again next pass. Not a // discarded pass: the header's side effects (focusing a search field on // open) would run twice. ui.data_mut(|d| { - d.insert_temp(footer_id, height); d.insert_temp(remeasure_id, true); d.insert_temp(count_id, remeasures + 1); }); diff --git a/crates/ui/src/combo/tests.rs b/crates/ui/src/combo/tests.rs index 48e02da2e..90e1afb15 100644 --- a/crates/ui/src/combo/tests.rs +++ b/crates/ui/src/combo/tests.rs @@ -524,3 +524,77 @@ fn a_footer_that_changes_height_every_pass_settles() { h.run(); assert!(*h.state() - before <= 4, "the menu keeps remeasuring: {} passes in two frames", *h.state() - before); } + +#[test] +fn a_footer_that_changes_height_after_settling_still_lands() { + // (footer height, bottom of the footer) + let mut h = Harness::builder().with_size(egui::vec2(400.0, 600.0)).build_ui_state( + |ui, s: &mut (f32, f32)| { + let field = ui.button("Send"); + let _ = super::Menu::new(ui, &field).height(300.0).show_ui_with_header_footer( + |ui| ui.label("Title"), + |ui, _| { + for n in 0..50 { + let _ = ui.label(format!("Row {n}")); + } + }, + |ui| { + ui.add_space(s.0); + s.1 = ui.min_rect().bottom(); + }, + ); + }, + (20.0, 0.0), + ); + h.run(); + h.get_by_label("Send").click(); + h.run(); + let bottom = h.state().1; + // The list fills the menu's height, so the footer's bottom stays put when the footer's height + // changes and the list gives or takes the room. Changes are spaced over several passes: the + // remeasure budget resets while the footer is stable. + for (n, height) in [60.0, 20.0, 80.0, 30.0].into_iter().enumerate() { + h.state_mut().0 = height; + h.run(); + let now = h.state().1; + assert!((now - bottom).abs() < 1.0, "change {n} to {height}: the footer ends at {now}, was {bottom}"); + h.run(); + h.run(); + } +} + +#[test] +fn a_footer_that_hits_the_remeasure_cap_recovers_once_it_holds_still() { + // (held footer height, footer passes so far, bottom of the footer) + let mut h = Harness::builder().with_size(egui::vec2(400.0, 600.0)).build_ui_state( + |ui, s: &mut (f32, u32, f32)| { + let field = ui.button("Send"); + let _ = super::Menu::new(ui, &field).height(300.0).show_ui_with_header_footer( + |ui| ui.label("Title"), + |ui, _| { + for n in 0..50 { + let _ = ui.label(format!("Row {n}")); + } + }, + |ui| { + s.1 += 1; + // Grows on each of the first passes, past the cap of two remeasures. + ui.add_space(if s.1 <= 5 { 20.0 + 15.0 * s.1 as f32 } else { s.0 }); + s.2 = ui.min_rect().bottom(); + }, + ); + }, + (40.0, 0, 0.0), + ); + h.run(); + h.get_by_label("Send").click(); + // Passes with a stable footer follow the growth: each one resets the remeasure budget. + for _ in 0..3 { + h.step(); + } + let bottom = h.state().2; + h.state_mut().0 = 90.0; + h.run(); + let now = h.state().2; + assert!((now - bottom).abs() < 1.0, "the footer ends at {now}, was {bottom}"); +} From 6a66b95cbc2c4d4b732630113eb9a37263658c58 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 27 Oct 2026 13:49:38 -0400 Subject: [PATCH 07/13] Enforce Rust coding standards via Clippy (#372) * chore(lints): enforce workspace Clippy standards Deny cognitive_complexity (threshold 30, rust:S3776), too_many_arguments and wildcard_imports workspace-wide. Existing violations are fixed in follow-up commits. * refactor(keymap): isolate shortcut row rendering * test(ui): cover dock command contract cases * fix(keymap): remove unused shortcut layout state * refactor(media): simplify container serialization tests * docs: document workspace lint inheritance for new crates * refactor(text): split Knuth-Plass line costing and path extraction Brings knuth_plass under the workspace cognitive complexity limit. * test(font-store): split lookup test by concern --- CLAUDE.md | 3 + Cargo.toml | 3 + clippy.toml | 1 + crates/font-store/src/tests.rs | 8 + crates/keymap/src/ui/view.rs | 207 ++++++++++++------- crates/media/src/isobmff/codec.rs | 202 ++++++++++-------- crates/media/src/isobmff/demux.rs | 23 ++- crates/media/src/isobmff/mux.rs | 11 +- crates/media/src/matroska/demux.rs | 12 +- crates/media/src/matroska/meta.rs | 17 +- crates/media/src/matroska/mux.rs | 8 +- crates/media/src/matroska/synthetic_tests.rs | 108 +++++----- crates/media/tests/oracle_matroska.rs | 164 ++++++++------- crates/text/src/linebreak/knuth_plass.rs | 101 +++++---- crates/ui/src/docking/command_tests.rs | 62 ++++-- crates/ui/src/text.rs | 200 +++++++++--------- 16 files changed, 687 insertions(+), 443 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 11908a75d..95c995b2e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1 +1,4 @@ Read `AGENTS.md`. + +New workspace crates must include `[lints] workspace = true` in `Cargo.toml` to inherit +`[workspace.lints]`; production crate roots must begin with the required crate-level lint attribute. diff --git a/Cargo.toml b/Cargo.toml index a71f9fd62..daa783f89 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,6 +56,9 @@ unsafe_code = "forbid" unexpected_cfgs = { level = "warn", check-cfg = ["cfg(craft_pdf_unicode_fixtures)"] } [workspace.lints.clippy] +cognitive_complexity = "deny" +too_many_arguments = "deny" +wildcard_imports = "deny" dbg_macro = "warn" todo = "warn" diff --git a/clippy.toml b/clippy.toml index 287e14edc..8f54197d8 100644 --- a/clippy.toml +++ b/clippy.toml @@ -2,6 +2,7 @@ allow-unwrap-in-tests = true allow-expect-in-tests = true allow-panic-in-tests = true allow-indexing-slicing-in-tests = true +cognitive-complexity-threshold = 30 disallowed-types = [ { path = "egui::ComboBox", reason = "use craft_ui::ComboBox: egui's combo box menu can't grow back while open (egui#5225)" }, ] diff --git a/crates/font-store/src/tests.rs b/crates/font-store/src/tests.rs index de1aece83..44b75b1c2 100644 --- a/crates/font-store/src/tests.rs +++ b/crates/font-store/src/tests.rs @@ -192,7 +192,11 @@ fn lookups_match_exactly_and_refuse_near_matches() { assert_eq!(name("Test Var", "Light").unwrap(), "TestVar[wght].ttf"); assert!(matches!(name("Test Var", "Thin"), Err(Unavailable::NoStyle { .. }))); assert!(matches!(name("Test Var", "Bold Italic"), Err(Unavailable::NoStyle { .. }))); +} +#[test] +fn postscript_lookups_match_exactly() { + let index = index_of(&fixture().index).unwrap(); assert_eq!(index.find_postscript("TestSans-Bold").unwrap().family.family, "Test Sans"); assert_eq!(index.find_postscript("TestVar-SemiBold").unwrap().file.file_name(), "TestVar[wght].ttf"); assert!(matches!(index.find_postscript("testsans-bold"), Err(Unavailable::NoPostScript(_)))); @@ -200,7 +204,11 @@ fn lookups_match_exactly_and_refuse_near_matches() { let Err(Unavailable::Ambiguous { families, .. }) = index.find_postscript("Shared-Regular") else { panic!("not ambiguous") }; assert_eq!(families, ["Shared One", "Shared Two"]); assert!(index.family("Shared Two").unwrap().face_by_postscript("Shared-Regular").is_some()); +} +#[test] +fn styles_parse_and_urls_pin_the_commit() { + let index = index_of(&fixture().index).unwrap(); assert_eq!(parse_style("Extra Bold Italic"), Some((800, true))); assert_eq!(parse_style("semi-bold"), Some((600, false))); assert_eq!(parse_style("Condensed Bold"), None); diff --git a/crates/keymap/src/ui/view.rs b/crates/keymap/src/ui/view.rs index 7b46245a1..6922d0aca 100644 --- a/crates/keymap/src/ui/view.rs +++ b/crates/keymap/src/ui/view.rs @@ -894,84 +894,8 @@ impl Draw<'_, '_> { let (g, _) = self.one_line(ui.painter(), &self.ordered(parts), label_w); ui.painter().galley(pos2(m.x(rect.min.x + 8.0, g.size().x), rect.center().y - g.size().y / 2.0), g, text_color); - // Shortcut column: chips, then "+". - let mut action = None; - let col = Rect::from_min_max(pos2(rect.max.x - shortcut_w - ICON - 4.0, rect.min.y), pos2(rect.max.x - ICON - 4.0, rect.max.y)); - let painter = ui.painter_at(m.rect(col)); - let mut x = col.min.x; - let rec = match self.recording_target() { - Some(Target::Cell { command, old }) if *command == r.command => Some(old.clone()), - _ => None, - }; - let chip_h = rect.height() - 4.0; - let mut cells: Vec<(Option, String, bool)> = - r.chips.iter().map(|c| (Some(c.binding.clone()), c.text.clone(), rec.as_ref() == Some(&Some(c.binding.clone())))).collect(); - if rec == Some(None) { - cells.push((None, String::new(), true)); - } - for (n, (binding, text, recording)) in cells.iter().enumerate() { - let shown = if *recording { self.recording_text() } else { text.clone() }; - let g = painter.layout_no_wrap(shown.clone(), font.clone(), text_color); - let w = g.size().x + 2.0 * CHIP_PAD; - let chip = m.rect(Rect::from_min_size(pos2(x, rect.min.y + 2.0), vec2(w, chip_h))); - x += w + CHIP_GAP; - let changed = r.chips.get(n).is_some_and(|c| c.changed); - let stroke = if *recording { - Stroke::new(1.5, self.style.accent) - } else if changed { - Stroke::new(1.0, self.style.changed) - } else { - v.widgets.inactive.bg_stroke - }; - let current_chip = selected && focused && n == self.state.chip; - painter.rect(chip, 3.0, v.widgets.inactive.weak_bg_fill, if current_chip { v.selection.stroke } else { stroke }, StrokeKind::Inside); - painter.galley(pos2(chip.min.x + CHIP_PAD, chip.center().y - g.size().y / 2.0), g, v.text_color()); - let waiting = self.state.recording.as_ref().is_some_and(|r| r.waiting.is_some()); - let label = if *recording && waiting { - fill(self.ui("{label}: {chord}"), &[("label", &r.label), ("chord", &shown)]) - } else if *recording { - fill(self.ui("{label}: press keys"), &[("label", &r.label)]) - } else { - fill(self.ui("{label}: {chord}"), &[("label", &r.label), ("chord", text)]) - }; - let resp = - ui.interact(chip.intersect(m.rect(col)), self.base.with(("cell", &r.key, n)), if editable { Sense::CLICK } else { Sense::hover() }); - resp.widget_info(|| WidgetInfo::labeled(Role::Button, editable, &label)); - self.tag(|| format!("shortcuts.cell.{}.{n}", r.key), chip, &label); - if resp.clicked() { - action = Some(RowAction::Record(r.command.clone(), binding.clone(), n)); - } - } - if cells.is_empty() { - // An empty cell to click for the first shortcut. - let cell = m.rect(Rect::from_min_max(pos2(col.min.x, rect.min.y), pos2(col.max.x - ICON - CHIP_GAP, rect.max.y))); - let label = fill(self.ui("{label}: {chord}"), &[("label", &r.label), ("chord", &self.ui("None"))]); - let resp = ui.interact(cell, self.base.with(("cell", &r.key, 0)), if editable { Sense::CLICK } else { Sense::hover() }); - resp.widget_info(|| WidgetInfo::labeled(Role::Button, editable, &label)); - self.tag(|| format!("shortcuts.cell.{}.0", r.key), cell, &label); - if resp.clicked() { - action = Some(RowAction::Record(r.command.clone(), None, 0)); - } - x = col.min.x; - } - if editable && r.chips.len() < MAX_BINDINGS && rec != Some(None) { - let plus = m.rect(Rect::from_min_size(pos2(x.min(col.max.x - ICON), rect.center().y - ICON / 2.0), Vec2::splat(ICON))); - let label = fill(self.ui("Add a shortcut to {label}"), &[("label", &r.label)]); - let resp = self.row_button(ui, plus, self.base.with(("add", &r.key)), &label, true, paint_plus); - self.tag(|| format!("shortcuts.add.{}", r.key), plus, &label); - if resp.clicked() { - action = Some(RowAction::Record(r.command.clone(), None, r.chips.len())); - } - } - if editable { - let reset = m.rect(Rect::from_min_size(pos2(rect.max.x - ICON - 2.0, rect.center().y - ICON / 2.0), Vec2::splat(ICON))); - let label = fill(self.ui("Restore the default for {label}"), &[("label", &r.label)]); - let resp = self.row_button(ui, reset, self.base.with(("reset", &r.key)), &label, r.changed, paint_reset); - self.tag(|| format!("shortcuts.reset.{}", r.key), reset, &label); - if resp.clicked() { - action = Some(RowAction::Reset(r.command.clone())); - } - } + let action = + self.shortcut_column(ui, RowShortcutLayout { row: r, rect, shortcut_w, mirror: m, editable, selected, focused, font, text_color }); let clicked = resp.clicked(); let mut tip = vec![self.vis(&r.label)]; @@ -987,6 +911,107 @@ impl Draw<'_, '_> { action.or_else(|| clicked.then(|| RowAction::Select(r.command.clone()))) } + fn shortcut_column(&self, ui: &mut Ui, layout: RowShortcutLayout<'_>) -> Option { + let RowShortcutLayout { row, rect, shortcut_w, mirror, editable, selected, focused, font, text_color } = layout; + let col = Rect::from_min_max(pos2(rect.max.x - shortcut_w - ICON - 4.0, rect.min.y), pos2(rect.max.x - ICON - 4.0, rect.max.y)); + let painter = ui.painter_at(mirror.rect(col)); + let mut x = col.min.x; + let rec = match self.recording_target() { + Some(Target::Cell { command, old }) if *command == row.command => Some(old.clone()), + _ => None, + }; + let mut cells: Vec<(Option, String, bool)> = + row.chips.iter().map(|chip| (Some(chip.binding.clone()), chip.text.clone(), rec.as_ref() == Some(&Some(chip.binding.clone())))).collect(); + if rec == Some(None) { + cells.push((None, String::new(), true)); + } + let mut action = self.draw_shortcut_chips( + ui, + ShortcutChipsLayout { painter: &painter, mirror, col, rect, row, cells: &cells, editable, selected, focused, font: &font, text_color }, + &mut x, + ); + if cells.is_empty() { + action = self.draw_empty_shortcut_cell(ui, mirror, col, rect, row, editable).or(action); + x = col.min.x; + } + if editable && row.chips.len() < MAX_BINDINGS && rec != Some(None) { + let plus = mirror.rect(Rect::from_min_size(pos2(x.min(col.max.x - ICON), rect.center().y - ICON / 2.0), Vec2::splat(ICON))); + let label = fill(self.ui("Add a shortcut to {label}"), &[("label", &row.label)]); + let resp = self.row_button(ui, plus, self.base.with(("add", &row.key)), &label, true, paint_plus); + self.tag(|| format!("shortcuts.add.{}", row.key), plus, &label); + if resp.clicked() { + action = Some(RowAction::Record(row.command.clone(), None, row.chips.len())); + } + } + if editable { + let reset = mirror.rect(Rect::from_min_size(pos2(rect.max.x - ICON - 2.0, rect.center().y - ICON / 2.0), Vec2::splat(ICON))); + let label = fill(self.ui("Restore the default for {label}"), &[("label", &row.label)]); + let resp = self.row_button(ui, reset, self.base.with(("reset", &row.key)), &label, row.changed, paint_reset); + self.tag(|| format!("shortcuts.reset.{}", row.key), reset, &label); + if resp.clicked() { + action = Some(RowAction::Reset(row.command.clone())); + } + } + action + } + + fn draw_shortcut_chips(&self, ui: &mut Ui, layout: ShortcutChipsLayout<'_>, x: &mut f32) -> Option { + let ShortcutChipsLayout { painter, mirror, col, rect, row, cells, editable, selected, focused, font, text_color } = layout; + let v = ui.visuals(); + let mut action = None; + for (n, (binding, text, recording)) in cells.iter().enumerate() { + let shown = if *recording { self.recording_text() } else { text.clone() }; + let g = painter.layout_no_wrap(shown.clone(), font.clone(), text_color); + let w = g.size().x + 2.0 * CHIP_PAD; + let chip = mirror.rect(Rect::from_min_size(pos2(*x, rect.min.y + 2.0), vec2(w, rect.height() - 4.0))); + *x += w + CHIP_GAP; + let changed = row.chips.get(n).is_some_and(|c| c.changed); + let stroke = if *recording { + Stroke::new(1.5, self.style.accent) + } else if changed { + Stroke::new(1.0, self.style.changed) + } else { + v.widgets.inactive.bg_stroke + }; + let current_chip = selected && focused && n == self.state.chip; + painter.rect(chip, 3.0, v.widgets.inactive.weak_bg_fill, if current_chip { v.selection.stroke } else { stroke }, StrokeKind::Inside); + painter.galley(pos2(chip.min.x + CHIP_PAD, chip.center().y - g.size().y / 2.0), g, v.text_color()); + let waiting = self.state.recording.as_ref().is_some_and(|r| r.waiting.is_some()); + let label = self.shortcut_accessibility_label(row, text, &shown, *recording, waiting); + let resp = ui.interact( + chip.intersect(mirror.rect(col)), + self.base.with(("cell", &row.key, n)), + if editable { Sense::CLICK } else { Sense::hover() }, + ); + resp.widget_info(|| WidgetInfo::labeled(Role::Button, editable, &label)); + self.tag(|| format!("shortcuts.cell.{}.{n}", row.key), chip, &label); + if resp.clicked() { + action = Some(RowAction::Record(row.command.clone(), binding.clone(), n)); + } + } + action + } + + fn shortcut_accessibility_label(&self, row: &Row, text: &str, shown: &str, recording: bool, waiting: bool) -> String { + if recording && waiting { + fill(self.ui("{label}: {chord}"), &[("label", &row.label), ("chord", shown)]) + } else if recording { + fill(self.ui("{label}: press keys"), &[("label", &row.label)]) + } else { + fill(self.ui("{label}: {chord}"), &[("label", &row.label), ("chord", text)]) + } + } + + fn draw_empty_shortcut_cell(&self, ui: &mut Ui, mirror: Mirror, col: Rect, rect: Rect, row: &Row, editable: bool) -> Option { + let cell = mirror.rect(Rect::from_min_max(pos2(col.min.x, rect.min.y), pos2(col.max.x - ICON - CHIP_GAP, rect.max.y))); + let none = self.ui("None"); + let label = fill(self.ui("{label}: {chord}"), &[("label", &row.label), ("chord", &none)]); + let resp = ui.interact(cell, self.base.with(("cell", &row.key, 0)), if editable { Sense::CLICK } else { Sense::hover() }); + resp.widget_info(|| WidgetInfo::labeled(Role::Button, editable, &label)); + self.tag(|| format!("shortcuts.cell.{}.0", row.key), cell, &label); + resp.clicked().then(|| RowAction::Record(row.command.clone(), None, 0)) + } + /// A read-only row from `Options::fixed`, struck through when a command took its chord. fn fixed_row(&self, ui: &mut Ui, rect: Rect, f: &FixedLine, shortcut_w: f32) { let v = ui.visuals(); @@ -1457,6 +1482,32 @@ struct Mirror { sum: f32, } +struct RowShortcutLayout<'a> { + row: &'a Row, + rect: Rect, + shortcut_w: f32, + mirror: Mirror, + editable: bool, + selected: bool, + focused: bool, + font: egui::FontId, + text_color: Color32, +} + +struct ShortcutChipsLayout<'a> { + painter: &'a Painter, + mirror: Mirror, + col: Rect, + rect: Rect, + row: &'a Row, + cells: &'a [(Option, String, bool)], + editable: bool, + selected: bool, + focused: bool, + font: &'a egui::FontId, + text_color: Color32, +} + impl Mirror { fn rect(self, r: Rect) -> Rect { if self.on { Rect::from_x_y_ranges((self.sum - r.max.x)..=(self.sum - r.min.x), r.y_range()) } else { r } diff --git a/crates/media/src/isobmff/codec.rs b/crates/media/src/isobmff/codec.rs index a0f63962d..b98fec6ca 100644 --- a/crates/media/src/isobmff/codec.rs +++ b/crates/media/src/isobmff/codec.rs @@ -1379,95 +1379,7 @@ impl SampleEntry { return Ok(()); } if let Some(v) = &self.video { - let m = b.start(&self.format.0); - b.zeros(6); - b.u16(self.data_reference_index); - b.u16(0); - b.u16(0); - b.u32(v.vendor.as_u32()); - b.u32(v.temporal_quality); - b.u32(v.spatial_quality); - b.u16(v.width); - b.u16(v.height); - b.u32(v.horiz_resolution); - b.u32(v.vert_resolution); - b.u32(0); - b.u16(v.frame_count); - let name = v.compressor_name.as_bytes(); - let n = name.len().min(31); - let mut nb = [0u8; 32]; - nb[0] = n as u8; - nb[1..1 + n].copy_from_slice(&name[..n]); - b.bytes(&nb); - b.u16(v.depth); - b.i16(-1); - match &self.codec { - CodecConfig::Avc(c) => b.leaf(b"avcC", &c.to_bytes()), - CodecConfig::Hevc(c) => b.leaf(b"hvcC", &c.to_bytes()), - CodecConfig::Av1(c) => b.leaf(b"av1C", &c.to_bytes()), - CodecConfig::Vp9(c) => b.leaf(b"vpcC", &c.to_bytes()), - CodecConfig::Apv(c) => b.leaf(b"apvC", &c.to_bytes()), - _ => {} - } - if let Some(g) = v.gamma { - b.leaf(b"gama", &g.to_be_bytes()); - } - if let Some(fi) = v.field_info { - b.leaf(b"fiel", &[fi.fields, fi.detail]); - } - if let Some(ci) = &v.color { - let m = b.start(b"colr"); - match ci { - ColorInfo::Nclx { primaries, transfer, matrix, full_range } => { - b.bytes(b"nclx"); - b.u16(*primaries); - b.u16(*transfer); - b.u16(*matrix); - b.u8(if *full_range { 0x80 } else { 0 }); - } - ColorInfo::Nclc { primaries, transfer, matrix } => { - b.bytes(b"nclc"); - b.u16(*primaries); - b.u16(*transfer); - b.u16(*matrix); - } - ColorInfo::Icc { kind, profile } => { - b.bytes(&kind.0); - b.bytes(profile); - } - } - b.end(m); - } - if let Some(md) = &v.mastering_display { - b.leaf(b"mdcv", &md.to_bytes()); - } - if let Some((cll, fall)) = v.content_light { - let mut d = cll.to_be_bytes().to_vec(); - d.extend_from_slice(&fall.to_be_bytes()); - b.leaf(b"clli", &d); - } - if let Some((h, vv)) = v.pixel_aspect { - let m = b.start(b"pasp"); - b.u32(h); - b.u32(vv); - b.end(m); - } - if let Some(c) = v.clean_aperture { - let m = b.start(b"clap"); - b.u32(c.width.0); - b.u32(c.width.1); - b.u32(c.height.0); - b.u32(c.height.1); - b.i32(c.horiz_offset.0); - b.u32(c.horiz_offset.1); - b.i32(c.vert_offset.0); - b.u32(c.vert_offset.1); - b.end(m); - } - if let Some(br) = self.bitrate { - write_btrt(b, br); - } - b.end(m); + self.write_video(b, v); return Ok(()); } let a = self.audio.clone().ok_or_else(|| Error::Mux("sample entry has neither video nor audio params".into()))?; @@ -1581,6 +1493,118 @@ impl SampleEntry { b.end(m); Ok(()) } + + fn write_video(&self, b: &mut BoxBuf, v: &VideoParams) { + let m = b.start(&self.format.0); + b.zeros(6); + b.u16(self.data_reference_index); + b.u16(0); + b.u16(0); + b.u32(v.vendor.as_u32()); + b.u32(v.temporal_quality); + b.u32(v.spatial_quality); + b.u16(v.width); + b.u16(v.height); + b.u32(v.horiz_resolution); + b.u32(v.vert_resolution); + b.u32(0); + b.u16(v.frame_count); + let name = v.compressor_name.as_bytes(); + let n = name.len().min(31); + let mut nb = [0u8; 32]; + nb[0] = n as u8; + nb[1..1 + n].copy_from_slice(&name[..n]); + b.bytes(&nb); + b.u16(v.depth); + b.i16(-1); + write_video_codec_config(b, &self.codec); + write_video_descriptive_boxes(b, v); + if let Some(br) = self.bitrate { + write_btrt(b, br); + } + b.end(m); + } +} + +fn write_video_codec_config(b: &mut BoxBuf, codec: &CodecConfig) { + match codec { + CodecConfig::Avc(config) => b.leaf(b"avcC", &config.to_bytes()), + CodecConfig::Hevc(config) => b.leaf(b"hvcC", &config.to_bytes()), + CodecConfig::Av1(config) => b.leaf(b"av1C", &config.to_bytes()), + CodecConfig::Vp9(config) => b.leaf(b"vpcC", &config.to_bytes()), + CodecConfig::Apv(config) => b.leaf(b"apvC", &config.to_bytes()), + _ => {} + } +} + +fn write_video_descriptive_boxes(b: &mut BoxBuf, v: &VideoParams) { + if let Some(gamma) = v.gamma { + b.leaf(b"gama", &gamma.to_be_bytes()); + } + if let Some(field_info) = v.field_info { + b.leaf(b"fiel", &[field_info.fields, field_info.detail]); + } + if let Some(color) = &v.color { + write_color_box(b, color); + } + if let Some(display) = &v.mastering_display { + b.leaf(b"mdcv", &display.to_bytes()); + } + if let Some((cll, fall)) = v.content_light { + let mut data = cll.to_be_bytes().to_vec(); + data.extend_from_slice(&fall.to_be_bytes()); + b.leaf(b"clli", &data); + } + write_pixel_aspect_box(b, v.pixel_aspect); + write_clean_aperture_box(b, v.clean_aperture); +} + +fn write_color_box(b: &mut BoxBuf, color: &ColorInfo) { + let m = b.start(b"colr"); + match color { + ColorInfo::Nclx { primaries, transfer, matrix, full_range } => { + b.bytes(b"nclx"); + b.u16(*primaries); + b.u16(*transfer); + b.u16(*matrix); + b.u8(if *full_range { 0x80 } else { 0 }); + } + ColorInfo::Nclc { primaries, transfer, matrix } => { + b.bytes(b"nclc"); + b.u16(*primaries); + b.u16(*transfer); + b.u16(*matrix); + } + ColorInfo::Icc { kind, profile } => { + b.bytes(&kind.0); + b.bytes(profile); + } + } + b.end(m); +} + +fn write_pixel_aspect_box(b: &mut BoxBuf, pixel_aspect: Option<(u32, u32)>) { + if let Some((horizontal, vertical)) = pixel_aspect { + let m = b.start(b"pasp"); + b.u32(horizontal); + b.u32(vertical); + b.end(m); + } +} + +fn write_clean_aperture_box(b: &mut BoxBuf, clean_aperture: Option) { + if let Some(aperture) = clean_aperture { + let m = b.start(b"clap"); + b.u32(aperture.width.0); + b.u32(aperture.width.1); + b.u32(aperture.height.0); + b.u32(aperture.height.1); + b.i32(aperture.horiz_offset.0); + b.u32(aperture.horiz_offset.1); + b.i32(aperture.vert_offset.0); + b.u32(aperture.vert_offset.1); + b.end(m); + } } fn write_btrt(b: &mut BoxBuf, br: BitRate) { diff --git a/crates/media/src/isobmff/demux.rs b/crates/media/src/isobmff/demux.rs index f3c0d78f7..8f7c413d6 100644 --- a/crates/media/src/isobmff/demux.rs +++ b/crates/media/src/isobmff/demux.rs @@ -604,7 +604,19 @@ impl SttsCursor<'_> { } } -fn parse_stbl(stbl: &[u8], t: &mut Track, is_qt: bool, file_len: u64, budget: &mut SampleBudget) -> Result<()> { +struct StblTables { + stts: Vec<(u32, u32)>, + ctts: Vec<(u32, u32)>, + stss: Option>, + stsc: Vec<(u32, u32, u32)>, + const_size: u32, + sample_count: usize, + sizes: Vec, + chunks: Vec, + sample_groups: Vec, +} + +fn parse_stbl_tables(stbl: &[u8], t: &mut Track, is_qt: bool, file_len: u64) -> Result { let mut stts: Vec<(u32, u32)> = Vec::new(); let mut ctts: Vec<(u32, u32)> = Vec::new(); let mut stss: Option> = None; @@ -730,8 +742,15 @@ fn parse_stbl(stbl: &[u8], t: &mut Track, is_qt: bool, file_len: u64, budget: &m _ => {} } } - t.sample_groups = + let sample_groups = sbgp.into_iter().map(|(gt, runs)| SampleGroup { grouping_type: gt, runs, descriptions: sgpd.remove(>).unwrap_or_default() }).collect(); + Ok(StblTables { stts, ctts, stss, stsc, const_size, sample_count, sizes, chunks, sample_groups }) +} + +fn parse_stbl(stbl: &[u8], t: &mut Track, is_qt: bool, file_len: u64, budget: &mut SampleBudget) -> Result<()> { + let StblTables { stts, ctts, stss, stsc, const_size, mut sample_count, sizes, chunks, sample_groups } = + parse_stbl_tables(stbl, t, is_qt, file_len)?; + t.sample_groups = sample_groups; if sample_count == 0 || chunks.is_empty() || stsc.is_empty() { return Ok(()); } diff --git a/crates/media/src/isobmff/mux.rs b/crates/media/src/isobmff/mux.rs index 8f50d1f14..836aaaed7 100644 --- a/crates/media/src/isobmff/mux.rs +++ b/crates/media/src/isobmff/mux.rs @@ -422,6 +422,14 @@ fn write_trak(b: &mut BoxBuf, t: &WTrack, movie_ts: u32, qt: bool, shift: u64, f b.end(u); b.end(r); b.end(d); + write_stbl(b, t, qt, shift, force_co64)?; + b.end(minf); + b.end(mdia); + b.end(trak); + Ok(()) +} + +fn write_stbl(b: &mut BoxBuf, t: &WTrack, qt: bool, shift: u64, force_co64: bool) -> Result<()> { let stbl = b.start(b"stbl"); let m = b.start_full(b"stsd", 0, 0); b.u32(1); @@ -503,9 +511,6 @@ fn write_trak(b: &mut BoxBuf, t: &WTrack, movie_ts: u32, qt: bool, shift: u64, f } b.end(m); b.end(stbl); - b.end(minf); - b.end(mdia); - b.end(trak); Ok(()) } diff --git a/crates/media/src/matroska/demux.rs b/crates/media/src/matroska/demux.rs index 15ca9dbd9..253f613c5 100644 --- a/crates/media/src/matroska/demux.rs +++ b/crates/media/src/matroska/demux.rs @@ -4,8 +4,16 @@ use std::collections::{HashMap, VecDeque}; use crate::matroska::ebml::{self, Children, Lacing, Size, parse_lacing, read_id, read_size}; use crate::matroska::error::{Error, Result}; -use crate::matroska::ids::*; -use crate::matroska::meta::*; +use crate::matroska::ids::{ + ATTACHED_FILE, ATTACHMENTS, BLOCK, BLOCK_ADD_ID, BLOCK_ADDITIONAL, BLOCK_ADDITIONS, BLOCK_DURATION, BLOCK_GROUP, BLOCK_MORE, CHAPTERS, CLUSTER, + CRC32, CUES, DISCARD_PADDING, DOC_TYPE, DOC_TYPE_READ_VERSION, DOC_TYPE_VERSION, EBML, EBML_MAX_ID_LENGTH, EBML_MAX_SIZE_LENGTH, FILE_DATA, + FILE_DESCRIPTION, FILE_MEDIA_TYPE, FILE_NAME, FILE_UID, INFO, REFERENCE_BLOCK, SEEK_HEAD, SEGMENT, SIMPLE_BLOCK, TAGS, TIMESTAMP, TRACK_ENTRY, + TRACKS, VOID, is_top_level, +}; +use crate::matroska::meta::{ + Attachment, ClusterInfo, CuePoint, Edition, SeekEntry, SegmentInfo, Tag, parse_chapters, parse_cues, parse_info, parse_seekhead, parse_tags, + parse_tracks, +}; use crate::matroska::source::{ByteSource, ReadSeekSource}; use crate::matroska::track::{Sample, Track, TrackKind, gcd}; diff --git a/crates/media/src/matroska/meta.rs b/crates/media/src/matroska/meta.rs index 8cf802488..d0356a6be 100644 --- a/crates/media/src/matroska/meta.rs +++ b/crates/media/src/matroska/meta.rs @@ -3,7 +3,22 @@ use crate::matroska::codec::map_codec; use crate::matroska::ebml::{Children, float, int, string, uint}; -use crate::matroska::ids::*; +use crate::matroska::ids::{ + ALPHA_MODE, AUDIO, BIT_DEPTH, BITS_PER_CHANNEL, CB_SUBSAMPLING_HORZ, CB_SUBSAMPLING_VERT, CHANNELS, CHAP_COUNTRY, CHAP_LANGUAGE, + CHAP_LANGUAGE_BCP47, CHAP_STRING, CHAPTER_ATOM, CHAPTER_DISPLAY, CHAPTER_FLAG_ENABLED, CHAPTER_FLAG_HIDDEN, CHAPTER_STRING_UID, CHAPTER_TIME_END, + CHAPTER_TIME_START, CHAPTER_UID, CHROMA_SITING_HORZ, CHROMA_SITING_VERT, CHROMA_SUBSAMPLING_HORZ, CHROMA_SUBSAMPLING_VERT, CODEC_DELAY, CODEC_ID, + CODEC_NAME, CODEC_PRIVATE, COLOUR, COLOUR_SPACE, CONTENT_COMP_ALGO, CONTENT_COMP_SETTINGS, CONTENT_COMPRESSION, CONTENT_ENCODING, + CONTENT_ENCODING_ORDER, CONTENT_ENCODING_SCOPE, CONTENT_ENCODING_TYPE, CONTENT_ENCODINGS, CUE_BLOCK_NUMBER, CUE_CLUSTER_POSITION, CUE_DURATION, + CUE_POINT, CUE_RELATIVE_POSITION, CUE_TIME, CUE_TRACK, CUE_TRACK_POSITIONS, DATE_UTC, DEFAULT_DURATION, DISPLAY_HEIGHT, DISPLAY_UNIT, + DISPLAY_WIDTH, DURATION, EDITION_ENTRY, EDITION_FLAG_DEFAULT, EDITION_FLAG_HIDDEN, EDITION_FLAG_ORDERED, EDITION_UID, FIELD_ORDER, FLAG_DEFAULT, + FLAG_ENABLED, FLAG_FORCED, FLAG_HEARING_IMPAIRED, FLAG_INTERLACED, FLAG_LACING, LANGUAGE, LANGUAGE_BCP47, LUMINANCE_MAX, LUMINANCE_MIN, + MASTERING_METADATA, MATRIX_COEFFICIENTS, MAX_CLL, MAX_FALL, MUXING_APP, NAME, OUTPUT_SAMPLING_FREQUENCY, PIXEL_CROP_BOTTOM, PIXEL_CROP_LEFT, + PIXEL_CROP_RIGHT, PIXEL_CROP_TOP, PIXEL_HEIGHT, PIXEL_WIDTH, PRIMARIES, PRIMARY_B_X, PRIMARY_B_Y, PRIMARY_G_X, PRIMARY_G_Y, PRIMARY_R_X, + PRIMARY_R_Y, PROJECTION, PROJECTION_POSE_PITCH, PROJECTION_POSE_ROLL, PROJECTION_POSE_YAW, PROJECTION_TYPE, RANGE, SAMPLING_FREQUENCY, SEEK, + SEEK_ID, SEEK_POSITION, SEEK_PRE_ROLL, SEGMENT_UUID, SIMPLE_TAG, STEREO_MODE, TAG, TAG_ATTACHMENT_UID, TAG_BINARY, TAG_CHAPTER_UID, TAG_DEFAULT, + TAG_EDITION_UID, TAG_LANGUAGE, TAG_NAME, TAG_STRING, TAG_TRACK_UID, TARGET_TYPE, TARGET_TYPE_VALUE, TARGETS, TIMESTAMP_SCALE, TITLE, TRACK_ENTRY, + TRACK_NUMBER, TRACK_TYPE, TRACK_UID, TRANSFER_CHARACTERISTICS, VIDEO, WHITE_POINT_X, WHITE_POINT_Y, WRITING_APP, +}; use crate::matroska::track::{AudioInfo, Color, ContentEncoding, MasteringMetadata, Projection, Track, TrackKind, VideoInfo, gcd}; // A malformed Matroska file may contain arbitrarily deep ChapterAtom and SimpleTag chains. diff --git a/crates/media/src/matroska/mux.rs b/crates/media/src/matroska/mux.rs index f481ceace..7d26a8f1e 100644 --- a/crates/media/src/matroska/mux.rs +++ b/crates/media/src/matroska/mux.rs @@ -6,7 +6,13 @@ use std::io::{Seek, SeekFrom, Write}; use crate::matroska::ebml::{write_id, write_size, write_unknown_size}; use crate::matroska::error::{Error, Result}; -use crate::matroska::ids::*; +use crate::matroska::ids::{ + AUDIO, BIT_DEPTH, BLOCK, BLOCK_DURATION, BLOCK_GROUP, CHANNELS, CLUSTER, CODEC_DELAY, CODEC_ID, CODEC_PRIVATE, CUE_CLUSTER_POSITION, CUE_POINT, + CUE_TIME, CUE_TRACK, CUE_TRACK_POSITIONS, CUES, DEFAULT_DURATION, DOC_TYPE, DOC_TYPE_READ_VERSION, DOC_TYPE_VERSION, DURATION, EBML, + EBML_MAX_ID_LENGTH, EBML_MAX_SIZE_LENGTH, EBML_READ_VERSION, EBML_VERSION, FLAG_LACING, INFO, LANGUAGE, MUXING_APP, NAME, PIXEL_HEIGHT, + PIXEL_WIDTH, REFERENCE_BLOCK, SAMPLING_FREQUENCY, SEEK, SEEK_HEAD, SEEK_ID, SEEK_POSITION, SEEK_PRE_ROLL, SEGMENT, SIMPLE_BLOCK, TIMESTAMP, + TIMESTAMP_SCALE, TITLE, TRACK_ENTRY, TRACK_NUMBER, TRACK_TYPE, TRACK_UID, TRACKS, VIDEO, VOID, WRITING_APP, +}; use crate::matroska::track::TrackKind; /// Lacing used for audio tracks. Only frames of a track with a DefaultDuration lace, and a lace diff --git a/crates/media/src/matroska/synthetic_tests.rs b/crates/media/src/matroska/synthetic_tests.rs index 3ec0674f6..0fbe5faec 100644 --- a/crates/media/src/matroska/synthetic_tests.rs +++ b/crates/media/src/matroska/synthetic_tests.rs @@ -8,7 +8,7 @@ use crate::matroska::error::Error; use crate::matroska::ids::*; use crate::matroska::mux::{el, el_float, el_str, el_uint}; use crate::matroska::track::ContentEncoding; -use crate::matroska::{ByteSource, Demuxer, OpenOptions, Packet, open_with}; +use crate::matroska::{ByteSource, Demuxer, MkvFile, OpenOptions, Packet, open_with}; fn block(track: u8, rel: i16, flags: u8, data: &[u8]) -> Vec { let mut b = vec![0x80 | track]; @@ -149,71 +149,81 @@ fn synthetic() -> Vec { f } -#[test] -fn synthetic_file() { - let bytes = synthetic(); - let f = open_with(&bytes[..], &OpenOptions { index: true, verify_crc: true }).unwrap(); +fn assert_synthetic_metadata(f: &MkvFile, bytes: &[u8]) { assert!(f.live); assert!(f.warnings.is_empty(), "{:?}", f.warnings); assert_eq!((f.crc_checked, f.crc_errors.len()), (1, 0)); assert_eq!(f.info.title.as_deref(), Some("Synthetic")); - let v = &f.tracks[0]; - assert!(matches!(&v.codec, Codec::Avc { avcc } if avcc[1] == 0x64)); - let vi = v.video.as_ref().unwrap(); - assert_eq!(vi.pixel_aspect(), (4, 3)); - assert_eq!(vi.projection.as_ref().map(|p| p.roll), Some(-90.0)); - assert_eq!(vi.display_rotation(), Some(1)); - let c = vi.color.as_ref().unwrap(); - assert!(c.full_range()); + let video = &f.tracks[0]; + assert!(matches!(&video.codec, Codec::Avc { avcc } if avcc[1] == 0x64)); + let info = video.video.as_ref().unwrap(); + assert_eq!(info.pixel_aspect(), (4, 3)); + assert_eq!(info.projection.as_ref().map(|projection| projection.roll), Some(-90.0)); + assert_eq!(info.display_rotation(), Some(1)); + let color = info.color.as_ref().unwrap(); + assert!(color.full_range()); assert_eq!( - (c.matrix_coefficients, c.transfer_characteristics, c.primaries, c.max_cll, c.max_fall), + (color.matrix_coefficients, color.transfer_characteristics, color.primaries, color.max_cll, color.max_fall), (Some(9), Some(16), Some(9), Some(1000), Some(400)) ); - let m = c.mastering.as_ref().unwrap(); - assert_eq!(m.primaries[1], (0.17, 0.797)); - assert_eq!(m.white_point, (0.3127, 0.329)); - assert_eq!((m.luminance_max, m.luminance_min), (1000.0, 0.0001)); - assert_eq!(v.encodings, vec![ContentEncoding::HeaderStripping { bytes: vec![0, 0, 0, 1], scope: 1 }]); - // legacy AAC ID: ASC synthesized with explicit SBR signaling (LC, 24 kHz, stereo → 48 kHz) + let mastering = color.mastering.as_ref().unwrap(); + assert_eq!(mastering.primaries[1], (0.17, 0.797)); + assert_eq!(mastering.white_point, (0.3127, 0.329)); + assert_eq!((mastering.luminance_max, mastering.luminance_min), (1000.0, 0.0001)); + assert_eq!(video.encodings, vec![ContentEncoding::HeaderStripping { bytes: vec![0, 0, 0, 1], scope: 1 }]); + // Legacy AAC ID synthesizes explicit SBR signaling (LC, 24 kHz, stereo to 48 kHz). assert_eq!(f.tracks[1].codec, Codec::Aac { asc: vec![0x13, 0x10, 0x56, 0xE5, 0x98] }); assert_eq!(f.tracks[1].language, "ger"); assert_eq!(f.chapters[0].chapters[0].title(), Some("Intro")); assert_eq!(f.chapters[0].chapters[0].end_ns, Some(40_000_000)); assert_eq!(f.attachments.len(), 1); - let at = &f.attachments[0]; - assert_eq!((at.name.as_str(), at.media_type.as_str(), at.uid), ("font.ttf", "font/ttf", 77)); - assert_eq!(&bytes[at.data_offset as usize..][..100], &[9; 100]); + let attachment = &f.attachments[0]; + assert_eq!((attachment.name.as_str(), attachment.media_type.as_str(), attachment.uid), ("font.ttf", "font/ttf", 77)); + assert_eq!(&bytes[attachment.data_offset as usize..][..100], &[9; 100]); assert_eq!(f.tags[0].targets.track_uids, vec![11]); assert_eq!(f.tags[0].simple[0].string.as_deref(), Some("Nobody")); assert_eq!(f.clusters.len(), 2); - assert_eq!(v.samples.len(), 3); - assert_eq!(v.keyframes(), &[0, 2]); + assert_eq!(video.samples.len(), 3); + assert_eq!(video.keyframes(), &[0, 2]); +} - let d = Demuxer::from_slice(&bytes).unwrap(); - let p: Vec = d.map(|p| p.unwrap()).collect(); - assert_eq!(p.len(), 4); - assert_eq!((p[0].track, p[0].pts, p[0].keyframe, p[0].duration), (0, 1000, true, 40)); - assert_eq!(p[0].data, [0, 0, 0, 1, 0x65, 1, 2]); - assert_eq!((p[1].track, p[1].pts, p[1].data.len()), (1, 1005, 10)); - assert_eq!((p[2].pts, p[2].keyframe, p[2].duration), (1040, false, 80)); - assert_eq!(p[2].data, [0, 0, 0, 1, 0x41, 3]); - assert_eq!((p[3].pts, p[3].pts_ns), (1120, 1_120_000_000)); - assert_eq!(f.read_sample(&bytes[..], 0, 1).unwrap(), p[2].data); +fn assert_synthetic_packets(f: &MkvFile, bytes: &[u8]) { + let packets: Vec = Demuxer::from_slice(bytes).unwrap().map(|packet| packet.unwrap()).collect(); + assert_eq!(packets.len(), 4); + assert_eq!((packets[0].track, packets[0].pts, packets[0].keyframe, packets[0].duration), (0, 1000, true, 40)); + assert_eq!(packets[0].data, [0, 0, 0, 1, 0x65, 1, 2]); + assert_eq!((packets[1].track, packets[1].pts, packets[1].data.len()), (1, 1005, 10)); + assert_eq!((packets[2].pts, packets[2].keyframe, packets[2].duration), (1040, false, 80)); + assert_eq!(packets[2].data, [0, 0, 0, 1, 0x41, 3]); + assert_eq!((packets[3].pts, packets[3].pts_ns), (1120, 1_120_000_000)); + assert_eq!(f.read_sample(bytes, 0, 1).unwrap(), packets[2].data); +} - // CRC mismatch is reported, not fatal - let mut bad = bytes.clone(); - let title = bad.windows(9).position(|w| w == b"Synthetic").unwrap(); - bad[title] = b's'; - let f = open_with(&bad[..], &OpenOptions { index: true, verify_crc: true }).unwrap(); - assert_eq!(f.crc_errors.len(), 1); +fn assert_crc_mismatch_is_reported(bytes: &[u8]) { + let mut corrupted = bytes.to_vec(); + let title = corrupted.windows(9).position(|window| window == b"Synthetic").unwrap(); + corrupted[title] = b's'; + let file = open_with(&corrupted[..], &OpenOptions { index: true, verify_crc: true }).unwrap(); + assert_eq!(file.crc_errors.len(), 1); +} - // seeking without index or cues builds the index - let mut d = Demuxer::with_options(&bytes[..], OpenOptions { index: false, verify_crc: false }).unwrap(); - assert_eq!(d.file().tracks.len(), 2); - assert_eq!(d.seek(0, 1_100_000_000).unwrap().pts, 1000); - assert_eq!(d.seek(0, 1_130_000_000).unwrap().pts, 1120); - assert_eq!(d.next_packet().unwrap().unwrap().pts, 1120); - assert!(d.file().indexed); +fn assert_seek_builds_an_index(bytes: &[u8]) { + let mut demuxer = Demuxer::with_options(bytes, OpenOptions { index: false, verify_crc: false }).unwrap(); + assert_eq!(demuxer.file().tracks.len(), 2); + assert_eq!(demuxer.seek(0, 1_100_000_000).unwrap().pts, 1000); + assert_eq!(demuxer.seek(0, 1_130_000_000).unwrap().pts, 1120); + assert_eq!(demuxer.next_packet().unwrap().unwrap().pts, 1120); + assert!(demuxer.file().indexed); +} + +#[test] +fn synthetic_file() { + let bytes = synthetic(); + let file = open_with(&bytes[..], &OpenOptions { index: true, verify_crc: true }).unwrap(); + assert_synthetic_metadata(&file, &bytes); + assert_synthetic_packets(&file, &bytes); + assert_crc_mismatch_is_reported(&bytes); + assert_seek_builds_an_index(&bytes); } /// One video track and one cluster at timestamp `ts` holding `content`. @@ -317,7 +327,7 @@ fn cue_position_past_the_file_falls_back_to_the_index() { let mut el_cues = Vec::new(); el(&mut el_cues, CUES, &cues); bytes.splice(at..at, el_cues); - let mut d = Demuxer::with_options(&bytes[..], OpenOptions { index: false, verify_crc: false }).unwrap(); + let mut d = Demuxer::with_options(bytes, OpenOptions { index: false, verify_crc: false }).unwrap(); assert_eq!(d.file().cues.len(), 1); assert_eq!(d.seek(0, 0).unwrap().pts, 1000); } diff --git a/crates/media/tests/oracle_matroska.rs b/crates/media/tests/oracle_matroska.rs index d46198034..c7476aaaf 100644 --- a/crates/media/tests/oracle_matroska.rs +++ b/crates/media/tests/oracle_matroska.rs @@ -10,90 +10,116 @@ use mkv::*; use serde_json::Value; use std::path::Path; -/// Compare every packet (file order) and stream parameter against ffprobe. -fn compare(ffprobe: &Path, path: &Path) -> (MkvFile, Vec) { - let json = ffprobe_json(ffprobe, path); - let bytes = std::fs::read(path).unwrap(); - let mut d = Demuxer::from_slice(&bytes).unwrap(); - let name = path.file_name().unwrap().to_string_lossy().to_string(); - let file = d.file().clone(); +fn assert_streams_match(json: &Value, file: &MkvFile, name: &str) { let streams = json["streams"].as_array().unwrap(); assert_eq!(streams.len(), file.tracks.len(), "{name}: stream count"); - for (i, st) in streams.iter().enumerate() { - let t = &file.tracks[i]; - assert_eq!(st["codec_name"].as_str().unwrap_or(""), t.codec.name(), "{name}: track {i} codec ({})", t.codec_id); - assert_eq!(st["time_base"].as_str(), Some(format!("{}/{}", t.timebase.0, t.timebase.1).as_str()), "{name}: time_base"); - match t.kind { + for (index, stream) in streams.iter().enumerate() { + let track = &file.tracks[index]; + assert_eq!(stream["codec_name"].as_str().unwrap_or(""), track.codec.name(), "{name}: track {index} codec ({})", track.codec_id); + assert_eq!(stream["time_base"].as_str(), Some(format!("{}/{}", track.timebase.0, track.timebase.1).as_str()), "{name}: time_base"); + match track.kind { TrackKind::Video => { - let v = t.video.as_ref().unwrap(); - assert_eq!(int(&st["width"]), Some(v.pixel_width as i64), "{name}: width"); - assert_eq!(int(&st["height"]), Some(v.pixel_height as i64), "{name}: height"); + let video = track.video.as_ref().unwrap(); + assert_eq!(int(&stream["width"]), Some(video.pixel_width as i64), "{name}: width"); + assert_eq!(int(&stream["height"]), Some(video.pixel_height as i64), "{name}: height"); } TrackKind::Audio => { - let a = t.audio.as_ref().unwrap(); - assert_eq!(int(&st["sample_rate"]), Some(a.sampling_frequency as i64), "{name}: sample_rate"); - assert_eq!(int(&st["channels"]), Some(a.channels as i64), "{name}: channels"); + let audio = track.audio.as_ref().unwrap(); + assert_eq!(int(&stream["sample_rate"]), Some(audio.sampling_frequency as i64), "{name}: sample_rate"); + assert_eq!(int(&stream["channels"]), Some(audio.channels as i64), "{name}: channels"); } _ => {} } } - let packets: Vec = d.by_ref().collect::>().unwrap(); - let ff = json["packets"].as_array().unwrap(); - assert_eq!(packets.len(), ff.len(), "{name}: packet count"); - let (mut pts_checked, mut dur_checked) = (0, 0); - for (i, (p, f)) in packets.iter().zip(ff).enumerate() { - let ctx = format!("{name}: packet {i} (track {})", p.track); - assert_eq!(int(&f["stream_index"]), Some(p.track as i64), "{ctx}: stream"); - if file.tracks[p.track].codec_id.starts_with("D_WEBVTT") { - // WebM WebVTT blocks carry "id\nsettings\npayload"; FFmpeg moves id/settings to side data - assert!(p.data.len() as i64 >= int(&f["size"]).unwrap(), "{ctx}: size"); +} + +fn assert_packets_match(json: &Value, file: &MkvFile, packets: &[Packet], name: &str) { + let ffprobe_packets = json["packets"].as_array().unwrap(); + assert_eq!(packets.len(), ffprobe_packets.len(), "{name}: packet count"); + let (mut pts_checked, mut duration_checked) = (0, 0); + for (index, (packet, ffprobe_packet)) in packets.iter().zip(ffprobe_packets).enumerate() { + let context = format!("{name}: packet {index} (track {})", packet.track); + assert_eq!(int(&ffprobe_packet["stream_index"]), Some(packet.track as i64), "{context}: stream"); + if file.tracks[packet.track].codec_id.starts_with("D_WEBVTT") { + // WebM WebVTT blocks carry "id\nsettings\npayload"; FFmpeg moves id/settings to side data. + assert!(packet.data.len() as i64 >= int(&ffprobe_packet["size"]).unwrap(), "{context}: size"); } else { - assert_eq!(int(&f["size"]), Some(p.data.len() as i64), "{ctx}: size"); - } - let key = f["flags"].as_str().unwrap_or("").starts_with('K'); - assert_eq!(key, p.keyframe, "{ctx}: keyframe"); - if let Some(pts) = int(&f["pts"]) - && !(p.lace > 0 && p.duration == 0) - { - // FFmpeg truncates the per-lace duration to whole ticks; we place laces exactly - // (rounded once), so later laces may differ by up to one tick per lace. - assert!((pts - p.pts).abs() <= p.lace as i64, "{ctx}: pts ffprobe {pts} vs {}", p.pts); - pts_checked += 1; - } - // The last audio or video frame of a track cut by `-t` is shorter than a codec frame. - // ffmpeg 8 writes a shortened BlockDuration there and reports the codec frame duration; a - // remux by our writer stores DefaultDuration and ffprobe reports the shortened one. So - // that frame's duration is not compared. A subtitle's duration is its cue's, and is. - let last_of_track = packets[i + 1..].iter().all(|q| q.track != p.track); - if let Some(dur) = int(&f["duration"]) - && p.duration != 0 - && (!last_of_track || file.tracks[p.track].kind == TrackKind::Subtitle) - { - assert!((dur - p.duration as i64).abs() <= 1, "{ctx}: duration ffprobe {dur} vs {}", p.duration); - dur_checked += 1; + assert_eq!(int(&ffprobe_packet["size"]), Some(packet.data.len() as i64), "{context}: size"); } + assert_eq!(ffprobe_packet["flags"].as_str().unwrap_or("").starts_with('K'), packet.keyframe, "{context}: keyframe"); + compare_packet_pts(ffprobe_packet, packet, &context, &mut pts_checked); + compare_packet_duration(ffprobe_packet, packet, packets, file, &context, index, &mut duration_checked); } assert!(pts_checked > 0, "{name}: no pts compared"); - eprintln!("{name}: {} packets, {pts_checked} pts and {dur_checked} durations compared", packets.len()); - // the sample index agrees with the packet stream - for (ti, t) in file.tracks.iter().enumerate() { - let pk: Vec<&Packet> = packets.iter().filter(|p| p.track == ti).collect(); - assert_eq!(t.samples.len(), pk.len(), "{name}: index size track {ti}"); - for (i, p) in pk.iter().enumerate() { - assert_eq!(t.samples[i].pts, p.pts); - assert_eq!(t.samples[i].keyframe, p.keyframe); - assert_eq!(t.sample_size(i), Some(p.data.len() as u64)); - } - if let Some(i) = pk.len().checked_sub(1) { - assert_eq!(file.read_sample(&bytes[..], ti, i).unwrap(), pk[i].data); - } - } - // duration - if let Some(fd) = json["format"]["duration"].as_str().and_then(|s| s.parse::().ok()) - && let Some(ours) = file.duration_ns() + eprintln!("{name}: {} packets, {pts_checked} pts and {duration_checked} durations compared", packets.len()); +} + +fn compare_packet_pts(ffprobe_packet: &Value, packet: &Packet, context: &str, checked: &mut usize) { + if let Some(pts) = int(&ffprobe_packet["pts"]) + && !(packet.lace > 0 && packet.duration == 0) { - assert!((fd - ours as f64 / 1e9).abs() < 0.05, "{name}: duration {fd} vs {ours}"); + // FFmpeg truncates per-lace duration to whole ticks; later laces can differ by one tick each. + assert!((pts - packet.pts).abs() <= packet.lace as i64, "{context}: pts ffprobe {pts} vs {}", packet.pts); + *checked += 1; } +} + +fn compare_packet_duration( + ffprobe_packet: &Value, + packet: &Packet, + packets: &[Packet], + file: &MkvFile, + context: &str, + index: usize, + checked: &mut usize, +) { + // The final audio/video frame cut by `-t` has a shortened BlockDuration, but ffprobe reports + // the codec frame duration. Subtitle durations remain exact cue intervals and are compared. + let last_of_track = packets[index + 1..].iter().all(|next| next.track != packet.track); + if let Some(duration) = int(&ffprobe_packet["duration"]) + && packet.duration != 0 + && (!last_of_track || file.tracks[packet.track].kind == TrackKind::Subtitle) + { + assert!((duration - packet.duration as i64).abs() <= 1, "{context}: duration ffprobe {duration} vs {}", packet.duration); + *checked += 1; + } +} + +fn assert_packet_index_matches(file: &MkvFile, packets: &[Packet], bytes: &[u8], name: &str) { + for (track_index, track) in file.tracks.iter().enumerate() { + let track_packets: Vec<&Packet> = packets.iter().filter(|packet| packet.track == track_index).collect(); + assert_eq!(track.samples.len(), track_packets.len(), "{name}: index size track {track_index}"); + for (sample_index, packet) in track_packets.iter().enumerate() { + assert_eq!(track.samples[sample_index].pts, packet.pts); + assert_eq!(track.samples[sample_index].keyframe, packet.keyframe); + assert_eq!(track.sample_size(sample_index), Some(packet.data.len() as u64)); + } + if let Some(sample_index) = track_packets.len().checked_sub(1) { + assert_eq!(file.read_sample(bytes, track_index, sample_index).unwrap(), track_packets[sample_index].data); + } + } +} + +fn assert_duration_matches(json: &Value, file: &MkvFile, name: &str) { + if let Some(ffprobe_duration) = json["format"]["duration"].as_str().and_then(|duration| duration.parse::().ok()) + && let Some(duration_ns) = file.duration_ns() + { + assert!((ffprobe_duration - duration_ns as f64 / 1e9).abs() < 0.05, "{name}: duration {ffprobe_duration} vs {duration_ns}"); + } +} + +/// Compare every packet (file order) and stream parameter against ffprobe. +fn compare(ffprobe: &Path, path: &Path) -> (MkvFile, Vec) { + let json = ffprobe_json(ffprobe, path); + let bytes = std::fs::read(path).unwrap(); + let mut demuxer = Demuxer::from_slice(&bytes).unwrap(); + let name = path.file_name().unwrap().to_string_lossy().to_string(); + let file = demuxer.file().clone(); + assert_streams_match(&json, &file, &name); + let packets: Vec = demuxer.by_ref().collect::>().unwrap(); + assert_packets_match(&json, &file, &packets, &name); + assert_packet_index_matches(&file, &packets, &bytes, &name); + assert_duration_matches(&json, &file, &name); (file, packets) } diff --git a/crates/text/src/linebreak/knuth_plass.rs b/crates/text/src/linebreak/knuth_plass.rs index 71f63c2d7..742c3245f 100644 --- a/crates/text/src/linebreak/knuth_plass.rs +++ b/crates/text/src/linebreak/knuth_plass.rs @@ -98,43 +98,11 @@ pub fn knuth_plass(items: &[Item], width: &dyn Fn(usize) -> f64, p: &Params) -> continue; } } - let weight = if f.ratio > 0.0 { - p.stretch_weight - } else if f.ratio < 0.0 { - p.shrink_weight - } else { - 1.0 - }; - let badness = (weight * 100.0 * f.ratio.abs().min(MAX_RATIO).powi(3)).min(p.max_badness); - let lp = p.line_penalty + badness; - let mut d = if forced { - lp * lp - } else if cost >= 0.0 { - lp * lp + cost * cost - } else { - lp * lp - cost * cost - }; - if flagged && n.flagged { - d += p.flagged_demerits; - } - // A ragged line ending at a forced break says nothing about the rag: it keeps its - // neighbour's class. - let class = fitness_class(f.ratio, &p.fitness); - let fitness = if forced && p.rag.is_some() { n.fitness.unwrap_or(class) } else { class }; - if n.fitness.is_some_and(|nf| fitness.abs_diff(nf) > 1) { - d += p.fitness_demerits; - } + let (d, fitness) = line_demerits(&n, f.ratio, forced, flagged, cost, p); let hyphens = if flagged { (n.hyphens + 1).min(max_hyphens) } else { 0 }; let line = (n.line + 1).min(p.uniform_from); let demerits = n.demerits + d; - match best.iter_mut().find(|c| c.line == line && c.fitness == fitness && c.hyphens == hyphens) { - Some(c) if c.demerits <= demerits => {} - Some(c) => { - c.demerits = demerits; - c.from = a; - } - None => best.push(Candidate { line, fitness, hyphens, demerits, from: a }), - } + record(&mut best, Candidate { line, fitness, hyphens, demerits, from: a }); } if p.overfull && keep.is_empty() @@ -197,6 +165,62 @@ pub fn knuth_plass(items: &[Item], width: &dyn Fn(usize) -> f64, p: &Params) -> } } let end = if s.ends_forced() { m - 1 } else { m }; + let chain = best_chain(&nodes, &active, end)?; + let mut start = 0; + let mut lines = Vec::with_capacity(chain.len()); + for (k, &b) in chain.iter().enumerate() { + let next = s.next_start(b); + lines.push(s.line(start, b, next, width(k), p, 0.0)); + start = next; + } + Some(lines) +} + +/// Keep the cheaper of the candidates sharing a (line class, fitness, hyphen state). +fn record(best: &mut Vec, new: Candidate) { + match best.iter_mut().find(|c| c.line == new.line && c.fitness == new.fitness && c.hyphens == new.hyphens) { + Some(c) if c.demerits <= new.demerits => {} + Some(c) => { + c.demerits = new.demerits; + c.from = new.from; + } + None => best.push(new), + } +} + +/// Demerits of ending a line of adjustment `ratio` after node `n` at a break, and its fitness class. +fn line_demerits(n: &Node, ratio: f64, forced: bool, flagged: bool, cost: f64, p: &Params) -> (f64, u8) { + let weight = if ratio > 0.0 { + p.stretch_weight + } else if ratio < 0.0 { + p.shrink_weight + } else { + 1.0 + }; + let badness = (weight * 100.0 * ratio.abs().min(MAX_RATIO).powi(3)).min(p.max_badness); + let lp = p.line_penalty + badness; + let mut d = if forced { + lp * lp + } else if cost >= 0.0 { + lp * lp + cost * cost + } else { + lp * lp - cost * cost + }; + if flagged && n.flagged { + d += p.flagged_demerits; + } + // A ragged line ending at a forced break says nothing about the rag: it keeps its + // neighbour's class. + let class = fitness_class(ratio, &p.fitness); + let fitness = if forced && p.rag.is_some() { n.fitness.unwrap_or(class) } else { class }; + if n.fitness.is_some_and(|nf| fitness.abs_diff(nf) > 1) { + d += p.fitness_demerits; + } + (d, fitness) +} + +/// Break positions of the cheapest path that ends at `end`, in order. +fn best_chain(nodes: &[Node], active: &[usize], end: usize) -> Option> { let last = active .iter() .copied() @@ -212,12 +236,5 @@ pub fn knuth_plass(items: &[Item], width: &dyn Fn(usize) -> f64, p: &Params) -> cur = n.prev; } chain.reverse(); - let mut start = 0; - let mut lines = Vec::with_capacity(chain.len()); - for (k, &b) in chain.iter().enumerate() { - let next = s.next_start(b); - lines.push(s.line(start, b, next, width(k), p, 0.0)); - start = next; - } - Some(lines) + Some(chain) } diff --git a/crates/ui/src/docking/command_tests.rs b/crates/ui/src/docking/command_tests.rs index c6ac3f1d7..fd693ac46 100644 --- a/crates/ui/src/docking/command_tests.rs +++ b/crates/ui/src/docking/command_tests.rs @@ -69,13 +69,18 @@ fn tab(before: Option<&str>) -> Placement

{ Placement::Tab { before: before.map(s) } } -#[test] -fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { - let mut seen = BTreeSet::new(); - let mut id = |params: Value| { - seen.insert(params["command"].as_str().unwrap().to_owned()); - params - }; +fn record_id(seen: &mut BTreeSet, params: Value) -> Value { + seen.insert(params["command"].as_str().unwrap().to_owned()); + params +} + +fn ui_dock_operation(mut params: Value) -> Value { + params["command"] = json!("ui.dock"); + params +} + +fn exercise_move_commands(seen: &mut BTreeSet) { + let mut id = |params: Value| record_id(seen, params); // The raw action form, in both protocols. let raw = json!({"MoveFloating": {"panel": "color", "rect": [44, 66, 360, 450]}}); @@ -105,6 +110,10 @@ fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { to: Target::At { anchor: s("layers"), placement: Placement::Group { below: false, height: None } } }] ); +} + +fn exercise_float_and_dock_commands(seen: &mut BTreeSet) { + let mut id = |params: Value| record_id(seen, params); // float: a docked panel, a lone floating panel, a group and a column. assert_eq!( @@ -149,6 +158,10 @@ fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { assert_eq!(actions(json!({"command": "window.panel.dock", "panel": "layers"})), vec![]); let (shown, _) = applied(json!({"command": "window.panel.dock", "panel": "swatches"})); assert!(shown.layout.contains(&s("swatches")) && shown.hidden.is_empty()); +} + +fn exercise_visibility_and_state_commands(seen: &mut BTreeSet) { + let mut id = |params: Value| record_id(seen, params); // activate shows a hidden panel; close actions report what they closed. let (shown, _) = applied(id(json!({"command": "window.panel.activate", "panel": "swatches"}))); @@ -171,6 +184,10 @@ fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { actions(id(json!({"command": "window.panel.iconic", "panel": "history"}))), vec![Action::SetIconic { panel: s("history"), iconic: true }] ); +} + +fn exercise_window_and_workspace_commands(seen: &mut BTreeSet) { + let mut id = |params: Value| record_id(seen, params); // window: a floating window goes native at its rect; a docked group floats first. assert_eq!( @@ -204,13 +221,12 @@ fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { // Lock toggles or sets. assert!(applied(id(json!({"command": "window.workspace.lock"}))).0.locked); assert!(!applied(json!({"command": "window.workspace.lock", "locked": false})).0.locked); +} + +fn exercise_ui_dock_commands() { + let op = ui_dock_operation; // The small apps' ui.dock operations. - let op = |params: Value| { - let mut params = params; - params["command"] = json!("ui.dock"); - params - }; let (shown, _) = applied(op(json!({"operation": "open", "panel": "swatches"}))); assert!(shown.layout.contains(&s("swatches"))); assert_eq!(actions(op(json!({"operation": "close", "panel": "layers"}))), vec![Action::Close { panel: s("layers") }]); @@ -250,9 +266,10 @@ fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { actions(op(json!({"operation": "resizeSplit", "path": [], "size": {"Ratio": 0.35}}))), vec![Action::ResizeSplit { path: vec![], size: SplitSize::Ratio(0.35) }] ); +} - let ids: BTreeSet = DockCommand::

::IDS.iter().map(|id| (*id).to_owned()).collect(); - assert_eq!(seen, ids, "one case per id"); +fn reject_invalid_commands() { + let op = ui_dock_operation; // Bad params: parse errors, or a DockError with nothing changed. for params in [ @@ -282,6 +299,9 @@ fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { DockError::InvalidPlacement, ); rejected(op(json!({"operation": "resizeSplit", "path": [true, true, true], "size": {"Ratio": 0.5}})), DockError::MissingTarget); +} + +fn reject_locked_native_window() { // A docked group cannot float into a native window when locked. let mut locked = workspace(); locked.locked = true; @@ -290,6 +310,20 @@ fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { assert_eq!(locked, before); } +#[test] +fn dock_commands_parse_every_documented_shape_and_map_to_atomic_actions() { + let mut seen = BTreeSet::new(); + exercise_move_commands(&mut seen); + exercise_float_and_dock_commands(&mut seen); + exercise_visibility_and_state_commands(&mut seen); + exercise_window_and_workspace_commands(&mut seen); + exercise_ui_dock_commands(); + let ids: BTreeSet = DockCommand::

::IDS.iter().map(|id| (*id).to_owned()).collect(); + assert_eq!(seen, ids, "one case per id"); + reject_invalid_commands(); + reject_locked_native_window(); +} + #[test] fn side_zones_map_to_the_placement_the_anchor_container_takes_in_either_direction() { let command = diff --git a/crates/ui/src/text.rs b/crates/ui/src/text.rs index ec4d78263..b3f9e48ba 100644 --- a/crates/ui/src/text.rs +++ b/crates/ui/src/text.rs @@ -158,6 +158,109 @@ mod tests { } } + fn fractional_ink_fixture() -> (Arc, f32, Rect, Galley) { + let shift = f32::from_bits(0x3f2a_aaab); + let mut original = layout_text(" \nI", 1.5, Align::Min); + let fixture = Arc::make_mut(&mut original); + assert_eq!(fixture.rows.len(), 2); + fixture.rect = Rect::from_min_size(Pos2::ZERO, vec2(1.0, 64.0)); + fixture.rows[0].pos = Pos2::ZERO; + let empty = Arc::make_mut(&mut fixture.rows[0].row); + empty.size.x = 0.5; // Max at width 0 snaps to -one pixel, not -two pixels. + empty.visuals = Default::default(); + fixture.rows[1].pos = pos2(0.0, shift); + let ink = Arc::make_mut(&mut fixture.rows[1].row); + ink.size.x = 1.0; + let mut mesh = egui::epaint::Mesh::default(); + mesh.add_rect_with_uv( + Rect::from_min_max(pos2(-2.0, -2.0), pos2(f32::from_bits(0x4055_5555), shift)), + Rect::from_min_max(Pos2::ZERO, pos2(1.0, 1.0)), + Color32::BLACK, + ); + ink.visuals = egui::epaint::text::RowVisuals { + mesh_bounds: mesh.calc_bounds(), + glyph_index_start: 0, + glyph_vertex_range: 0..mesh.vertices.len(), + mesh, + }; + let local = ink.visuals.mesh_bounds; + assert!(ink.visuals.mesh.is_valid()); + assert_eq!(ink.visuals.mesh.vertices.len(), 4); + assert_eq!(ink.visuals.mesh.indices.len(), 6); + for vertex in &ink.visuals.mesh.vertices { + assert!(local.contains(vertex.pos)); + } + fixture.mesh_bounds = Rect::from_min_max(local.min + vec2(0.0, shift), local.max + vec2(0.0, shift)); + fixture.num_vertices = 4; + fixture.num_indices = 6; + let snapshot = (*original).clone(); + assert!(snapshot.rows[0].visuals.mesh.vertices.is_empty()); + assert!(snapshot.rows[0].visuals.mesh.indices.is_empty()); + assert_eq!(snapshot.rows[0].visuals.mesh_bounds, Rect::NOTHING); + assert_eq!(snapshot.rows.iter().filter(|row| !row.visuals.mesh.vertices.is_empty()).count(), 1); + (original, shift, local, snapshot) + } + + fn align_fixture(galley: Arc) -> AlignedGalley { + let mut index = 0; + let aligned = align_galley_rows(galley, 0.0, |_| { + index += 1; + if index == 1 { Align::Max } else { Align::Min } + }); + assert_eq!(index, 2); + aligned + } + + fn assert_shared_rows_and_bounds(aligned: &Galley, snapshot: &Galley, offset: Vec2, shift: f32) { + assert_eq!(aligned.num_vertices, snapshot.num_vertices); + assert_eq!(aligned.num_indices, snapshot.num_indices); + for (index, (row, old)) in aligned.rows.iter().zip(&snapshot.rows).enumerate() { + assert!(Arc::ptr_eq(&row.row, &old.row), "cached glyphs and meshes stay shared"); + assert_eq!(row.pos.y, old.pos.y); + let reading_position = if index == 0 { pos2(-shift, 0.0) } else { old.pos }; + assert_eq!(row.pos + offset, reading_position); + assert!(aligned.rect.contains_rect(row.rect())); + } + assert_eq!(aligned.rows[0].visuals.mesh_bounds, Rect::NOTHING); + } + + fn assert_translated_ink_is_contained(aligned: &Galley, local: Rect) { + let row = &aligned.rows[1]; + let legacy = local.translate(row.pos.to_vec2()); + let endpoints = Rect::from_min_max(local.min + row.pos.to_vec2(), local.max + row.pos.to_vec2()); + assert_eq!(legacy.max.x.to_bits(), 0x407f_ffff, "the legacy X maximum rounds inward"); + assert_eq!(endpoints.max.x.to_bits(), 0x4080_0000); + assert_eq!(legacy.max.y.to_bits(), 0x3faa_aaac, "the legacy Y maximum rounds outward"); + assert_eq!(endpoints.max.y.to_bits(), 0x3faa_aaab); + assert!(endpoints.max.x > legacy.max.x && endpoints.max.y < legacy.max.y); + assert!(aligned.mesh_bounds.is_finite()); + assert!(aligned.mesh_bounds.contains_rect(legacy), "retain the complete public translated row rectangle"); + for (index, vertex) in row.visuals.mesh.vertices.iter().enumerate() { + let position = row.pos + vertex.pos.to_vec2(); + assert!( + aligned.mesh_bounds.contains(position), + "actual translated vertex {index} must be contained: position={:?} bits={:x?}, bounds_max={:?} bits={:x?}", + [position.x, position.y], + [position.x.to_bits(), position.y.to_bits()], + [aligned.mesh_bounds.max.x, aligned.mesh_bounds.max.y], + [aligned.mesh_bounds.max.x.to_bits(), aligned.mesh_bounds.max.y.to_bits()] + ); + } + assert_eq!(aligned.mesh_bounds, legacy.union(endpoints)); + } + + fn assert_empty_ink_stays_empty(original: &Galley) { + let mut all_empty = original.clone(); + for row in &mut all_empty.rows { + Arc::make_mut(&mut row.row).visuals = Default::default(); + } + all_empty.mesh_bounds = Rect::NOTHING; + all_empty.num_vertices = 0; + all_empty.num_indices = 0; + let all_empty = align_fixture(Arc::new(all_empty)); + assert_eq!(parts(&all_empty).0.mesh_bounds, Rect::NOTHING); + } + /// Compare two rects componentwise within `epsilon`. fn close_rect(actual: Rect, expected: Rect, epsilon: f32) { assert!((actual.min - expected.min).length() <= epsilon, "{actual:?} != {expected:?}"); @@ -377,57 +480,7 @@ mod tests { /// and the complete legacy rect, equals the union of both rounding directions, survives repetition, /// and remains `Rect::NOTHING` when all ink is empty. fn fractional_ink_bounds_contain_translated_rectangles_and_vertices() { - // A synthetic cached quad isolates the two directions of binary32 rounding. The - // whitespace row supplies the normalization offset without contributing any ink. - let shift = f32::from_bits(0x3f2a_aaab); - let mut original = layout_text(" \nI", 1.5, Align::Min); - let fixture = Arc::make_mut(&mut original); - assert_eq!(fixture.rows.len(), 2); - fixture.rect = Rect::from_min_size(Pos2::ZERO, vec2(1.0, 64.0)); - fixture.rows[0].pos = Pos2::ZERO; - let empty = Arc::make_mut(&mut fixture.rows[0].row); - empty.size.x = 0.5; // Max at width 0 snaps to -one pixel, not -two pixels. - empty.visuals = Default::default(); - fixture.rows[1].pos = pos2(0.0, shift); - let ink = Arc::make_mut(&mut fixture.rows[1].row); - ink.size.x = 1.0; - let mut mesh = egui::epaint::Mesh::default(); - mesh.add_rect_with_uv( - Rect::from_min_max(pos2(-2.0, -2.0), pos2(f32::from_bits(0x4055_5555), shift)), - Rect::from_min_max(Pos2::ZERO, pos2(1.0, 1.0)), - Color32::BLACK, - ); - ink.visuals = egui::epaint::text::RowVisuals { - mesh_bounds: mesh.calc_bounds(), - glyph_index_start: 0, - glyph_vertex_range: 0..mesh.vertices.len(), - mesh, - }; - let local = ink.visuals.mesh_bounds; - assert!(ink.visuals.mesh.is_valid()); - assert_eq!(ink.visuals.mesh.vertices.len(), 4); - assert_eq!(ink.visuals.mesh.indices.len(), 6); - for vertex in &ink.visuals.mesh.vertices { - assert!(local.contains(vertex.pos)); - } - fixture.mesh_bounds = Rect::from_min_max(local.min + vec2(0.0, shift), local.max + vec2(0.0, shift)); - fixture.num_vertices = 4; - fixture.num_indices = 6; - let snapshot = (*original).clone(); - assert!(snapshot.rows[0].visuals.mesh.vertices.is_empty()); - assert!(snapshot.rows[0].visuals.mesh.indices.is_empty()); - assert_eq!(snapshot.rows[0].visuals.mesh_bounds, Rect::NOTHING); - assert_eq!(snapshot.rows.iter().filter(|row| !row.visuals.mesh.vertices.is_empty()).count(), 1); - - let align_fixture = |galley| { - let mut index = 0; - let aligned = align_galley_rows(galley, 0.0, |_| { - index += 1; - if index == 1 { Align::Max } else { Align::Min } - }); - assert_eq!(index, 2); - aligned - }; + let (original, shift, local, snapshot) = fractional_ink_fixture(); let result = align_fixture(original.clone()); let (aligned, offset, reading) = parts(&result); assert_eq!(offset, vec2(-shift, 0.0)); @@ -438,54 +491,15 @@ mod tests { assert_eq!(reading, Rect::from_min_size(Pos2::ZERO, vec2(0.0, 64.0))); assert!(aligned.rect.contains_rect(reading.translate(-offset))); assert_eq!(*original, snapshot, "alignment must leave the complete source unchanged"); - assert_eq!(aligned.num_vertices, snapshot.num_vertices); - assert_eq!(aligned.num_indices, snapshot.num_indices); - for (index, (row, old)) in aligned.rows.iter().zip(&snapshot.rows).enumerate() { - assert!(Arc::ptr_eq(&row.row, &old.row), "cached glyphs and meshes stay shared"); - assert_eq!(row.pos.y, old.pos.y); - let reading_position = if index == 0 { pos2(-shift, 0.0) } else { old.pos }; - assert_eq!(row.pos + offset, reading_position); - assert!(aligned.rect.contains_rect(row.rect())); - } - assert_eq!(aligned.rows[0].visuals.mesh_bounds, Rect::NOTHING); + assert_shared_rows_and_bounds(aligned, &snapshot, offset, shift); let repeated = align_fixture(aligned.clone()); let (again, again_offset, again_reading) = parts(&repeated); assert_eq!(again_offset, offset); assert_eq!(again_reading, reading); assert_eq!(**again, **aligned, "repeated alignment is exact, including cached ink bounds"); - let mut all_empty = (*original).clone(); - for row in &mut all_empty.rows { - Arc::make_mut(&mut row.row).visuals = Default::default(); - } - all_empty.mesh_bounds = Rect::NOTHING; - all_empty.num_vertices = 0; - all_empty.num_indices = 0; - let all_empty = align_fixture(Arc::new(all_empty)); - assert_eq!(parts(&all_empty).0.mesh_bounds, Rect::NOTHING); - - let row = &aligned.rows[1]; - let legacy = local.translate(row.pos.to_vec2()); - let endpoints = Rect::from_min_max(local.min + row.pos.to_vec2(), local.max + row.pos.to_vec2()); - assert_eq!(legacy.max.x.to_bits(), 0x407f_ffff, "the legacy X maximum rounds inward"); - assert_eq!(endpoints.max.x.to_bits(), 0x4080_0000); - assert_eq!(legacy.max.y.to_bits(), 0x3faa_aaac, "the legacy Y maximum rounds outward"); - assert_eq!(endpoints.max.y.to_bits(), 0x3faa_aaab); - assert!(endpoints.max.x > legacy.max.x && endpoints.max.y < legacy.max.y); - assert!(aligned.mesh_bounds.is_finite()); - assert!(aligned.mesh_bounds.contains_rect(legacy), "retain the complete public translated row rectangle"); - for (index, vertex) in row.visuals.mesh.vertices.iter().enumerate() { - let position = row.pos + vertex.pos.to_vec2(); - assert!( - aligned.mesh_bounds.contains(position), - "actual translated vertex {index} must be contained: position={:?} bits={:x?}, bounds_max={:?} bits={:x?}", - [position.x, position.y], - [position.x.to_bits(), position.y.to_bits()], - [aligned.mesh_bounds.max.x, aligned.mesh_bounds.max.y], - [aligned.mesh_bounds.max.x.to_bits(), aligned.mesh_bounds.max.y.to_bits()] - ); - } - assert_eq!(aligned.mesh_bounds, legacy.union(endpoints)); + assert_empty_ink_stays_empty(&original); + assert_translated_ink_is_contained(aligned, local); } #[test] From b380d16304123f724d8706ce59d24a06999156b8 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:18:59 -0400 Subject: [PATCH 08/13] ci: run macOS only when a workflow is started by hand (#399) * ci: window and ui run macOS only when started by hand macOS runners are kept for releases. Pushes and pull requests run Linux and Windows; workflow_dispatch runs all three. * ci: the other workflows run macOS only when started by hand atomic, docpath, ai-core, i18n, print, pdf, update and keymap follow window and ui: Linux and Windows on pushes and pull requests (keymap: Windows), all their platforms on workflow_dispatch. --- .github/workflows/ai-core.yml | 3 ++- .github/workflows/atomic.yml | 3 ++- .github/workflows/docpath.yml | 3 ++- .github/workflows/i18n.yml | 3 ++- .github/workflows/keymap.yml | 3 ++- .github/workflows/pdf.yml | 3 ++- .github/workflows/print.yml | 3 ++- .github/workflows/ui.yml | 3 ++- .github/workflows/update.yml | 3 ++- .github/workflows/window.yml | 3 ++- 10 files changed, 20 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ai-core.yml b/.github/workflows/ai-core.yml index 2dd031687..a18147307 100644 --- a/.github/workflows/ai-core.yml +++ b/.github/workflows/ai-core.yml @@ -17,7 +17,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 diff --git a/.github/workflows/atomic.yml b/.github/workflows/atomic.yml index 9cda9f286..8e8e1d201 100644 --- a/.github/workflows/atomic.yml +++ b/.github/workflows/atomic.yml @@ -17,7 +17,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 diff --git a/.github/workflows/docpath.yml b/.github/workflows/docpath.yml index 3cae8fb57..5192ade96 100644 --- a/.github/workflows/docpath.yml +++ b/.github/workflows/docpath.yml @@ -17,7 +17,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 diff --git a/.github/workflows/i18n.yml b/.github/workflows/i18n.yml index 7a8876499..adb4208bd 100644 --- a/.github/workflows/i18n.yml +++ b/.github/workflows/i18n.yml @@ -17,7 +17,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 diff --git a/.github/workflows/keymap.yml b/.github/workflows/keymap.yml index 51ff2ba0b..db25d0dd3 100644 --- a/.github/workflows/keymap.yml +++ b/.github/workflows/keymap.yml @@ -55,7 +55,8 @@ jobs: strategy: fail-fast: false matrix: - os: [macos-latest, windows-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["macos-latest","windows-latest"]') || fromJSON('["windows-latest"]') }} runs-on: ${{ matrix.os }} env: CRAFT_FONTS_DIR: ${{ github.workspace }}/.test-inputs/craft-fonts diff --git a/.github/workflows/pdf.yml b/.github/workflows/pdf.yml index 2ff73a7a9..45ff86c65 100644 --- a/.github/workflows/pdf.yml +++ b/.github/workflows/pdf.yml @@ -21,7 +21,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} env: RUSTFLAGS: --cfg craft_pdf_unicode_fixtures diff --git a/.github/workflows/print.yml b/.github/workflows/print.yml index 9e144a046..24c372df0 100644 --- a/.github/workflows/print.yml +++ b/.github/workflows/print.yml @@ -14,7 +14,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 diff --git a/.github/workflows/ui.yml b/.github/workflows/ui.yml index e9299f371..0086bc24a 100644 --- a/.github/workflows/ui.yml +++ b/.github/workflows/ui.yml @@ -22,7 +22,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 diff --git a/.github/workflows/update.yml b/.github/workflows/update.yml index be10934e6..c8f3a9cae 100644 --- a/.github/workflows/update.yml +++ b/.github/workflows/update.yml @@ -17,7 +17,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/window.yml b/.github/workflows/window.yml index b384d61b6..2d2e420fb 100644 --- a/.github/workflows/window.yml +++ b/.github/workflows/window.yml @@ -17,7 +17,8 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest, macos-latest] + # macOS runners are kept for releases: macOS runs only when started by hand. + os: ${{ github.event_name == 'workflow_dispatch' && fromJSON('["ubuntu-latest","windows-latest","macos-latest"]') || fromJSON('["ubuntu-latest","windows-latest"]') }} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 From e11f9d9aebea5f6b8e74643c1ea1ed172ba5f545 Mon Sep 17 00:00:00 2001 From: enriquemelzer-bit Date: Tue, 27 Oct 2026 22:53:44 -0300 Subject: [PATCH 09/13] fix(menu): preserve Cargo 1.95 manifest compatibility (#366) Co-authored-by: enriquemelzer-bit <319226743+enriquemelzer-bit@users.noreply.github.com> --- crates/menu/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/menu/Cargo.toml b/crates/menu/Cargo.toml index cac69ca29..7ff68ee09 100644 --- a/crates/menu/Cargo.toml +++ b/crates/menu/Cargo.toml @@ -8,7 +8,7 @@ rust-version.workspace = true repository.workspace = true [dependencies] -craft-keymap = { workspace = true, default-features = false, features = ["egui"] } +craft-keymap = { workspace = true, features = ["egui"] } craft-quit = { workspace = true, features = ["egui"] } egui = { workspace = true } From 5f00550cf7c621672b424b818312473a3b02299e Mon Sep 17 00:00:00 2001 From: aiongg Date: Wed, 28 Oct 2026 04:28:55 +0000 Subject: [PATCH 10/13] print: respell a QName value written after whitespace A pretty-printed ticket puts whitespace before the value, so the prefix starts after it; without this the ticket fell back to Windows' spelling. --- crates/print/src/ticket.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/print/src/ticket.rs b/crates/print/src/ticket.rs index 9b76290e4..e1ba8ce75 100644 --- a/crates/print/src/ticket.rs +++ b/crates/print/src/ticket.rs @@ -354,7 +354,10 @@ pub(crate) fn respell(ticket: &str, driver: &str) -> Option { edits.extend(respelled(a.range_value().start)); } } else if node.is_text() && node.parent().is_some_and(is_qname_value) { - edits.extend(respelled(node.range().start)); + // A pretty-printed ticket puts whitespace before the value; the prefix starts after it. + let start = node.range().start; + let lead = ticket.get(start..).map_or(0, |t| t.len() - t.trim_start().len()); + edits.extend(respelled(start + lead)); } } edits.sort_unstable_by_key(|(start, ..)| *start); @@ -502,6 +505,13 @@ mod tests { ) } + /// A QName value written after whitespace (a pretty-printed ticket) is respelled too. + #[test] + fn a_qname_value_after_whitespace_is_respelled() { + let spaced = |prefix: &str| brother(prefix).replace(&format!(">{prefix}:Fast<"), &format!(">\n {prefix}:Fast\n<")); + assert_eq!(respell(&spaced("ns0000"), &brother("brpsk")), Some(spaced("brpsk"))); + } + /// The ticket Windows returns (`ns0000`) gets the driver's own `brpsk` back, in the declaration, the names and the QName values, but not in text. #[test] fn a_vendor_namespace_gets_the_prefix_the_driver_declares_it_with() { From 1a11a4897865f7410c40d25ca6e452ce399a1a2c Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Wed, 28 Oct 2026 16:49:37 -0400 Subject: [PATCH 11/13] craft-dialogs: native file and message dialogs that never block the event loop (#412) * Add craft-dialogs: native file and message dialogs that never block the event loop Requests queue and show at the end of the frame on the main thread; answers come back with the app's continuation on a later frame. One dialog at a time, a drop guard and a panic guard so a dialog never stays open, and an override for tests. On macOS, Open and Folder show as a free-standing NSOpenPanel (beginWithCompletionHandler, which rfd can't show), Save and message boxes as sheets through rfd's async dialogs, never falling back to runModal. Filter extensions match in any letter case on Linux. * craft-dialogs: Dialogs::raise brings the open macOS panel to the front * CI: dialogs workflow (Linux clippy and tests for craft-dialogs) * craft-dialogs: per-instance macOS panel, parent window check, override panic guard, CI lints macOS code * craft-dialogs: raise only a visible panel; Open and Folder need a running app --- .github/workflows/dialogs.yml | 31 +++ Cargo.lock | 48 ++++- Cargo.toml | 1 + README.md | 4 + crates/dialogs/Cargo.toml | 26 +++ crates/dialogs/README.md | 114 +++++++++++ crates/dialogs/src/filter.rs | 58 ++++++ crates/dialogs/src/lib.rs | 347 ++++++++++++++++++++++++++++++++++ crates/dialogs/src/macos.rs | 112 +++++++++++ crates/dialogs/src/native.rs | 174 +++++++++++++++++ crates/dialogs/src/tests.rs | 207 ++++++++++++++++++++ 11 files changed, 1120 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/dialogs.yml create mode 100644 crates/dialogs/Cargo.toml create mode 100644 crates/dialogs/README.md create mode 100644 crates/dialogs/src/filter.rs create mode 100644 crates/dialogs/src/lib.rs create mode 100644 crates/dialogs/src/macos.rs create mode 100644 crates/dialogs/src/native.rs create mode 100644 crates/dialogs/src/tests.rs diff --git a/.github/workflows/dialogs.yml b/.github/workflows/dialogs.yml new file mode 100644 index 000000000..f56209999 --- /dev/null +++ b/.github/workflows/dialogs.yml @@ -0,0 +1,31 @@ +name: Dialogs + +on: + workflow_dispatch: + push: + branches: [main] + paths: ['crates/dialogs/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/dialogs.yml'] + pull_request: + paths: ['crates/dialogs/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/dialogs.yml'] + +permissions: + contents: read + +jobs: + dialogs: + timeout-minutes: 15 + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + # The token isn't needed after checkout; cargo runs build scripts and proc macros from + # dependencies, so don't leave it in .git/config for that third-party code to read. + persist-credentials: false + - uses: dtolnay/rust-toolchain@b725eda525362e7cf34af9a7243cac1e3f01a7ec # stable + with: + components: clippy + # The macOS code is linted on Linux: it compiles for the target without the SDK. + targets: aarch64-apple-darwin + - run: cargo clippy -p craft-dialogs --all-targets --locked -- -D warnings + - run: cargo clippy -p craft-dialogs --target aarch64-apple-darwin --locked -- -D warnings + - run: cargo test -p craft-dialogs --locked diff --git a/Cargo.lock b/Cargo.lock index e2f00610c..77ba4276b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -775,6 +775,18 @@ dependencies = [ "rayon", ] +[[package]] +name = "craft-dialogs" +version = "0.1.0" +dependencies = [ + "block2 0.6.2", + "objc2 0.6.5", + "objc2-app-kit 0.3.2", + "objc2-foundation 0.3.2", + "raw-window-handle", + "rfd", +] + [[package]] name = "craft-docpath" version = "0.1.0" @@ -1254,6 +1266,8 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1e0e367e4e7da84520dedcac1901e4da967309406d1e51017ae1abfb97adbd38" dependencies = [ "bitflags 2.13.2", + "block2 0.6.2", + "libc", "objc2 0.6.5", ] @@ -1336,7 +1350,7 @@ dependencies = [ "objc2-foundation 0.3.2", "parking_lot", "percent-encoding", - "pollster", + "pollster 1.0.1", "profiling", "raw-window-handle", "send_wrapper", @@ -1448,7 +1462,7 @@ dependencies = [ "kittest", "log", "open", - "pollster", + "pollster 1.0.1", "serde", "tempfile", "toml", @@ -3906,6 +3920,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "pollster" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f3a9f18d041e6d0e102a0a46750538147e5e8992d3b4873aaafee2520b00ce3" + [[package]] name = "pollster" version = "1.0.1" @@ -4389,6 +4409,30 @@ dependencies = [ "usvg", ] +[[package]] +name = "rfd" +version = "0.17.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "20dafead71c16a34e1ff357ddefc8afc11e7d51d6d2b9fbd07eaa48e3e540220" +dependencies = [ + "block2 0.6.2", + "dispatch2", + "js-sys", + "libc", + "log", + "objc2 0.6.5", + "objc2-app-kit 0.3.2", + "objc2-core-foundation", + "objc2-foundation 0.3.2", + "percent-encoding", + "pollster 0.4.0", + "raw-window-handle", + "wasm-bindgen", + "wasm-bindgen-futures", + "web-sys", + "windows-sys 0.61.2", +] + [[package]] name = "rgb" version = "0.8.53" diff --git a/Cargo.toml b/Cargo.toml index daa783f89..a4ab549b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ kurbo = "0.13" thiserror = "2" craft-atomic = { path = "crates/atomic" } craft-cms = { path = "crates/cms" } +craft-dialogs = { path = "crates/dialogs" } craft-docpath = { path = "crates/docpath" } craft-font-catalog = { path = "crates/font-catalog" } craft-font-manager = { path = "crates/font-manager" } diff --git a/README.md b/README.md index 421c5aeaa..55adcbfd9 100644 --- a/README.md +++ b/README.md @@ -34,6 +34,10 @@ craft- = { git = "https://github.com/storytold/craft-libs", rev = "; +pub type Override = Box Answer>; + +pub enum Kind { Open { multiple: bool }, Folder, Save } +pub struct FileDialog { pub kind: Kind, pub title: Option, pub filters: Vec, + pub directory: Option, pub file_name: Option } +pub struct Filter { pub name: String, pub extensions: Vec } +pub struct Message { pub level: MessageLevel, pub title: String, pub description: String, + pub buttons: MessageButtons } +pub enum Request { File(FileDialog), Message(Message) } +pub enum Answer { Cancelled, Paths(Vec), Message(MessageResult), Unavailable(String) } + +impl Dialogs { + pub fn new(wake: Wake) -> Self; + pub fn ask(&mut self, dialog: FileDialog, then: T); + pub fn ask_message(&mut self, message: Message, then: T); + pub fn show_pending(&mut self, parent: Option<&W>); + pub fn take_answers(&mut self) -> Vec<(T, Answer)>; + pub fn is_open(&self) -> bool; + pub fn raise(&self); + pub fn queued(&self) -> usize; + pub fn set_override(&mut self, answer_with: Option); +} +``` + +`FileDialog` has builders: `FileDialog::open().title("Place").filter("Images", &["png", "jpg"])`, +and `open_many`, `folder`, `save`, `directory`, `file_name`. + +In an eframe app: + +```rust +// Once, at startup. +let ctx = cc.egui_ctx.clone(); +let dialogs = Dialogs::new(Arc::new(move || ctx.request_repaint())); + +// A command that needs a file. +self.dialogs.ask(FileDialog::open().filter("PDF", &["pdf"]), Then::Open); + +// Every frame: answers first, then show what is queued, with the window as parent. +for (then, answer) in self.dialogs.take_answers() { + match answer { + Answer::Paths(paths) => self.run(then, paths), + Answer::Unavailable(why) => self.report_error(why), + Answer::Cancelled | Answer::Message(_) => {} + } +} +self.dialogs.show_pending(Some(frame)); +``` + +- `then` is the app's continuation: a command to re-run with the path, a panel field to fill. It + stays on the main thread, so it needs no `Send`. +- One dialog shows at a time. A request made while one is open waits and shows when it closes, in + the order asked. +- A dialog whose worker ends without answering (it panicked, or its thread couldn't start) + answers `Unavailable`. A panic while showing one answers `Unavailable` too, instead of unwinding + into winit's handler. Under `panic = "abort"` a panic while showing aborts the app; the crate + checks what rfd would otherwise panic on (on macOS, a parent view that isn't in a window) before + it calls rfd. +- `Unavailable` means no dialog could be shown; report it as an error, not as Cancel. +- `set_override` answers every request without a native dialog, for tests and automation. The + answers still arrive through `take_answers` after `show_pending`. +- Filter extensions match in any letter case on every platform. On Linux and the BSDs, where rfd + turns each extension into a case-sensitive `*.ext` glob, each one is also passed as `EXT` and + `[eE][xX][tT]`. + +## Presentation + +| Dialog | macOS | Windows, Linux, BSD | +| --- | --- | --- | +| Open, Folder | free-standing panel that doesn't block the window | modal owned by the parent | +| Save | sheet on the parent window | modal owned by the parent | +| Message | sheet on the parent window | modal owned by the parent | + +On macOS, Open and Folder follow native apps: `-[NSOpenPanel beginWithCompletionHandler:]` +shows a panel the user can move away from the window, and the window keeps working. A sheet stays +centred on its window when the window is resized, which feels wrong for Open. rfd 0.17.2 can't +show this panel: its async dialogs always begin a sheet, on the main or first window when no +parent is given (`backend/macos/modal_future.rs:71-102`). The crate shows it with objc2-app-kit's +safe bindings. A request made while the panel is open brings the panel to the front. + +Save and message boxes are sheets through rfd's async dialogs. rfd falls back to the blocking +`runModal` when the app isn't running or has no window (`modal_future.rs:79-118`), so on macOS +they start only with a parent in a running app, and answer `Unavailable` otherwise. Dialogs needed +before the event loop starts or after it ends (startup and crash messages) stay blocking `rfd` +calls in the app. + +## Follow-ups + +- Linux portal Save (`org.freedesktop.portal.FileChooser.SaveFile`), so the file type the user + picks sets the extension (PhotoCraft, LightCraft, WordCraft have it). +- Missing-portal probe: tell the user what to install when Linux has no desktop portal and no + zenity, instead of an empty answer that looks like Cancel. +- A dialog that never answers (a hung portal) keeps `is_open()` true and the queue waiting. A + timeout or a way to give up on it would free the app. +- wasm: the same API, answering with file contents, without `std::thread`. +- Android. diff --git a/crates/dialogs/src/filter.rs b/crates/dialogs/src/filter.rs new file mode 100644 index 000000000..fd35eb704 --- /dev/null +++ b/crates/dialogs/src/filter.rs @@ -0,0 +1,58 @@ +//! File-type filters that match extensions in any letter case. +//! +//! On Linux and the BSDs rfd turns each extension into a `*.ext` glob for the desktop portal, GTK +//! and zenity, which all match case-sensitively: a filter of `nef` hid `DSC_1234.NEF`. Windows and +//! macOS ignore case already. + +/// A file type a dialog offers: its name and its extensions, without the dot. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Filter { + pub name: String, + pub extensions: Vec, +} + +impl Filter { + pub fn new(name: impl Into, extensions: &[&str]) -> Self { + Self { name: name.into(), extensions: extensions.iter().map(|e| (*e).to_owned()).collect() } + } + + /// The patterns to hand rfd on this platform. + pub(crate) fn patterns(&self) -> Vec { + patterns(&self.extensions, cfg!(all(unix, not(target_vendor = "apple")))) + } +} + +/// The patterns for `extensions`. Without `globs` (Windows, macOS): each extension once, in lower +/// case. With `globs` (rfd formats each pattern as `*.{pattern}`): the lower-case form, the +/// upper-case form (for a portal backend without `[...]` classes) and a class per letter, such as +/// `[nN][eE][fF]`, that matches every mix. +pub(crate) fn patterns(extensions: &[String], globs: bool) -> Vec { + let mut out: Vec = Vec::new(); + let mut push = |s: String| { + if !s.is_empty() && !out.contains(&s) { + out.push(s); + } + }; + for ext in extensions { + let lower = ext.trim_start_matches('.').to_lowercase(); + if !globs { + push(lower); + continue; + } + push(lower.clone()); + push(lower.to_uppercase()); + // An extension that is already glob syntax stays as it is. + if lower.chars().any(char::is_alphabetic) && !lower.contains(['[', ']', '*', '?', '\\']) { + push( + lower + .chars() + .map(|c| { + let up: String = c.to_uppercase().collect(); + if up.chars().count() == 1 && up != c.to_string() { format!("[{c}{up}]") } else { c.to_string() } + }) + .collect(), + ); + } + } + out +} diff --git a/crates/dialogs/src/lib.rs b/crates/dialogs/src/lib.rs new file mode 100644 index 000000000..47133870b --- /dev/null +++ b/crates/dialogs/src/lib.rs @@ -0,0 +1,347 @@ +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] + +//! Native file and message dialogs that never block the event loop. +//! +//! A blocking `rfd::FileDialog` or `rfd::MessageDialog` runs AppKit's modal loop +//! (`-[NSSavePanel runModal]`) inside winit's event handler on macOS. An event that arrives while +//! that loop spins (a file dragged over the window, the panel resized) re-enters the handler, winit +//! panics with "tried to handle event while another event is currently being handled", and since +//! the panic can't unwind out of the AppKit callback, the app aborts. +//! +//! [`Dialogs`] never blocks: [`Dialogs::ask`] and [`Dialogs::ask_message`] queue a request, +//! [`Dialogs::show_pending`] shows the first one at the end of the frame on the main thread, and +//! [`Dialogs::take_answers`] hands back `(then, Answer)` pairs on a later frame. `then` is the +//! app's own continuation (a command to re-run, a field to fill), so each app keeps its command +//! model. One dialog shows at a time; later requests wait in order. +//! +//! On macOS, Open and Folder show as a free-standing panel that doesn't block the window, as +//! native apps do; Save and message boxes show as a sheet on the parent window. Elsewhere every +//! dialog is a modal owned by the parent window. + +mod filter; +mod native; + +#[cfg(target_os = "macos")] +mod macos; + +#[cfg(test)] +mod tests; + +use std::collections::VecDeque; +use std::panic::{AssertUnwindSafe, catch_unwind}; +use std::path::PathBuf; +use std::sync::Arc; +use std::sync::mpsc::{self, Receiver, Sender, TryRecvError}; + +use raw_window_handle::{HasDisplayHandle, HasWindowHandle}; + +pub use filter::Filter; + +/// Wakes the app's event loop so it runs a frame (`ctx.request_repaint()` in an egui app). Called +/// from any thread when a dialog has answered or stopped. +pub type Wake = Arc; + +/// Answers every request instead of a native dialog (tests and automation). +pub type Override = Box Answer>; + +/// Which file dialog to show. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Kind { + /// Choose existing files: one, or several when `multiple`. + Open { multiple: bool }, + /// Choose one folder. + Folder, + /// Choose where to write a file. + Save, +} + +/// A file dialog request. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct FileDialog { + pub kind: Kind, + /// The dialog's title, or its message line on macOS, where panels have no title bar. + pub title: Option, + /// The file types to offer. Extensions match in any letter case on every platform. + pub filters: Vec, + /// The folder the dialog starts in. + pub directory: Option, + /// The file name a Save dialog suggests. + pub file_name: Option, +} + +impl FileDialog { + /// A dialog of `kind` with nothing else set. + pub fn new(kind: Kind) -> Self { + Self { kind, title: None, filters: Vec::new(), directory: None, file_name: None } + } + + /// Choose one existing file. + pub fn open() -> Self { + Self::new(Kind::Open { multiple: false }) + } + + /// Choose one or more existing files. + pub fn open_many() -> Self { + Self::new(Kind::Open { multiple: true }) + } + + /// Choose a folder. + pub fn folder() -> Self { + Self::new(Kind::Folder) + } + + /// Choose where to save. + pub fn save() -> Self { + Self::new(Kind::Save) + } + + pub fn title(mut self, title: impl Into) -> Self { + self.title = Some(title.into()); + self + } + + /// Adds a file type: `name` as the dialog lists it, `extensions` without the dot. + pub fn filter(mut self, name: impl Into, extensions: &[&str]) -> Self { + self.filters.push(Filter::new(name, extensions)); + self + } + + pub fn directory(mut self, directory: impl Into) -> Self { + self.directory = Some(directory.into()); + self + } + + pub fn file_name(mut self, file_name: impl Into) -> Self { + self.file_name = Some(file_name.into()); + self + } +} + +/// The icon a message box shows. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum MessageLevel { + #[default] + Info, + Warning, + Error, +} + +/// The buttons a message box offers. The custom variants name the buttons, in the order of the +/// plain variant they replace. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub enum MessageButtons { + #[default] + Ok, + OkCancel, + YesNo, + YesNoCancel, + OkCustom(String), + OkCancelCustom(String, String), + YesNoCancelCustom(String, String, String), +} + +/// The button a message box was closed with. A custom button answers with its label. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum MessageResult { + Ok, + Cancel, + Yes, + No, + Custom(String), +} + +/// A message box request. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct Message { + pub level: MessageLevel, + pub title: String, + pub description: String, + pub buttons: MessageButtons, +} + +/// What [`Dialogs`] was asked to show, as an [`Override`] sees it. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Request { + File(FileDialog), + Message(Message), +} + +/// How a dialog ended. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Answer { + /// The user closed a file dialog without choosing. + Cancelled, + /// The chosen files, folder or save path. + Paths(Vec), + /// The button a message box was closed with. + Message(MessageResult), + /// No dialog could be shown, or it stopped without an answer; the text says why. Report it as + /// an error rather than taking it for Cancel. + Unavailable(String), +} + +/// The answer of a dialog whose worker ended without answering (it panicked, or couldn't start). +pub(crate) const STOPPED: &str = "the dialog stopped without an answer"; + +/// Hands one dialog's answer back to [`Dialogs`] from any thread, and wakes the app. Dropped +/// without an answer (a worker that panicked or never started), it still wakes the app, which then +/// answers [`Answer::Unavailable`], so a dialog never stays open for good. +pub(crate) struct Reply { + tx: Option>, + wake: Wake, +} + +impl Reply { + pub(crate) fn send(mut self, answer: Answer) { + if let Some(tx) = self.tx.take() { + // The app gone meanwhile has no use for the answer. + let _ = tx.send(answer); + } + } +} + +impl Drop for Reply { + fn drop(&mut self) { + // The sender goes first, so the woken app sees either the answer or the disconnect. + self.tx = None; + (self.wake)(); + } +} + +/// Who shows a request. +enum Show { + Native, + Override(Override), + /// Tests: a stand-in for the native dialog that answers (or doesn't) through the reply. + #[cfg(test)] + Custom(Box), +} + +/// The dialog on screen. +struct Open { + then: T, + answer: Receiver, + #[cfg_attr(not(target_os = "macos"), allow(dead_code))] + shown: native::Shown, +} + +/// The app's dialogs: a queue of requests, the one on screen, and the answers not yet taken. +/// +/// `T` is the app's continuation for an answer. It stays on the main thread: no `Send` bound. +pub struct Dialogs { + wake: Wake, + queue: VecDeque<(Request, T)>, + open: Option>, + answers: Vec<(T, Answer)>, + show: Show, +} + +impl Dialogs { + pub fn new(wake: Wake) -> Self { + Self { wake, queue: VecDeque::new(), open: None, answers: Vec::new(), show: Show::Native } + } + + /// Queues a file dialog. It shows at the next [`Self::show_pending`] once the dialogs asked + /// before it have answered; its answer comes back with `then`. + pub fn ask(&mut self, dialog: FileDialog, then: T) { + self.push(Request::File(dialog), then); + } + + /// Queues a message box, like [`Self::ask`]. + pub fn ask_message(&mut self, message: Message, then: T) { + self.push(Request::Message(message), then); + } + + fn push(&mut self, request: Request, then: T) { + if self.open.is_some() { + self.raise(); + } + self.queue.push_back((request, then)); + (self.wake)(); + } + + /// Shows the next queued request unless a dialog is open. Call it on the main thread at the end + /// of every frame, with the window as `parent`. On macOS, Save and message boxes need the + /// parent (they show as its sheet) and answer [`Answer::Unavailable`] without one. + pub fn show_pending(&mut self, parent: Option<&W>) { + self.poll(); + while self.open.is_none() { + let Some((request, then)) = self.queue.pop_front() else { return }; + let (tx, answer) = mpsc::channel(); + let reply = Reply { tx: Some(tx), wake: self.wake.clone() }; + // A panic while showing (rfd's own checks) must not unwind into winit's handler. + let shown = match &mut self.show { + Show::Override(answer_with) => catch_unwind(AssertUnwindSafe(|| reply.send(answer_with(&request)))).map(|()| None), + Show::Native => catch_unwind(AssertUnwindSafe(|| native::show(request, parent, reply))), + #[cfg(test)] + Show::Custom(show) => catch_unwind(AssertUnwindSafe(|| show(request, reply))).map(|()| None), + }; + let shown = match shown { + Ok(shown) => shown, + Err(panic) => { + let why = panic_text(panic.as_ref()).unwrap_or(STOPPED); + self.answers.push((then, Answer::Unavailable(format!("the dialog couldn't be shown: {why}")))); + continue; + } + }; + self.open = Some(Open { then, answer, shown }); + self.poll(); + } + } + + /// The answers that came in since the last call, in the order the dialogs were asked. + pub fn take_answers(&mut self) -> Vec<(T, Answer)> { + self.poll(); + std::mem::take(&mut self.answers) + } + + /// A dialog is on screen, or its answer hasn't been collected yet (by [`Self::show_pending`] + /// or [`Self::take_answers`]). + pub fn is_open(&self) -> bool { + self.open.is_some() + } + + /// Brings the open free-standing panel (Open or Folder on macOS) to the front, for a command + /// that would show it again. Does nothing elsewhere, where dialogs are modal to the window. + pub fn raise(&self) { + #[cfg(target_os = "macos")] + if let Some(panel) = self.open.as_ref().and_then(|open| open.shown.as_ref()) { + macos::raise(panel); + } + } + + /// Requests waiting behind the open dialog. + pub fn queued(&self) -> usize { + self.queue.len() + } + + /// Answers every request with `answer_with` instead of a native dialog (tests and automation); + /// `None` shows native dialogs again. The answers still come back from + /// [`Self::take_answers`], after [`Self::show_pending`], like real ones. + pub fn set_override(&mut self, answer_with: Option) { + self.show = answer_with.map_or(Show::Native, Show::Override); + } + + /// Tests: show requests through `show` instead of a native dialog. + #[cfg(test)] + fn set_custom(&mut self, show: impl FnMut(Request, Reply) + 'static) { + self.show = Show::Custom(Box::new(show)); + } + + /// Moves the open dialog's answer, once it is in, to the answers. + fn poll(&mut self) { + let Some(open) = &self.open else { return }; + let answer = match open.answer.try_recv() { + Ok(answer) => answer, + Err(TryRecvError::Empty) => return, + Err(TryRecvError::Disconnected) => Answer::Unavailable(STOPPED.into()), + }; + if let Some(open) = self.open.take() { + self.answers.push((open.then, answer)); + } + } +} + +fn panic_text(panic: &(dyn std::any::Any + Send)) -> Option<&str> { + panic.downcast_ref::<&str>().copied().or_else(|| panic.downcast_ref::().map(String::as_str)) +} diff --git a/crates/dialogs/src/macos.rs b/crates/dialogs/src/macos.rs new file mode 100644 index 000000000..f6c938d71 --- /dev/null +++ b/crates/dialogs/src/macos.rs @@ -0,0 +1,112 @@ +//! macOS presentation. +//! +//! Open and Folder show as a free-standing panel (`-[NSOpenPanel beginWithCompletionHandler:]`), +//! as native apps show them: the panel doesn't block the window, and it stays where the user moves +//! it when the window is resized. A sheet stays centred on its window, which feels wrong for Open. +//! rfd can't show this panel: its async dialogs always begin a sheet, on the main or first window +//! when no parent is given (rfd 0.17.2 `backend/macos/modal_future.rs:71-102`). +//! +//! Save and message boxes are sheets on the parent window through rfd, as native apps show them. +//! rfd falls back to the blocking `runModal` when the app isn't running or has no window +//! (`modal_future.rs:79-118`), so [`sheet_parent`] lets a sheet start only with a parent in a +//! running app. + +use std::cell::Cell; +use std::ffi::c_void; + +use block2::RcBlock; +use objc2::MainThreadMarker; +use objc2::rc::Retained; +use objc2_app_kit::{NSApplication, NSModalResponse, NSModalResponseOK, NSOpenPanel, NSView}; +use objc2_foundation::{NSArray, NSString, NSURL}; +use raw_window_handle::{HasWindowHandle, RawWindowHandle}; + +use crate::{Answer, FileDialog, Kind, Reply}; + +const NOT_MAIN: &str = "dialogs show from the main thread only"; +const NOT_RUNNING: &str = "the app isn't running yet, so the dialog can't be shown"; + +/// The free-standing panel a [`crate::Dialogs`] has on screen. +pub(crate) type Panel = Retained; + +/// Brings `panel` to the front, so a command that would show it again shows where it is. A panel +/// that has closed, but whose answer the app hasn't collected yet, stays closed. +pub(crate) fn raise(panel: &Panel) { + if panel.isVisible() { + panel.makeKeyAndOrderFront(None); + } +} + +/// The parent a sheet can be shown on, or why there is none. +pub(crate) fn sheet_parent(parent: Option<&W>) -> Result<&W, String> { + let mtm = MainThreadMarker::new().ok_or(NOT_MAIN)?; + let app = NSApplication::sharedApplication(mtm); + if !app.isRunning() { + return Err(NOT_RUNNING.into()); + } + let parent = parent.ok_or("no window to show the dialog on")?; + // rfd `expect`s the parent's view to be in a window (rfd 0.17.2 `backend/macos/utils.rs:34-46`), + // and a panic aborts under `panic = "abort"`. Look for the view among the app's windows. + let view = match parent.window_handle().map(|h| h.as_raw()) { + Ok(RawWindowHandle::AppKit(h)) => h.ns_view.as_ptr().cast_const(), + _ => return Err("the window has no macOS view".into()), + }; + if !app.windows().iter().any(|window| window.contentView().is_some_and(|content| contains(&content, view))) { + return Err("the window to show the dialog on isn't on screen".into()); + } + Ok(parent) +} + +/// Whether `view` is `root` or one of its subviews. +fn contains(root: &NSView, view: *const c_void) -> bool { + std::ptr::eq((root as *const NSView).cast(), view) || root.subviews().iter().any(|child| contains(&child, view)) +} + +/// Shows an Open or Folder panel that doesn't block the window and answers through `reply` when +/// it closes. AppKit calls the completion handler on the main thread. +pub(crate) fn show_panel(d: &FileDialog, reply: Reply) -> Option { + let Some(mtm) = MainThreadMarker::new() else { + reply.send(Answer::Unavailable(NOT_MAIN.into())); + return None; + }; + // Begun before the app runs, the panel would never show or answer. + if !NSApplication::sharedApplication(mtm).isRunning() { + reply.send(Answer::Unavailable(NOT_RUNNING.into())); + return None; + } + let panel = NSOpenPanel::openPanel(mtm); + let folder = d.kind == Kind::Folder; + panel.setCanChooseFiles(!folder); + panel.setCanChooseDirectories(folder); + panel.setCanCreateDirectories(folder); + panel.setAllowsMultipleSelection(d.kind == (Kind::Open { multiple: true })); + if let Some(title) = &d.title { + // Panels have no title bar on current macOS; the message line shows above the files. + panel.setMessage(Some(&NSString::from_str(title))); + } + let extensions: Vec<_> = + d.filters.iter().flat_map(|f| &f.extensions).map(|e| e.trim_start_matches('.')).filter(|e| !e.is_empty()).map(NSString::from_str).collect(); + if !extensions.is_empty() { + // As rfd does. `allowedContentTypes` would add a dependency for UTType bindings; plain + // extensions match in any letter case. + #[allow(deprecated)] + panel.setAllowedFileTypes(Some(&NSArray::from_retained_slice(&extensions))); + } + if let Some(directory) = d.directory.as_deref().and_then(NSURL::from_directory_path) { + panel.setDirectoryURL(Some(&directory)); + } + // The handler keeps the panel alive until it closes; AppKit releases the handler after + // calling it, which ends the cycle. Never called, the reply drops and answers Unavailable. + let reply = Cell::new(Some(reply)); + let answered = panel.clone(); + let handler = RcBlock::new(move |response: NSModalResponse| { + let Some(reply) = reply.take() else { return }; + if response != NSModalResponseOK { + return reply.send(Answer::Cancelled); + } + let paths: Vec<_> = answered.URLs().to_vec().iter().filter_map(|url| url.to_file_path()).collect(); + reply.send(if paths.is_empty() { Answer::Cancelled } else { Answer::Paths(paths) }); + }); + panel.beginWithCompletionHandler(&handler); + Some(panel) +} diff --git a/crates/dialogs/src/native.rs b/crates/dialogs/src/native.rs new file mode 100644 index 000000000..04dc70841 --- /dev/null +++ b/crates/dialogs/src/native.rs @@ -0,0 +1,174 @@ +//! Native dialogs through rfd's async dialogs, with a worker thread that waits for the answer. +//! +//! How rfd 0.17.2 shows a dialog (from its source): +//! - macOS: creating the future begins the panel, so it happens here on the main thread. The async +//! path is always a sheet: on the parent's window, else the app's main or first window +//! (`backend/macos/modal_future.rs:71-75`). Without a running app and a window it falls back to +//! the blocking `runModal` (`modal_future.rs:79-118`), the crash this crate exists to avoid, so +//! a sheet starts only with a parent in a running app ([`crate::macos::sheet_parent`]). Open and +//! Folder don't go through rfd at all on macOS: see [`crate::macos`]. +//! - Windows: the dialog runs on a thread rfd starts when the worker first polls the future. +//! - Linux and the BSDs: the desktop portal (zenity as a fallback) works when the worker polls the +//! future; rfd's optional GTK3 backend runs on rfd's own GTK thread. + +use std::future::Future; +use std::pin::Pin; +use std::sync::Arc; +use std::sync::mpsc; +use std::task::{Context, Poll, Wake, Waker}; + +use raw_window_handle::{HasDisplayHandle, HasWindowHandle}; + +use crate::{Answer, FileDialog, Kind, Message, MessageButtons, MessageLevel, MessageResult, Reply, Request}; + +type Pending = Pin + Send>>; + +/// What [`crate::Dialogs`] keeps while a dialog is on screen: on macOS, the free-standing panel to +/// raise. Other platforms have none. +pub(crate) type Shown = Option; +#[cfg(target_os = "macos")] +pub(crate) use crate::macos::Panel; +#[cfg(not(target_os = "macos"))] +pub(crate) type Panel = std::convert::Infallible; + +pub(crate) fn show(request: Request, parent: Option<&W>, reply: Reply) -> Shown { + // rfd quietly drops a parent whose handles it can't get; the checks below need to know. + let parent = parent.filter(|w| w.window_handle().is_ok() && w.display_handle().is_ok()); + match request { + Request::File(dialog) => show_file(dialog, parent, reply), + Request::Message(message) => { + show_message(message, parent, reply); + None + } + } +} + +fn show_file(d: FileDialog, parent: Option<&W>, reply: Reply) -> Shown { + #[cfg(target_os = "macos")] + let parent = match d.kind { + Kind::Open { .. } | Kind::Folder => return crate::macos::show_panel(&d, reply), + Kind::Save => match crate::macos::sheet_parent(parent) { + Ok(parent) => Some(parent), + Err(why) => { + reply.send(Answer::Unavailable(why)); + return None; + } + }, + }; + let mut dialog = rfd::AsyncFileDialog::new(); + if let Some(parent) = parent { + dialog = dialog.set_parent(parent); + } + if let Some(title) = &d.title { + dialog = dialog.set_title(title); + } + for filter in &d.filters { + dialog = dialog.add_filter(&filter.name, &filter.patterns()); + } + if let Some(directory) = &d.directory { + dialog = dialog.set_directory(directory); + } + if let Some(name) = &d.file_name { + dialog = dialog.set_file_name(name); + } + // The worker first, so a dialog once shown always has somewhere to answer. + let worker = worker(reply)?; + let one = |file: Option| file.map_or(Answer::Cancelled, |f| Answer::Paths(vec![f.path().to_path_buf()])); + let pending: Pending = match d.kind { + Kind::Open { multiple: false } => { + let picked = dialog.pick_file(); + Box::pin(async move { one(picked.await) }) + } + Kind::Open { multiple: true } => { + let picked = dialog.pick_files(); + Box::pin(async move { + match picked.await { + Some(files) if !files.is_empty() => Answer::Paths(files.iter().map(|f| f.path().to_path_buf()).collect()), + _ => Answer::Cancelled, + } + }) + } + Kind::Folder => { + let picked = dialog.pick_folder(); + Box::pin(async move { one(picked.await) }) + } + Kind::Save => { + let picked = dialog.save_file(); + Box::pin(async move { one(picked.await) }) + } + }; + // The worker can't be gone: it waits for this. If it were, its reply answers Unavailable. + let _ = worker.send(pending); + None +} + +fn show_message(m: Message, parent: Option<&W>, reply: Reply) { + #[cfg(target_os = "macos")] + let parent = match crate::macos::sheet_parent(parent) { + Ok(parent) => Some(parent), + Err(why) => return reply.send(Answer::Unavailable(why)), + }; + let level = match m.level { + MessageLevel::Info => rfd::MessageLevel::Info, + MessageLevel::Warning => rfd::MessageLevel::Warning, + MessageLevel::Error => rfd::MessageLevel::Error, + }; + let buttons = match m.buttons { + MessageButtons::Ok => rfd::MessageButtons::Ok, + MessageButtons::OkCancel => rfd::MessageButtons::OkCancel, + MessageButtons::YesNo => rfd::MessageButtons::YesNo, + MessageButtons::YesNoCancel => rfd::MessageButtons::YesNoCancel, + MessageButtons::OkCustom(ok) => rfd::MessageButtons::OkCustom(ok), + MessageButtons::OkCancelCustom(ok, cancel) => rfd::MessageButtons::OkCancelCustom(ok, cancel), + MessageButtons::YesNoCancelCustom(yes, no, cancel) => rfd::MessageButtons::YesNoCancelCustom(yes, no, cancel), + }; + let mut dialog = rfd::AsyncMessageDialog::new().set_level(level).set_title(m.title).set_description(m.description).set_buttons(buttons); + if let Some(parent) = parent { + dialog = dialog.set_parent(parent); + } + let Some(worker) = worker(reply) else { return }; + let shown = dialog.show(); + let _ = worker.send(Box::pin(async move { + Answer::Message(match shown.await { + rfd::MessageDialogResult::Ok => MessageResult::Ok, + rfd::MessageDialogResult::Cancel => MessageResult::Cancel, + rfd::MessageDialogResult::Yes => MessageResult::Yes, + rfd::MessageDialogResult::No => MessageResult::No, + rfd::MessageDialogResult::Custom(label) => MessageResult::Custom(label), + }) + })); +} + +/// Starts a thread that waits for the dialog sent to it and answers through `reply`. `None` when +/// the thread couldn't start; `reply` is then dropped, which answers Unavailable. +fn worker(reply: Reply) -> Option> { + let (tx, rx) = mpsc::channel::(); + std::thread::Builder::new() + .name("craft-dialogs".into()) + .spawn(move || { + if let Ok(pending) = rx.recv() { + reply.send(block_on(pending)); + } + }) + .ok()?; + Some(tx) +} + +/// Runs `future` to completion on this thread, sleeping until it is woken. +fn block_on(future: F) -> F::Output { + struct Unpark(std::thread::Thread); + impl Wake for Unpark { + fn wake(self: Arc) { + self.0.unpark(); + } + } + let waker = Waker::from(Arc::new(Unpark(std::thread::current()))); + let mut cx = Context::from_waker(&waker); + let mut future = std::pin::pin!(future); + loop { + if let Poll::Ready(output) = future.as_mut().poll(&mut cx) { + return output; + } + std::thread::park(); + } +} diff --git a/crates/dialogs/src/tests.rs b/crates/dialogs/src/tests.rs new file mode 100644 index 000000000..7c43d60b9 --- /dev/null +++ b/crates/dialogs/src/tests.rs @@ -0,0 +1,207 @@ +use std::cell::RefCell; +use std::path::PathBuf; +use std::rc::Rc; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use raw_window_handle::{DisplayHandle, HandleError, HasDisplayHandle, HasWindowHandle, WindowHandle}; + +use crate::{Answer, Dialogs, FileDialog, Kind, Message, MessageResult, Reply, Request, STOPPED, filter}; + +/// No window: `show_pending(None::<&NoWindow>)`. +struct NoWindow; + +impl HasWindowHandle for NoWindow { + fn window_handle(&self) -> Result, HandleError> { + Err(HandleError::Unavailable) + } +} + +impl HasDisplayHandle for NoWindow { + fn display_handle(&self) -> Result, HandleError> { + Err(HandleError::Unavailable) + } +} + +const NONE: Option<&NoWindow> = None; + +/// The requests shown so far, with the replies a test answers them through. +type Shown = Rc>>; + +/// Dialogs whose requests stay open until a test answers their reply; counts wake-ups. +/// `T` is an `Rc`, which isn't `Send`: continuations stay on the main thread. +fn held() -> (Dialogs>, Shown, Arc) { + let wakes = Arc::new(AtomicUsize::new(0)); + let counter = wakes.clone(); + let mut dialogs = Dialogs::new(Arc::new(move || { + counter.fetch_add(1, Ordering::SeqCst); + })); + let shown = Rc::new(RefCell::new(Vec::new())); + let log = shown.clone(); + dialogs.set_custom(move |request, reply| log.borrow_mut().push((request, reply))); + (dialogs, shown, wakes) +} + +fn answered(dialogs: &mut Dialogs>) -> Vec<(String, Answer)> { + dialogs.take_answers().into_iter().map(|(then, answer)| (then.to_string(), answer)).collect() +} + +#[test] +fn requests_wait_in_order_behind_the_open_dialog() { + let (mut dialogs, shown, _) = held(); + dialogs.ask(FileDialog::open(), "a".into()); + dialogs.ask(FileDialog::save().file_name("b.txt"), "b".into()); + dialogs.ask_message(Message { title: "c".into(), ..Message::default() }, "c".into()); + dialogs.show_pending(NONE); + dialogs.show_pending(NONE); + assert_eq!(shown.borrow().len(), 1, "one dialog at a time"); + assert!(dialogs.is_open()); + assert_eq!(dialogs.queued(), 2); + + let (request, reply) = shown.borrow_mut().remove(0); + assert_eq!(request, Request::File(FileDialog::open())); + reply.send(Answer::Paths(vec![PathBuf::from("a.txt")])); + dialogs.show_pending(NONE); + let (request, reply) = shown.borrow_mut().remove(0); + assert!(matches!(request, Request::File(FileDialog { kind: Kind::Save, .. }))); + reply.send(Answer::Cancelled); + dialogs.show_pending(NONE); + let (request, reply) = shown.borrow_mut().remove(0); + assert!(matches!(request, Request::Message(_))); + reply.send(Answer::Message(MessageResult::Ok)); + + assert_eq!( + answered(&mut dialogs), + [ + ("a".to_owned(), Answer::Paths(vec![PathBuf::from("a.txt")])), + ("b".to_owned(), Answer::Cancelled), + ("c".to_owned(), Answer::Message(MessageResult::Ok)), + ] + ); + assert!(!dialogs.is_open()); + assert!(dialogs.take_answers().is_empty()); +} + +#[test] +fn a_cancelled_dialog_answers_cancelled_and_closes() { + let (mut dialogs, shown, wakes) = held(); + dialogs.ask(FileDialog::folder(), "f".into()); + dialogs.show_pending(NONE); + let before = wakes.load(Ordering::SeqCst); + let (_, reply) = shown.borrow_mut().remove(0); + reply.send(Answer::Cancelled); + assert!(wakes.load(Ordering::SeqCst) > before, "the answer wakes the app"); + assert_eq!(answered(&mut dialogs), [("f".to_owned(), Answer::Cancelled)]); + assert!(!dialogs.is_open()); +} + +#[test] +fn the_override_answers_every_request_on_a_later_take() { + let mut dialogs: Dialogs = Dialogs::new(Arc::new(|| {})); + let seen = Rc::new(RefCell::new(Vec::new())); + let log = seen.clone(); + dialogs.set_override(Some(Box::new(move |request: &Request| { + log.borrow_mut().push(request.clone()); + match request { + Request::File(FileDialog { kind: Kind::Open { .. }, .. }) => Answer::Paths(vec![PathBuf::from("in.pdf")]), + Request::File(_) => Answer::Cancelled, + Request::Message(_) => Answer::Message(MessageResult::Yes), + } + }))); + dialogs.ask(FileDialog::open_many(), 1); + dialogs.ask(FileDialog::save(), 2); + dialogs.ask_message(Message::default(), 3); + assert!(dialogs.take_answers().is_empty(), "nothing answers before show_pending"); + dialogs.show_pending(NONE); + assert_eq!(seen.borrow().len(), 3); + assert_eq!( + dialogs.take_answers(), + [(1, Answer::Paths(vec![PathBuf::from("in.pdf")])), (2, Answer::Cancelled), (3, Answer::Message(MessageResult::Yes))] + ); + assert!(!dialogs.is_open()); +} + +#[test] +fn a_panicking_override_answers_unavailable() { + let mut dialogs: Dialogs = Dialogs::new(Arc::new(|| {})); + dialogs.set_override(Some(Box::new(|_: &Request| panic!("test override")))); + dialogs.ask(FileDialog::open(), 1); + dialogs.show_pending(NONE); + let answers = dialogs.take_answers(); + assert!(matches!(answers.as_slice(), [(1, Answer::Unavailable(why))] if why.contains("test override")), "{answers:?}"); + assert!(!dialogs.is_open()); +} + +#[test] +fn a_worker_that_ends_without_answering_closes_the_dialog() { + let (mut dialogs, shown, wakes) = held(); + dialogs.ask(FileDialog::open(), "dropped".into()); + dialogs.ask(FileDialog::open(), "panicked".into()); + dialogs.ask(FileDialog::open(), "next".into()); + dialogs.show_pending(NONE); + + let (_, reply) = shown.borrow_mut().remove(0); + let before = wakes.load(Ordering::SeqCst); + drop(reply); + assert!(wakes.load(Ordering::SeqCst) > before, "the drop wakes the app"); + dialogs.show_pending(NONE); + + // A worker thread that panics while it holds the reply. + let (_, reply) = shown.borrow_mut().remove(0); + let worker = std::thread::spawn(move || { + let _reply = reply; + std::panic::resume_unwind(Box::new("rfd panicked")); + }); + assert!(worker.join().is_err()); + + assert_eq!( + answered(&mut dialogs), + [("dropped".to_owned(), Answer::Unavailable(STOPPED.into())), ("panicked".to_owned(), Answer::Unavailable(STOPPED.into()))] + ); + assert!(!dialogs.is_open()); + dialogs.show_pending(NONE); + assert_eq!(shown.borrow().len(), 1, "the next request shows"); +} + +#[test] +fn a_panic_while_showing_answers_unavailable_and_moves_on() { + let mut dialogs: Dialogs = Dialogs::new(Arc::new(|| {})); + let shown = Rc::new(RefCell::new(0)); + let count = shown.clone(); + dialogs.set_custom(move |_, reply| { + *count.borrow_mut() += 1; + if *count.borrow() == 1 { + std::panic::resume_unwind(Box::new("no window")); + } + reply.send(Answer::Cancelled); + }); + dialogs.ask(FileDialog::open(), 1); + dialogs.ask(FileDialog::open(), 2); + dialogs.show_pending(NONE); + assert_eq!(dialogs.take_answers(), [(1, Answer::Unavailable("the dialog couldn't be shown: no window".into())), (2, Answer::Cancelled)]); +} + +#[test] +fn linux_globs_match_every_letter_case() { + // Matches `name` against a glob of literals and `[..]` classes, as the portal does after `*.`. + fn matches(glob: &str, name: &str) -> bool { + let mut name = name.chars(); + let mut glob = glob.chars(); + while let Some(g) = glob.next() { + let class: String = if g == '[' { glob.by_ref().take_while(|&c| c != ']').collect() } else { g.to_string() }; + if !name.next().is_some_and(|n| class.contains(n)) { + return false; + } + } + name.next().is_none() + } + let exts = |e: &[&str]| e.iter().map(|s| (*s).to_owned()).collect::>(); + assert_eq!(filter::patterns(&exts(&["NEF", ".Jpg", "jpg"]), false), ["nef", "jpg"]); + assert_eq!(filter::patterns(&exts(&["nef"]), true), ["nef", "NEF", "[nN][eE][fF]"]); + assert_eq!(filter::patterns(&exts(&["mp4", "MP4"]), true), ["mp4", "MP4", "[mM][pP]4"]); + let globs = filter::patterns(&exts(&["jpeg"]), true); + for name in ["jpeg", "JPEG", "JpEg", "jPeG"] { + assert!(globs.iter().any(|g| matches(g, name)), "{name}"); + } + assert!(!globs.iter().any(|g| matches(g, "jpg") || matches(g, "jpegx"))); +} From 7fd60c58706d2ed7db8add8747fbb95668fac3de Mon Sep 17 00:00:00 2001 From: Jeremy Date: Thu, 29 Oct 2026 13:29:47 +0000 Subject: [PATCH 12/13] craft-update: a build from source is never offered the release it already contains Reported by FredApps: PhotoCraft built from main as 0.17.0-integration.20261029.f5b3f123 offered "Update and Restart" to 0.17.0, a downgrade. Semver puts a pre-release before its release, and the update check and the self-update guard compared with plain semver. New version::offers_update: a pre-release label the suite publishes (alpha, beta, rc, nightly) sorts as before; any other label marks a build from source, which is only offered a strictly higher X.Y.Z. check_for_update (through a new testable offered_release) and the self-update guard both use it. Co-Authored-By: Claude Opus 5.5 --- crates/update/src/lib.rs | 49 +++++++++++++++++++++++------- crates/update/src/self_update.rs | 20 ++++++++++++- crates/update/src/version.rs | 51 ++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 12 deletions(-) diff --git a/crates/update/src/lib.rs b/crates/update/src/lib.rs index d0fec4961..d886a0629 100644 --- a/crates/update/src/lib.rs +++ b/crates/update/src/lib.rs @@ -50,17 +50,7 @@ pub use dialog::{StartupPrompt, StartupPromptResponse}; pub fn check_for_update(app: &App, http: &Http, current_version: &str) -> Result, String> { let url = format!("https://api.github.com/repos/{}/releases?per_page=30", app.repo); match http.get_api(&url, None)? { - crate::http::Fetched::Body { text, .. } => { - let latest = crate::release::parse_github_release_list(&text, app.repo, false)?; - if let Some(release) = latest - && let Some(rel_v) = &release.version - && let Some(cur_v) = crate::version::parse(current_version) - && crate::version::is_newer(rel_v, &cur_v) - { - return Ok(Some(release)); - } - Ok(None) - } + crate::http::Fetched::Body { text, .. } => offered_release(&text, app.repo, current_version), crate::http::Fetched::RateLimited { .. } => Err("GitHub's request limit was reached".into()), crate::http::Fetched::NotModified | crate::http::Fetched::NotFound => Ok(None), } @@ -71,5 +61,42 @@ pub fn now() -> i64 { std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map_or(0, |d| i64::try_from(d.as_secs()).unwrap_or(i64::MAX)) } +/// The release in GitHub's release list `json` to offer a copy running `current_version`, if any: +/// the latest published release, when [`version::offers_update`] says so. +fn offered_release(json: &str, repo: &str, current_version: &str) -> Result, String> { + let latest = crate::release::parse_github_release_list(json, repo, false)?; + Ok(latest.filter(|release| { + let running = crate::version::parse(current_version); + release.version.as_ref().zip(running.as_ref()).is_some_and(|(rel_v, cur_v)| crate::version::offers_update(rel_v, cur_v)) + })) +} + #[cfg(test)] pub(crate) const TEST_AGENT: &str = "ArtCraft-Launcher-Test/0.0"; + +#[cfg(test)] +mod offer_tests { + use super::offered_release; + + const REPO: &str = "storytold/photocraft"; + const LIST: &str = r#"[{"tag_name":"v0.17.0","prerelease":false,"draft":false,"assets":[]}, + {"tag_name":"v0.16.4","prerelease":false,"draft":false,"assets":[]}]"#; + + fn offered(running: &str) -> Option { + offered_release(LIST, REPO, running).unwrap().map(|release| release.tag) + } + + #[test] + fn a_build_from_main_is_not_offered_the_release_it_already_contains() { + assert_eq!(offered("0.17.0-integration.20261029.f5b3f123"), None); + assert_eq!(offered("0.16.5-dev"), Some("v0.17.0".into())); + } + + #[test] + fn releases_and_release_candidates_are_offered_as_before() { + assert_eq!(offered("0.17.0-rc.2"), Some("v0.17.0".into())); + assert_eq!(offered("0.16.4"), Some("v0.17.0".into())); + assert_eq!(offered("0.17.0"), None); + assert_eq!(offered("not a version"), None); + } +} diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index f22dfb35a..1793a5a51 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -305,8 +305,9 @@ pub fn update( ) -> Result<(), String> { let cur_v = running_version.and_then(|v| current_version(app, v)); if let (Some(rel_v), Some(cur_v)) = (release.version.as_ref(), cur_v.as_ref()) - && !version::is_newer(rel_v, cur_v) + && !version::offers_update(rel_v, cur_v) { + // A build from source is only replaced by a strictly higher release (see `offers_update`). return Err(format!("{} {} is not newer than currently running version {}", app.name, release.version_label(), cur_v)); } let (asset, _) = pick_asset(release, &[app.id], platform, &[install.kind()]) @@ -1738,6 +1739,23 @@ mod tests { assert!(result.unwrap_err().contains("is not newer than currently running version")); } + #[test] + fn a_build_from_main_refuses_to_install_the_release_it_already_contains() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let http = Http::new(crate::TEST_AGENT).unwrap(); + let mut phase = |_| (); + let last = || Ok(()); + let stop = Stop::default(); + let release = Release { version: version::parse("0.2.0"), tag: "v0.2.0".into(), ..Release::default() }; + let install = Install::AppImage(dir.path().join("launcher.AppImage")); + use crate::{Arch, Os}; + let platform = Platform { os: Os::Linux, arch: Arch::X64 }; + let running = Some("0.2.0-integration.20261029.f5b3f123"); + let result = update(&crate::suite::LAUNCHER, &install, &release, platform, &http, &paths, &stop, &mut phase, &last, running); + assert!(result.unwrap_err().contains("is not newer than currently running version")); + } + #[cfg(target_os = "linux")] #[test] fn parses_flatpak_metadata_for_restart() { diff --git a/crates/update/src/version.rs b/crates/update/src/version.rs index 0beceef15..1915f7b37 100644 --- a/crates/update/src/version.rs +++ b/crates/update/src/version.rs @@ -112,6 +112,28 @@ pub fn is_newer(latest: &Version, installed: &Version) -> bool { compare(latest, installed) == Ordering::Greater } +/// The pre-release labels the suite publishes, which sort against releases as semver says +/// (`0.17.0-rc.2` is before `0.17.0`). +const RELEASE_CHANNELS: [&str; 4] = ["alpha", "beta", "rc", "nightly"]; + +/// Whether `v` is a build made from source rather than a published release: it has a pre-release +/// label the suite never publishes (`0.17.0-integration.20261029.f5b3f123`, `0.18.0-dev`). +pub fn is_local_build(v: &Version) -> bool { + let label = v.pre.as_str().split('.').next().unwrap_or_default(); + !v.pre.is_empty() && !RELEASE_CHANNELS.iter().any(|channel| label.eq_ignore_ascii_case(channel)) +} + +/// Whether a copy running `running` should be offered `latest`. As [`is_newer`], except for a +/// local build: semver puts `0.17.0-integration…` before `0.17.0`, but a build from `main` already +/// holds 0.17.0's changes and more, so replacing it with 0.17.0 is a downgrade. A local build is +/// only offered a strictly higher `X.Y.Z`. +pub fn offers_update(latest: &Version, running: &Version) -> bool { + if is_local_build(running) { + return (latest.major, latest.minor, latest.patch) > (running.major, running.minor, running.patch); + } + is_newer(latest, running) +} + /// Platforms that only record `X.Y.Z` (the MSI ProductVersion, `CFBundleShortVersionString`) /// drop a pre-release suffix. A numeric-only installed version therefore counts as current when it /// matches a pre-release's numbers, rather than nagging forever. @@ -133,6 +155,35 @@ mod tests { parse(s).unwrap() } + #[test] + fn a_build_from_main_is_never_offered_its_own_release() { + let running = v("0.17.0-integration.20261029.f5b3f123"); + assert!(is_local_build(&running)); + assert!(!offers_update(&v("0.17.0"), &running)); + assert!(!offers_update(&v("0.16.4"), &running)); + assert!(!offers_update(&v("0.17.0-rc.3"), &running)); + assert!(offers_update(&v("0.17.1"), &running)); + assert!(offers_update(&v("0.18.0"), &running)); + assert!(offers_update(&v("1.0.0"), &v("0.18.0-dev"))); + assert!(!offers_update(&v("0.18.0"), &v("0.18.0-DEV.4"))); + } + + #[test] + fn published_pre_releases_and_releases_order_as_before() { + for label in ["0.17.0-rc.2", "0.17.0-beta.1", "0.17.0-alpha", "0.17.0-nightly.20261029", "0.17.0-RC.1"] { + let running = v(label); + assert!(!is_local_build(&running), "{label}"); + assert!(offers_update(&v("0.17.0"), &running), "{label}"); + } + assert!(offers_update(&v("0.17.0-rc.3"), &v("0.17.0-rc.2"))); + assert!(!offers_update(&v("0.17.0-rc.2"), &v("0.17.0-rc.2"))); + assert!(!is_local_build(&v("0.17.0"))); + assert!(offers_update(&v("0.17.1"), &v("0.17.0"))); + assert!(!offers_update(&v("0.17.0"), &v("0.17.0"))); + assert!(!offers_update(&v("0.16.9"), &v("0.17.0"))); + assert!(!offers_update(&v("0.17.0+build.5"), &v("0.17.0"))); + } + #[test] fn reads_tags_and_platform_spellings() { assert_eq!(v("v0.5.0"), Version::new(0, 5, 0)); From df399edff8afedffac40a2115ba108f4466923e9 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Thu, 29 Oct 2026 15:26:20 +0000 Subject: [PATCH 13/13] craft-update: a Windows MSI copy updates itself in its own context, wherever it is installed (#419) * craft-update: a Windows MSI copy updates itself in its own context, wherever it is installed Self-update only recognised an MSI copy by the launcher's per-user folder (the HKCU InstallDir key, or %LOCALAPPDATA%\Programs\). Every app installed for everyone, in Program Files\ArtCraft\ or the older Program Files\, was treated as a loose folder and updated by unpacking the portable zip over Program Files without permission. - msi_context moves here from the launcher (craft-launcher crates/native/src/msi_context.rs at b827fed, unchanged apart from visibility, MsiScope and scope_arguments). Windows Installer's own registration decides a copy's context, for the launcher and the apps alike, so the launcher can drop its copy and both routes follow one rule. - New msi_copy module: a copy is an MSI copy when an App Paths entry names its exe (a registry read, cheap enough for status checks). Its update asks Windows Installer for the context; the App Paths hive is only the fallback when that lookup cannot run. - The update stops, before the app exits, when the copy is registered in both hives, the lookup is unclear, or Windows lists the app more than once. - After the update the helper starts the exe the registrations name now, since the package may have moved an exact old default folder. - A failed self-update now names the Windows Installer log, as the launcher's installs do. - The launcher's own per-user package is recognised and updated exactly as before. Co-Authored-By: Claude Opus 5.5 * craft-update: a copy only takes the Windows Installer context of its own hive Review follow-up (CodeRabbit on #419). The product code for the context lookup came from the app's only uninstall record, which was not proven to own the running exe. A package writes App Paths under HKMU, always the hive of its own context, so a registration whose Windows Installer context differs from the hive that named this exe belongs to another copy. The update now stops then, instead of taking the other copy's context. Also documents the remaining helpers the diff touches. Co-Authored-By: Claude Opus 5.5 * craft-update: an app in the old per-user folder is judged by its registration first Review follow-up (CodeRabbit on #419). The folder rule for the launcher's per-user package also matched an app installed in %LOCALAPPDATA%\Programs\, and it ran before the App Paths check, so such a copy skipped the product-count, conflict and context checks. Now an app is judged by its registration first; the folder rule only covers an app copy no registration names. The launcher in its own folder still keeps its old path whatever is registered: on a real PC it has an App Paths entry and a stale second uninstall record, and registration-first would have stopped its self-update. Co-Authored-By: Claude Opus 5.5 * craft-update: when the Windows Installer lookup cannot run, the uninstall records decide Review follow-up (CodeRabbit on #419). The fallback trusted the App Paths hive, so a stale entry or a product registered in both scopes could run the update in the wrong context. The fallback now follows the launcher's rule: the context every uninstall record of the product agrees on (msi_context::single_scope over HKLM in both views and HKCU). Missing or mixed records stop the update, and an agreed context that differs from the App Paths hive still stops it as another copy's registration. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- crates/update/src/install.rs | 43 +++- crates/update/src/lib.rs | 2 + crates/update/src/msi_context.ps1 | 23 ++ crates/update/src/msi_context.rs | 288 +++++++++++++++++++++++++ crates/update/src/msi_copy.rs | 338 ++++++++++++++++++++++++++++++ crates/update/src/self_update.rs | 46 ++-- 6 files changed, 717 insertions(+), 23 deletions(-) create mode 100644 crates/update/src/msi_context.ps1 create mode 100644 crates/update/src/msi_context.rs create mode 100644 crates/update/src/msi_copy.rs diff --git a/crates/update/src/install.rs b/crates/update/src/install.rs index 09de4e0c2..7c9e773ee 100644 --- a/crates/update/src/install.rs +++ b/crates/update/src/install.rs @@ -1645,6 +1645,15 @@ fn msi_command(program: &Path, msi: &Path) -> Result Result { + let mut command = msi_command(program, msi)?; + command.args(context); + Ok(command) +} + /// Where `msi_command` asks Windows Installer to write its log. #[cfg_attr(not(windows), allow(dead_code))] fn msi_log(msi: &Path) -> PathBuf { @@ -1667,7 +1676,9 @@ pub(crate) fn msi_path(msi: &Path) -> Result { pub mod windows { use crate::protocol::CANCELLED; - use super::{App, INSTALLER, JobPhase, Path, PathBuf, Stop, cancelled, msi_command, msi_folder_argument, msi_log, process, turn}; + use super::{ + App, INSTALLER, JobPhase, Path, PathBuf, Stop, cancelled, msi_command, msi_folder_argument, msi_log, process, reinstall_command, turn, + }; fn msiexec() -> PathBuf { std::env::var_os("SystemRoot").map_or_else(|| PathBuf::from(r"C:\Windows"), PathBuf::from).join(r"System32\msiexec.exe") @@ -1700,11 +1711,15 @@ pub mod windows { }) } - /// Runs the launcher's own new MSI for a self-update, with the same progress window and no - /// restart as [`run_msi`], and waits for it. - pub fn reinstall(msi: &Path) -> Result<(), String> { - let status = msi_command(&msiexec(), msi)?.status().map_err(|e| format!("couldn't start the Windows Installer ({e})"))?; - explain(status.code()) + /// Runs an app's own new MSI for a self-update, with the same progress window and no restart + /// as [`run_msi`], and waits for it. `context` keeps the installed copy's Windows Installer + /// context. A failure names the installer's log, which is the only place 1603 is explained. + pub fn reinstall(msi: &Path, context: &[&str]) -> Result<(), String> { + let status = reinstall_command(&msiexec(), msi, context)?.status().map_err(|e| format!("couldn't start the Windows Installer ({e})"))?; + explain(status.code()).map_err(|e| match status.code() { + Some(1602) | None => e, + _ => format!("{e}. Its log is at {}", msi_log(msi).display()), + }) } /// Runs `work` once no other Windows Installer job of ours is running (a second one would @@ -1760,6 +1775,22 @@ mod tests { ); } + #[test] + fn a_self_update_keeps_the_installed_copys_context_after_the_package_options() { + let dir = tempfile::tempdir().unwrap(); + let file = dir.path().join("GridCraft.msi"); + std::fs::write(&file, b"test package").unwrap(); + let resolved = msi_path(&file).unwrap(); + for context in [&["ALLUSERS=1"][..], &["ALLUSERS=2", "MSIINSTALLPERUSER=1"], &[]] { + let command = reinstall_command(Path::new("msiexec.exe"), &file, context).unwrap(); + let args: Vec<_> = command.get_args().map(|a| a.to_string_lossy().into_owned()).collect(); + let mut expected: Vec = ["/i", &resolved.to_string_lossy(), "/passive", "/norestart", "/l*v"].map(String::from).to_vec(); + expected.push(resolved.with_extension("log").to_string_lossy().into_owned()); + expected.extend(context.iter().map(|p| p.to_string())); + assert_eq!(args, expected); + } + } + #[test] fn an_unavailable_msi_is_refused_before_starting_the_installer() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/update/src/lib.rs b/crates/update/src/lib.rs index d0fec4961..58bb0e946 100644 --- a/crates/update/src/lib.rs +++ b/crates/update/src/lib.rs @@ -10,6 +10,8 @@ mod folders; pub mod http; pub mod install; pub mod manifest; +pub mod msi_context; +mod msi_copy; pub mod packages; pub mod paths; pub mod platform; diff --git a/crates/update/src/msi_context.ps1 b/crates/update/src/msi_context.ps1 new file mode 100644 index 000000000..d0e0e0ebc --- /dev/null +++ b/crates/update/src/msi_context.ps1 @@ -0,0 +1,23 @@ +# Read-only MSI registration, limited to the current account and machine contexts. +# $code is supplied by the caller only after strict braced-GUID validation. +$ErrorActionPreference = 'Stop' +$installer = $null +$products = $null +try { + $installer = New-Object -ComObject WindowsInstaller.Installer + $products = $installer.ProductsEx($code, '', 7) + foreach ($product in $products) { + try { + $context = $product.GetType().InvokeMember('Context', 'GetProperty', $null, $product, $null) + Write-Output ([int]$context) + } finally { + [void][Runtime.InteropServices.Marshal]::FinalReleaseComObject($product) + } + } +} finally { + foreach ($com in @($products, $installer)) { + if ($null -ne $com) { + [void][Runtime.InteropServices.Marshal]::FinalReleaseComObject($com) + } + } +} diff --git a/crates/update/src/msi_context.rs b/crates/update/src/msi_context.rs new file mode 100644 index 000000000..b51c2eedd --- /dev/null +++ b/crates/update/src/msi_context.rs @@ -0,0 +1,288 @@ +//! The Windows Installer context of an installed copy, from Windows Installer's own registration, +//! never inferred from a display registry entry. An existing copy must never fall back to a fresh +//! installation when discovery fails. +//! +//! Shared by the launcher's installs and the apps' self-updates, so both keep a copy in the same +//! context by the same rule. + +#![cfg_attr(not(windows), allow(dead_code))] + +/// The Windows Installer context a copy is installed in. A per-machine copy elevates and writes +/// HKLM registrations; a per-user one must not silently become a second, per-machine copy (or the +/// other way round). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum MsiScope { + PerUser, + PerMachine, +} + +/// The `msiexec` properties that ask a dual-scope package for `scope`. `ALLUSERS=1` installs for +/// everyone (Windows asks for permission); `ALLUSERS=2` with `MSIINSTALLPERUSER=1` is the documented +/// single-package way to ask for a per-user install. +pub fn scope_arguments(scope: MsiScope) -> &'static [&'static str] { + match scope { + MsiScope::PerUser => &["ALLUSERS=2", "MSIINSTALLPERUSER=1"], + MsiScope::PerMachine => &["ALLUSERS=1"], + } +} + +const UNKNOWN_CONTEXT: &str = "Windows could not confirm who this app is installed for. No changes were made. Try the app's installer instead."; +const CONFLICTING_CONTEXT: &str = "This app is installed for both your account and everyone. Use its installer to choose which copy to update."; + +/// Why the Windows Installer lookup gave no context. +#[derive(Debug, PartialEq, Eq)] +pub enum ProbeError { + /// The lookup ran but gave no usable answer. The context is never guessed from that. + Unknown, + /// The same product is registered for this account and for everyone. + Conflicting, + /// The lookup could not run (a blocked shell, a timeout, a slow first start). Carries the real + /// error so it can be logged. + Unavailable(String), +} + +/// The context an update must keep. Windows Installer's own registration decides. Only when that +/// lookup cannot run at all does the context come from the uninstall record the copy was found by +/// (`record_scope`): the machine's records are per-machine and the account's own are per-user. A +/// lookup that ran and gave no answer, or a copy registered twice, still stops the update. +pub fn update_scope( + is_msi: bool, + product_code: Option<&str>, + probe: impl FnOnce(&str) -> Result, + record_scope: impl FnOnce(&str) -> Option, +) -> Result, String> { + if !is_msi { + return Ok(None); + } + let code = product_code.filter(|code| valid_product_code(code)).ok_or(UNKNOWN_CONTEXT)?; + match probe(code) { + Ok(scope) => Ok(Some(scope)), + Err(ProbeError::Conflicting) => Err(CONFLICTING_CONTEXT.into()), + Err(ProbeError::Unknown) => Err(UNKNOWN_CONTEXT.into()), + Err(ProbeError::Unavailable(reason)) => { + log::warn!("Windows Installer context lookup for {code} could not run: {reason}"); + let scope = record_scope(code).ok_or(UNKNOWN_CONTEXT)?; + log::info!("{code}: keeping the {scope:?} context of its uninstall record"); + Ok(Some(scope)) + } + } +} + +/// The one context every uninstall record of a product agrees on. Records that disagree (the copy is +/// registered for this account and for everyone) or no records at all give `None`, so the update stops. +pub fn single_scope(scopes: impl IntoIterator) -> Option { + let mut scopes = scopes.into_iter(); + let first = scopes.next()?; + scopes.all(|scope| scope == first).then_some(first) +} + +/// The context to install in. An update keeps `updated`, the context of the copy it replaces. A fresh +/// install (no copy at all) is machine-wide, so every app lands in `C:\Program Files\ArtCraft\` +/// (or the folder the person chose) and not in one account's profile; a dual-scope package would +/// otherwise take its own default, which is per-user. Windows asks for permission once, for that +/// install. Anything else (a copy that is not an MSI) takes the package's own default. +pub fn install_scope(updated: Option, fresh: bool) -> Option { + updated.or_else(|| fresh.then_some(MsiScope::PerMachine)) +} + +/// Whether `code` is a braced Windows Installer product GUID, the only text ever placed in the +/// lookup script. +pub fn valid_product_code(code: &str) -> bool { + code.len() == 38 + && code.bytes().enumerate().all(|(index, byte)| match index { + 0 => byte == b'{', + 37 => byte == b'}', + 9 | 14 | 19 | 24 => byte == b'-', + _ => byte.is_ascii_hexdigit(), + }) +} + +/// The one context in the lookup's output (one `MSIINSTALLCONTEXT` number per line): 1 and 2 are +/// an account (managed or not), 4 the machine. Anything else is unclear, and two contexts conflict. +fn parse_contexts(output: &str) -> Result { + let mut scope = None; + for line in output.lines().map(str::trim).filter(|line| !line.is_empty()) { + let context = match line { + "1" | "2" => MsiScope::PerUser, + "4" => MsiScope::PerMachine, + _ => return Err(ProbeError::Unknown), + }; + if scope.is_some_and(|previous| previous != context) { + return Err(ProbeError::Conflicting); + } + scope = Some(context); + } + scope.ok_or(ProbeError::Unknown) +} + +/// Asks Windows Installer, read-only, which context `product_code` is installed in, for this +/// account and the machine. Runs PowerShell with a 10 second limit. +#[cfg(windows)] +pub fn probe(product_code: &str) -> Result { + use std::path::PathBuf; + use std::time::Duration; + + if !valid_product_code(product_code) { + return Err(ProbeError::Unknown); + } + let root = std::env::var_os("SystemRoot") + .map(PathBuf::from) + .filter(|root| root.is_absolute()) + .ok_or_else(|| ProbeError::Unavailable("SystemRoot is not set to an absolute path".into()))?; + let powershell = root.join(r"System32\WindowsPowerShell\v1.0\powershell.exe"); + // Only a validated GUID is interpolated. ProductsEx reads registration for the current + // account and the machine, never another account and never Win32_Product (which repairs). + let script = format!("$code='{product_code}';\n{}", include_str!("msi_context.ps1")); + let mut command = crate::process::command(powershell); + command.args(["-NoProfile", "-NonInteractive", "-Command", &script]); + let output = crate::process::run_command(command, "Windows installation lookup", Duration::from_secs(10)).map_err(ProbeError::Unavailable)?; + parse_contexts(&output) +} + +#[cfg(test)] +mod tests { + use super::*; + + const CODE: &str = "{12345678-1234-1234-1234-123456789ABC}"; + + #[test] + fn authoritative_machine_context_is_preserved() { + assert_eq!(parse_contexts("4\r\n"), Ok(MsiScope::PerMachine)); + } + + #[test] + fn managed_and_unmanaged_accounts_are_both_per_user() { + assert_eq!(parse_contexts("1\n"), Ok(MsiScope::PerUser)); + assert_eq!(parse_contexts("2\n"), Ok(MsiScope::PerUser)); + assert_eq!(parse_contexts("1\n2\n"), Ok(MsiScope::PerUser)); + } + + #[test] + fn missing_unknown_or_conflicting_contexts_are_not_guessed() { + for output in ["", " \r\n", "0", "7", "user", "2\n4", "4\n1", "4\nnoise"] { + assert!(parse_contexts(output).is_err(), "{output:?}"); + } + assert_eq!(parse_contexts("4\n1"), Err(ProbeError::Conflicting)); + assert_eq!(parse_contexts("7"), Err(ProbeError::Unknown)); + } + + #[test] + fn a_fresh_install_is_not_looked_up() { + assert_eq!(update_scope(false, None, |_| Err(ProbeError::Unknown), |_| None), Ok(None)); + } + + #[test] + fn known_updates_keep_the_authoritative_context() { + for scope in [MsiScope::PerUser, MsiScope::PerMachine] { + assert_eq!(update_scope(true, Some(CODE), |_| Ok(scope), |_| None), Ok(Some(scope))); + } + } + + #[test] + fn an_update_without_its_product_code_stops_before_probing() { + let called = std::cell::Cell::new(false); + assert!( + update_scope( + true, + None, + |_| { + called.set(true); + Ok(MsiScope::PerMachine) + }, + |_| None + ) + .is_err() + ); + assert!(!called.get()); + } + + #[test] + fn malformed_product_codes_stop_before_any_shell_call() { + for code in ["", "not-a-guid", "{12345678-1234-1234-1234-123456789ABZ}", "';exit 0;#", "{12345678-1234-1234-1234-123456789ABC}\n"] { + let called = std::cell::Cell::new(false); + assert!( + update_scope( + true, + Some(code), + |_| { + called.set(true); + Ok(MsiScope::PerMachine) + }, + |_| None + ) + .is_err() + ); + assert!(!called.get(), "{code:?}"); + } + } + + #[test] + fn unavailable_context_never_becomes_a_fresh_install() { + let unavailable = || Err(ProbeError::Unavailable("query failed".into())); + assert!(update_scope(true, Some(CODE), |_| unavailable(), |_| None).is_err()); + } + + #[test] + fn a_lookup_that_cannot_run_falls_back_to_the_uninstall_record() { + for scope in [MsiScope::PerUser, MsiScope::PerMachine] { + let seen = std::cell::RefCell::new(String::new()); + let result = update_scope( + true, + Some(CODE), + |_| Err(ProbeError::Unavailable("PowerShell is blocked".into())), + |code| { + *seen.borrow_mut() = code.to_owned(); + Some(scope) + }, + ); + assert_eq!(result, Ok(Some(scope))); + assert_eq!(*seen.borrow(), CODE); + } + } + + #[test] + fn an_answer_that_is_unclear_or_doubled_never_uses_the_uninstall_record() { + for failure in [ProbeError::Unknown, ProbeError::Conflicting] { + let result = update_scope(true, Some(CODE), |_| Err(failure), |_| panic!("must not consult the record")); + assert!(result.is_err()); + } + } + + #[test] + fn a_blocked_lookup_without_a_record_stops_the_update() { + let result = update_scope(true, Some(CODE), |_| Err(ProbeError::Unavailable("timed out".into())), |_| None); + assert_eq!(result, Err(UNKNOWN_CONTEXT.to_string())); + } + + #[test] + fn records_in_two_contexts_give_no_fallback_scope() { + assert_eq!(single_scope([]), None); + assert_eq!(single_scope([MsiScope::PerUser]), Some(MsiScope::PerUser)); + assert_eq!(single_scope([MsiScope::PerMachine, MsiScope::PerMachine]), Some(MsiScope::PerMachine)); + assert_eq!(single_scope([MsiScope::PerMachine, MsiScope::PerUser]), None); + assert_eq!(single_scope([MsiScope::PerUser, MsiScope::PerMachine, MsiScope::PerMachine]), None); + } + + #[test] + fn each_context_asks_the_package_for_itself() { + assert_eq!(scope_arguments(MsiScope::PerMachine), ["ALLUSERS=1"]); + assert_eq!(scope_arguments(MsiScope::PerUser), ["ALLUSERS=2", "MSIINSTALLPERUSER=1"]); + } + + #[test] + fn a_fresh_install_is_machine_wide() { + assert_eq!(install_scope(None, true), Some(MsiScope::PerMachine)); + } + + #[test] + fn an_update_keeps_the_context_of_the_copy_it_replaces() { + for scope in [MsiScope::PerUser, MsiScope::PerMachine] { + assert_eq!(install_scope(Some(scope), false), Some(scope)); + } + } + + #[test] + fn a_copy_that_is_not_an_msi_takes_the_package_default() { + assert_eq!(install_scope(None, false), None); + } +} diff --git a/crates/update/src/msi_copy.rs b/crates/update/src/msi_copy.rs new file mode 100644 index 000000000..05e1ca3ff --- /dev/null +++ b/crates/update/src/msi_copy.rs @@ -0,0 +1,338 @@ +//! Whether a running Windows copy was installed by its MSI, and the `msiexec` properties its +//! self-update needs to keep that copy's context. +//! +//! Every Craft App MSI registers its exe under `App Paths` (HKLM when installed for everyone, +//! HKCU for one account). A copy whose exe one of those entries names is an MSI copy, wherever its +//! folder is: `Program Files\ArtCraft\`, the older `Program Files\`, a folder the person +//! chose. Recognising it only reads the registry, so it is cheap enough for any status check. +//! +//! Which context to update in is a separate, slower question that only an update asks. Windows +//! Installer's own registration answers it ([`crate::msi_context`]), as it does for the launcher; +//! only when that lookup cannot run at all do the product's own uninstall records answer, and +//! only when they all agree. The answer must +//! match the hive that named this exe: a package writes `App Paths` under `HKMU`, which is always +//! the hive of its own context, so a registration in the other context belongs to another copy and +//! the update stops. So a copy installed for everyone is never unpacked over without permission, +//! and a per-user copy never gains a second, per-machine twin or takes another copy's context. +//! +//! The launcher's own per-user MSI is still recognised by the folder it records in HKCU (or its +//! default folder) and updated exactly as before, with the package's own default context. An app +//! copy in that kind of folder is judged by its registration first ([`classify`]). + +#![cfg_attr(not(windows), allow(dead_code))] + +use std::path::{Path, PathBuf}; + +use crate::msi_context::MsiScope; + +/// How an MSI copy was recognised. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum MsiCopy { + /// An `App Paths` entry in the hive of `hive` names this exe. + Registered { hive: MsiScope }, + /// Entries in both hives name it: the update stops rather than guess. + Conflicting, + /// The launcher's own per-user package, found by its recorded or default folder. + Legacy, +} + +const CONFLICTING: &str = + "this app is registered both for your account and for everyone, so it can't tell which copy to update. Use the app's installer instead."; +const ANOTHER_COPY: &str = "Windows lists this app for a different account setting than this copy, so it can't be sure which copy it would update. Use the app's installer instead."; +const NOT_ONE_PRODUCT: &str = + "Windows lists this app's installation more than once or not at all, so it can't tell which copy to update. Use the app's installer instead."; + +/// The `msiexec` properties for `copy`'s update. `product_codes` are the app's Windows Installer +/// registrations; `probe` asks Windows Installer for one product's context, and `record_scopes` +/// lists the context of every uninstall record of that product, read only when the lookup cannot +/// run. Exactly one product must be registered: with two, either could be the copy that runs. The +/// records must agree on one context. Whichever answers, it must be the context of the hive that +/// named this exe, or the registration is another copy's. +pub(crate) fn update_arguments( + copy: MsiCopy, + product_codes: &[String], + probe: impl FnOnce(&str) -> Result, + record_scopes: impl FnOnce(&str) -> Vec, +) -> Result<&'static [&'static str], String> { + let hive = match copy { + MsiCopy::Legacy => return Ok(&[]), + MsiCopy::Conflicting => return Err(CONFLICTING.into()), + MsiCopy::Registered { hive } => hive, + }; + let [code] = product_codes else { return Err(NOT_ONE_PRODUCT.into()) }; + let fallback = |code: &str| crate::msi_context::single_scope(record_scopes(code)); + let scope = crate::msi_context::update_scope(true, Some(code), probe, fallback)?.ok_or(NOT_ONE_PRODUCT)?; + if scope != hive { + return Err(ANOTHER_COPY.into()); + } + Ok(crate::msi_context::scope_arguments(scope)) +} + +/// How a copy was installed, from what is known about it. The launcher's own per-user package in +/// its recorded or default folder keeps its update exactly as before, whatever else is registered +/// (a stale second record must not stop it). An app is judged by its `App Paths` registration +/// first, so a registered copy always passes the context checks; the folder rule only covers an +/// app copy that no registration names. `registration` is only read when it can decide. +pub(crate) fn classify(is_launcher: bool, in_legacy_folder: bool, registration: impl FnOnce() -> Option) -> Option { + if is_launcher && in_legacy_folder { + return Some(MsiCopy::Legacy); + } + registration().or_else(|| in_legacy_folder.then_some(MsiCopy::Legacy)) +} + +/// Which registrations name `exe`. `registered` holds every `App Paths` target with the context +/// of the hive it came from; both sides are expected in one canonical form. +pub(crate) fn match_registration(exe: &Path, registered: &[(MsiScope, PathBuf)]) -> Option { + let mut hives = registered.iter().filter(|(_, target)| same_path(target, exe)).map(|(hive, _)| *hive); + let first = hives.next()?; + Some(if hives.all(|hive| hive == first) { MsiCopy::Registered { hive: first } } else { MsiCopy::Conflicting }) +} + +/// The one exe the app's registrations name after an update. The package may have moved an exact +/// old default folder, so the helper starts this rather than the path it was given. `None` unless +/// every registration names the same file. +pub(crate) fn registered_exe(registered: &[(MsiScope, PathBuf)]) -> Option { + let mut targets = registered.iter().map(|(_, target)| target); + let first = targets.next()?; + targets.all(|t| same_path(t, first)).then(|| first.clone()) +} + +/// Windows paths compare without regard to case. +fn same_path(a: &Path, b: &Path) -> bool { + a.as_os_str().to_string_lossy().eq_ignore_ascii_case(&b.as_os_str().to_string_lossy()) +} + +/// An `App Paths` default value as a path: Windows allows it in quotes. +pub(crate) fn app_path_value(value: &str) -> Option { + let value = value.trim().trim_matches('"').trim(); + (!value.is_empty()).then(|| PathBuf::from(value)) +} + +#[cfg(windows)] +pub(crate) mod windows { + use std::path::{Path, PathBuf}; + + use winreg::RegKey; + use winreg::enums::{HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE, KEY_READ, KEY_WOW64_32KEY, KEY_WOW64_64KEY}; + + use super::{MsiCopy, MsiScope, app_path_value, match_registration}; + use crate::app::App; + + /// `path` with links and `..` resolved in ordinary drive syntax, or as given when it cannot be + /// resolved, so registry values and the running exe compare in one form. + fn canonical(path: &Path) -> PathBuf { + dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) + } + + /// Every `App Paths` target registered for one of `app`'s exe names, with the context of its + /// hive. Entries naming a missing file are left out. + pub(crate) fn registered_exes(app: &App) -> Vec<(MsiScope, PathBuf)> { + let hives = [ + (HKEY_LOCAL_MACHINE, KEY_WOW64_64KEY, MsiScope::PerMachine), + (HKEY_LOCAL_MACHINE, KEY_WOW64_32KEY, MsiScope::PerMachine), + (HKEY_CURRENT_USER, 0, MsiScope::PerUser), + ]; + let mut out: Vec<(MsiScope, PathBuf)> = Vec::new(); + for stem in app.stems() { + let key = format!(r"SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\{stem}.exe"); + for (hive, flags, scope) in hives { + let Ok(entry) = RegKey::predef(hive).open_subkey_with_flags(&key, KEY_READ | flags) else { continue }; + let Some(target) = entry.get_value::("").ok().as_deref().and_then(app_path_value) else { continue }; + if target.is_file() { + let target = canonical(&target); + if !out.iter().any(|(s, t)| *s == scope && *t == target) { + out.push((scope, target)); + } + } + } + } + out + } + + /// How `exe` was installed by an MSI, or `None` when it wasn't. Reads the registry only. + pub(crate) fn find(app: &App, exe: &Path) -> Option { + let exe = canonical(exe); + let is_launcher = app.id == crate::suite::LAUNCHER.id; + super::classify(is_launcher, is_legacy(app, &exe), || match_registration(&exe, ®istered_exes(app))) + } + + /// The `msiexec` properties for `exe`'s update, asking Windows Installer for its context. + pub(crate) fn update_arguments(app: &App, exe: &Path) -> Result<&'static [&'static str], String> { + let copy = find(app, exe).ok_or_else(|| format!("{} isn't registered with the Windows Installer", app.name))?; + let codes: Vec = crate::detect::windows::msi_installs(app) + .into_iter() + .map(|m| m.product_code) + .filter(|code| crate::msi_context::valid_product_code(code)) + .collect(); + super::update_arguments(copy, &codes, crate::msi_context::probe, record_scopes) + } + + /// The context of every uninstall record of `code`: the machine's (both registry views) are + /// per-machine, the account's own per-user. Two views of one machine record count once. + fn record_scopes(code: &str) -> Vec { + const UNINSTALL: &str = r"SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall"; + let roots = [ + (HKEY_LOCAL_MACHINE, KEY_WOW64_64KEY, MsiScope::PerMachine), + (HKEY_LOCAL_MACHINE, KEY_WOW64_32KEY, MsiScope::PerMachine), + (HKEY_CURRENT_USER, 0, MsiScope::PerUser), + ]; + let mut scopes = Vec::new(); + for (hive, flags, scope) in roots { + let found = RegKey::predef(hive).open_subkey_with_flags(format!(r"{UNINSTALL}\{code}"), KEY_READ | flags).is_ok(); + if found && !scopes.contains(&scope) { + scopes.push(scope); + } + } + scopes + } + + /// The launcher's own per-user MSI: the folder its wizard records in HKCU, or its default + /// `%LOCALAPPDATA%\Programs\`. + fn is_legacy(app: &App, exe: &Path) -> bool { + let Some(dir) = exe.parent() else { return false }; + let recorded = recorded_dir(app).map(|d| canonical(&d)); + let default = crate::folders::data_local_dir().map(|d| canonical(&d.join("Programs").join(app.name))); + [recorded, default].into_iter().flatten().any(|d| d == dir) + } + + /// The install folder the launcher's per-user wizard records in HKCU, when it recorded one. + fn recorded_dir(app: &App) -> Option { + let key = RegKey::predef(HKEY_CURRENT_USER).open_subkey(format!(r"Software\Learning Machines LLC\{}", app.name)).ok()?; + let dir: String = key.get_value("InstallDir").ok()?; + let dir = dir.trim(); + (!dir.is_empty()).then(|| PathBuf::from(dir)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::msi_context::ProbeError; + + const CODE: &str = "{500F5254-26F4-4AAA-B90D-6F3FD9947D14}"; + + fn p(s: &str) -> PathBuf { + PathBuf::from(s) + } + + fn codes(list: &[&str]) -> Vec { + list.iter().map(|c| c.to_string()).collect() + } + + #[test] + fn a_registration_makes_an_msi_copy_wherever_its_folder_is() { + let registered = [(MsiScope::PerMachine, p(r"C:\Program Files\GridCraft\gridcraft.exe"))]; + let copy = Some(MsiCopy::Registered { hive: MsiScope::PerMachine }); + assert_eq!(match_registration(&p(r"C:\Program Files\GridCraft\gridcraft.exe"), ®istered), copy); + assert_eq!(match_registration(&p(r"c:\program files\gridcraft\GRIDCRAFT.EXE"), ®istered), copy); + let user = [(MsiScope::PerUser, p(r"D:\Chosen\GridCraft\gridcraft.exe"))]; + assert_eq!(match_registration(&p(r"D:\Chosen\GridCraft\gridcraft.exe"), &user), Some(MsiCopy::Registered { hive: MsiScope::PerUser })); + } + + #[test] + fn an_exe_no_registration_names_is_not_an_msi_copy() { + let registered = [(MsiScope::PerMachine, p(r"C:\Program Files\ArtCraft\GridCraft\gridcraft.exe"))]; + assert_eq!(match_registration(&p(r"D:\Tools\GridCraft\gridcraft.exe"), ®istered), None); + assert_eq!(match_registration(&p(r"D:\Tools\GridCraft\gridcraft.exe"), &[]), None); + } + + #[test] + fn a_copy_registered_in_both_hives_stops_before_any_lookup() { + let exe = p(r"C:\Program Files\ArtCraft\GridCraft\gridcraft.exe"); + let copy = match_registration(&exe, &[(MsiScope::PerMachine, exe.clone()), (MsiScope::PerUser, exe.clone())]).unwrap(); + assert_eq!(copy, MsiCopy::Conflicting); + assert!(update_arguments(copy, &codes(&[CODE]), |_| panic!("must not look up"), |_| panic!("must not read the records")).is_err()); + } + + #[test] + fn windows_installer_confirms_the_context_of_the_hive_that_named_the_exe() { + let copy = MsiCopy::Registered { hive: MsiScope::PerMachine }; + assert_eq!( + update_arguments(copy, &codes(&[CODE]), |_| Ok(MsiScope::PerMachine), |_| panic!("must not read the records")).unwrap(), + ["ALLUSERS=1"] + ); + let copy = MsiCopy::Registered { hive: MsiScope::PerUser }; + assert_eq!( + update_arguments(copy, &codes(&[CODE]), |_| Ok(MsiScope::PerUser), |_| panic!("must not read the records")).unwrap(), + ["ALLUSERS=2", "MSIINSTALLPERUSER=1"] + ); + } + + #[test] + fn a_registration_in_the_other_context_is_another_copys_and_stops_the_update() { + for (hive, other) in [(MsiScope::PerUser, MsiScope::PerMachine), (MsiScope::PerMachine, MsiScope::PerUser)] { + let error = + update_arguments(MsiCopy::Registered { hive }, &codes(&[CODE]), |_| Ok(other), |_| panic!("must not read the records")).unwrap_err(); + assert_eq!(error, ANOTHER_COPY); + } + } + + #[test] + fn the_uninstall_records_answer_only_when_the_lookup_cannot_run() { + let copy = MsiCopy::Registered { hive: MsiScope::PerMachine }; + let blocked = |_: &str| Err(ProbeError::Unavailable("PowerShell is blocked".into())); + assert_eq!(update_arguments(copy, &codes(&[CODE]), blocked, |_| vec![MsiScope::PerMachine]).unwrap(), ["ALLUSERS=1"]); + for unclear in [ProbeError::Unknown, ProbeError::Conflicting] { + assert!(update_arguments(copy, &codes(&[CODE]), |_| Err(unclear), |_| panic!("must not read the records")).is_err()); + } + } + + #[test] + fn missing_mixed_or_mismatched_records_stop_the_fallback() { + let copy = MsiCopy::Registered { hive: MsiScope::PerMachine }; + let blocked = |_: &str| Err(ProbeError::Unavailable("PowerShell is blocked".into())); + assert!(update_arguments(copy, &codes(&[CODE]), blocked, |_| vec![]).is_err()); + assert!(update_arguments(copy, &codes(&[CODE]), blocked, |_| vec![MsiScope::PerMachine, MsiScope::PerUser]).is_err()); + let error = update_arguments(copy, &codes(&[CODE]), blocked, |_| vec![MsiScope::PerUser]).unwrap_err(); + assert_eq!(error, ANOTHER_COPY); + } + + #[test] + fn the_update_stops_unless_exactly_one_product_is_registered() { + let copy = MsiCopy::Registered { hive: MsiScope::PerMachine }; + assert!(update_arguments(copy, &[], |_| panic!("must not look up"), |_| panic!("must not read the records")).is_err()); + let two = codes(&[CODE, "{0BE5923F-1AAB-4240-8174-2F0CCC05E14C}"]); + assert!(update_arguments(copy, &two, |_| panic!("must not look up"), |_| panic!("must not read the records")).is_err()); + } + + #[test] + fn the_launcher_in_its_own_folder_keeps_its_old_path_whatever_is_registered() { + let registered = Some(MsiCopy::Registered { hive: MsiScope::PerUser }); + assert_eq!(classify(true, true, || registered), Some(MsiCopy::Legacy)); + assert_eq!(classify(true, false, || registered), registered); + assert_eq!(classify(true, false, || None), None); + } + + #[test] + fn an_app_in_the_old_per_user_folder_is_judged_by_its_registration_first() { + for registered in [MsiCopy::Registered { hive: MsiScope::PerUser }, MsiCopy::Conflicting] { + assert_eq!(classify(false, true, || Some(registered)), Some(registered)); + } + assert_eq!(classify(false, true, || None), Some(MsiCopy::Legacy)); + assert_eq!(classify(false, false, || None), None); + } + + #[test] + fn the_launchers_own_package_keeps_its_default() { + assert!(update_arguments(MsiCopy::Legacy, &[], |_| panic!("must not look up"), |_| panic!("must not read the records")).unwrap().is_empty()); + } + + #[test] + fn the_helper_restarts_the_one_exe_the_registrations_name() { + let moved = p(r"C:\Program Files\ArtCraft\GridCraft\gridcraft.exe"); + assert_eq!(registered_exe(&[(MsiScope::PerMachine, moved.clone())]), Some(moved.clone())); + assert_eq!(registered_exe(&[(MsiScope::PerMachine, moved.clone()), (MsiScope::PerUser, moved.clone())]), Some(moved)); + assert_eq!(registered_exe(&[]), None); + let two = [(MsiScope::PerMachine, p(r"C:\a\gridcraft.exe")), (MsiScope::PerUser, p(r"C:\b\gridcraft.exe"))]; + assert_eq!(registered_exe(&two), None); + } + + #[test] + fn app_paths_values_may_be_quoted() { + assert_eq!( + app_path_value(r#""C:\Program Files\ArtCraft\GridCraft\gridcraft.exe""#), + Some(p(r"C:\Program Files\ArtCraft\GridCraft\gridcraft.exe")) + ); + assert_eq!(app_path_value(" "), None); + } +} diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index f22dfb35a..a3b3bc4c4 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -9,7 +9,7 @@ //! | Installed as | Swap | //! |---|---| //! | macOS `.app` | copy the new bundle from the DMG over this one (fine while running), reopen it | -//! | Windows MSI (per-user, any folder) | after exit, run the new MSI (it reinstalls into the same folder), then start the exe | +//! | Windows MSI (any folder, for one account or everyone) | after exit, run the new MSI in the copy's own context (everyone asks for permission), then start the exe it registers | //! | Windows portable zip | after exit, unzip over this folder (settings in `ArtCraftLauncherData` stay) | //! | Linux AppImage | rename the new AppImage over this one (fine while running), start it | //! | Linux `.deb` / `.rpm` | the system package manager, with the password prompt; then restart | @@ -182,18 +182,17 @@ fn detect(app: &App, exe: &Path) -> Option { None } +/// How the Windows copy at `exe` was installed: portable (a `portable.txt` beside it), by its MSI +/// (registered in either context, or the launcher's own per-user package), else a loose folder. #[cfg(windows)] fn detect(app: &App, exe: &Path) -> Option { let dir = exe.parent()?; if dir.join("portable.txt").is_file() { return Some(Install::Portable(exe.to_path_buf())); } - // The MSI installs per user, into the folder chosen in its wizard (which it records in the - // registry) or by default %LOCALAPPDATA%\Programs\. - let canonical = |p: PathBuf| p.canonicalize().unwrap_or(p); - let recorded = msi_install_dir(app).map(canonical); - let default = crate::folders::data_local_dir().map(|d| canonical(d.join("Programs").join(app.name))); - if [recorded, default].into_iter().flatten().any(|d| d == dir) { + // Any folder, either context: the MSI's own registration says so (see `msi_copy`). A copy + // installed for everyone must never be unpacked over as if it were a loose folder. + if crate::msi_copy::windows::find(app, exe).is_some() { return Some(Install::Msi(exe.to_path_buf())); } if exe.is_file() { @@ -202,16 +201,22 @@ fn detect(app: &App, exe: &Path) -> Option { None } -/// The folder the MSI installed into, as it records it. +/// The `msiexec` properties that keep `exe`'s Windows Installer context, or why its update stops. +/// Asked while the app still runs (so it can show the error) and again by the helper. #[cfg(windows)] -fn msi_install_dir(app: &App) -> Option { - use winreg::RegKey; - use winreg::enums::HKEY_CURRENT_USER; - let subkey = format!(r"Software\Learning Machines LLC\{}", app.name); - let key = RegKey::predef(HKEY_CURRENT_USER).open_subkey(&subkey).ok()?; - let dir: String = key.get_value("InstallDir").ok()?; - let dir = dir.trim(); - (!dir.is_empty()).then(|| PathBuf::from(dir)) +fn msi_arguments(app: &App, exe: &Path) -> Result<&'static [&'static str], String> { + crate::msi_copy::windows::update_arguments(app, exe) +} + +/// The exe to start after an MSI update of a registered copy: the one its registrations name now +/// (the package may have moved an exact old default folder), else `exe`. +#[cfg(windows)] +fn exe_after_msi(app: &App, registered: bool, exe: &Path) -> PathBuf { + registered + .then(|| crate::msi_copy::registered_exe(&crate::msi_copy::windows::registered_exes(app))) + .flatten() + .filter(|moved| is_own_executable(app, moved)) + .unwrap_or_else(|| exe.to_path_buf()) } #[cfg(target_os = "linux")] @@ -628,11 +633,15 @@ pub fn finish(app: &App, args: &[OsString]) -> Option> { record_failure(&paths, &e); return Some(Err(e)); } + // Read before the step: the update may move or replace the old exe. + let registered = + matches!(step, Finish::Msi(_)) && matches!(crate::msi_copy::windows::find(app, &exe), Some(crate::msi_copy::MsiCopy::Registered { .. })); let done = run_step(app, &me, pid, &exe, &step, std::time::Duration::from_secs(60)); if let Err(e) = &done { record_failure(&paths, e); } // The new app, or the old one when the step failed. + let exe = if done.is_ok() { exe_after_msi(app, registered, &exe) } else { exe }; let started = signed_like(app, &me, &exe) .and_then(|()| process::spawn_detached(std::process::Command::new(&exe)).map_err(|e| format!("couldn't start {} ({e})", exe.display()))); if let Err(e) = &started { @@ -688,6 +697,7 @@ fn windows_precheck(app: &App, install: &Install, paths: &Paths, download: &Path let me = std::env::current_exe().map_err(|e| format!("couldn't find this program ({e})"))?; match install { Install::Msi(exe) => signed_like(app, &me, exe) + .and_then(|()| msi_arguments(app, exe).map(drop)) .and_then(|()| signed_like(app, &me, download)) .and_then(|()| own_executable(app, exe)) .and_then(|()| same_cache_as_helper(app, paths, exe)), @@ -824,7 +834,9 @@ fn run_step(app: &App, reference: &Path, pid: u32, exe: &Path, step: &Finish, wa wait_watch::returned(pid, waited); let result = if waited { match step { - Finish::Msi(msi) => signed_like(app, reference, msi).and_then(|()| install::windows::reinstall(msi)), + Finish::Msi(msi) => signed_like(app, reference, msi) + .and_then(|()| msi_arguments(app, exe)) + .and_then(|arguments| install::windows::reinstall(msi, arguments)), Finish::Portable { zip, tmp } => match (exe.parent(), exe.file_name()) { (Some(dir), Some(executable)) => crate::portable::unpack_over(zip, tmp, dir, executable, &|file| { // The expected executable must be signed even when its contents aren't a PE file.