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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions crates/engine/src/cmd/labels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ pub fn commands() -> Vec<C> {
})
.view()
.params(r#"{"on"?: bool}"#),
C::new("labels.fonts", "Label Fonts", "Style › Text Symbol", |_, p| {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

labels.fonts calls families::installed(), which blocks on the OnceLock if the background font-index thread is still walking. Commands run on the UI thread in the app, so calling this from the command palette or an assistant tool right after startup freezes the UI until the index finishes. The panes avoid this with installed_now(). Consider returning an empty list (or an "indexing" flag) when the index isn't ready and warming has started. Confidence: medium.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid, fixed in 9e51082. I had kept this blocking in the earlier replies, but commands do run on the UI thread in the app. labels.fonts now uses families::available(), which find_ready uses too. While the app's warm_up index is still running, it answers {"fonts": [], "indexing": true} at once, and asking again later lists them. Where nothing warmed up (scripts, the command line, the engine tests), the families are indexed on the spot, as before. The params string documents indexing.

// The families a font picker lists, those containing `search` (in any case) if given.
// While the app is still indexing them (commands run on its UI thread), none yet,
// and `indexing` says so: asking again later lists them.
let search = ps(p, "search").map(str::to_lowercase).unwrap_or_default();
let Some(all) = crate::render::families::available() else { return Ok(json!({"fonts": [], "indexing": true})) };
let fonts: Vec<&str> =
all.iter().filter(|f| f.listed && f.name.to_lowercase().contains(&search)).map(|f| f.name.as_str()).collect();
Ok(json!({"fonts": fonts}))
})
.view()
.params(r#"{"search"?: text in the family name} → {"fonts": [the installed font families, by name], "indexing"?: true (still being indexed: none listed yet)}"#),
C::new("labels.convert", "Convert To Annotation", "View › Labeling", convert)
.when(need_feature_layer)
.params(r#"{"layer"?, "name"?: string}"#),
Expand Down
8 changes: 7 additions & 1 deletion crates/engine/src/cmd/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,13 @@ pub fn commands() -> Vec<C> {
let size = f(p, "size").map(|v| v.clamp(*LABEL_SIZE.start(), *LABEL_SIZE.end()) as f32);
let color = s.get_color(p, "color")?;
let (halo, bold, italic) = (b(p, "halo"), b(p, "bold"), b(p, "italic"));
// A family by name, whether or not this machine has it: the project may be drawn on
// one that does. Blank goes back to the label font.
let font = ps(p, "font").map(rustgis_map::symbology::font_name);
super::labels::with_class(s, p, |c| {
if let Some(name) = font.clone() {
c.font = name;
}
if let Some(v) = size {
c.size = v;
}
Expand All @@ -307,7 +313,7 @@ pub fn commands() -> Vec<C> {
})
})
.when(need_feature_layer)
.params(r##"{"layer"?, "class"?: name (none: the Default class, by whatever name it has), "size"?: number, "color"?: "#RRGGBB"|"#RRGGBBAA", "halo"?: bool, "bold"?: bool, "italic"?: bool}"##),
.params(r##"{"layer"?, "class"?: name (none: the Default class, by whatever name it has), "size"?: number, "color"?: "#RRGGBB"|"#RRGGBBAA", "halo"?: bool, "bold"?: bool, "italic"?: bool, "font"?: installed family name (labels.fonts lists them; "" is the label font)}"##),
C::new("layer.definitionQuery", "Definition Query", "Data › Definition Query", |s, p| {
let q = ps(p, "where").map(str::trim).unwrap_or("").to_string();
let id = s.layer_id(p)?;
Expand Down
24 changes: 24 additions & 0 deletions crates/engine/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1710,6 +1710,30 @@ fn label_range_placement_lock_and_convert() {
assert!(!s.map().layer_by_name("Countries").unwrap().labels.enabled);
}

/// A label class's font is set by its family name, installed here or not (the project may be
/// drawn on a machine that has it), and cleared with a blank one; `labels.fonts` lists the
/// installed families.
#[test]
fn labels_take_a_font_by_family_name() {
let mut s = world();
let font = |s: &Session| s.map().layer_by_name("Cities").unwrap().labels.default.font.clone();
s.run("labels.style", &json!({"layer": "Cities", "font": " Liberation Sans Narrow "})).unwrap();
assert_eq!(font(&s).as_deref(), Some("Liberation Sans Narrow"));
// Drawn in the label font where it isn't installed, without fail.
s.run("labels.enable", &json!({"layer": "Cities", "on": true})).unwrap();
let v = crate::render::View { width: 400.0, height: 300.0, pixels_per_point: 1.0, camera: s.map().camera, sky: false };
crate::render::render(s.map_mut(), &v).unwrap();
s.run("labels.style", &json!({"layer": "Cities", "size": 12})).unwrap();
assert!(font(&s).is_some(), "other settings leave it");
s.run("labels.style", &json!({"layer": "Cities", "font": ""})).unwrap();
assert_eq!(font(&s), None);
s.run("edit.undo", &json!({})).unwrap();
assert!(font(&s).is_some(), "undoes");
let listed = s.run("labels.fonts", &json!({"search": "zzz-no-such-family"})).unwrap();
assert_eq!(listed["fonts"], json!([]));
assert!(s.run("labels.fonts", &json!({})).unwrap()["fonts"].is_array());
}

/// A lock names each label by its class's place in the list: a change to the list (a class added,
/// moved or removed) locks the labels again on the next frame, a change within a class keeps the
/// lock (#732).
Expand Down
60 changes: 60 additions & 0 deletions crates/map/src/symbology.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,37 @@ pub struct LabelClass {
/// Slanted (an italic face where the font has one).
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
pub italic: bool,
/// The installed font family the labels are drawn in, by name ("Liberation Sans Narrow"), as
/// QGIS names a text format's; none is the label font. A machine without the family draws
/// them in the label font too.
#[serde(default, deserialize_with = "de_font", skip_serializing_if = "Option::is_none")]
pub font: Option<String>,
}

/// The longest font family name kept (characters).
pub const MAX_FONT_NAME: usize = 128;

/// A label font's family name as it is kept: trimmed, at most [`MAX_FONT_NAME`] characters, and
/// none when blank.
pub fn font_name(name: &str) -> Option<String> {
let name: String = name.trim().chars().filter(|c| !c.is_control()).take(MAX_FONT_NAME).collect();
let name = name.trim();
(!name.is_empty()).then(|| name.to_string())
}

/// A label class's font as a project has it, kept as [`font_name`] keeps it: a name, or none for
/// anything else (a number, a list), so a damaged font doesn't make the project unreadable.
fn de_font<'de, D: serde::Deserializer<'de>>(d: D) -> Result<Option<String>, D::Error> {
#[derive(Deserialize)]
#[serde(untagged)]
enum Font {
Name(String),
Other(serde::de::IgnoredAny),
}
Ok(match Option::<Font>::deserialize(d)? {
Some(Font::Name(n)) => font_name(&n),
Some(Font::Other(_)) | None => None,
})
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

de_font calls Option::<String>::deserialize, so a project whose font is not a string (a number, say) fails to deserialize the whole LabelClass, and probably the project with it. The PR calls project files hostile input and the repo says never to crash, so a lenient read would fit better: deserialize a serde_json::Value and keep only strings. Confidence: low-medium.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid, fixed in 44b2d77. de_font now reads either a string or anything else (IgnoredAny). Anything that isn't a name reads as no font (the label font), so the class and the project still read. a_label_font_is_kept_by_name now also feeds 7, ["Arial"] and {"a": 1} and checks the labels read with no font.

fn default_class_name() -> String {
Expand Down Expand Up @@ -1415,6 +1446,7 @@ impl LabelClass {
farthest: None,
placement: LabelPlacement::Auto,
italic: false,
font: None,
}
}

Expand Down Expand Up @@ -1540,6 +1572,34 @@ mod tests {
assert_eq!(serde_json::from_value::<Labeling>(v).unwrap(), with);
}

/// A label font is kept by its family name, trimmed and bounded (a project file is hostile
/// input); a blank one is the label font, and isn't written.
#[test]
fn a_label_font_is_kept_by_name() {
let read = |font: serde_json::Value| {
serde_json::from_value::<Labeling>(serde_json::json!({
"enabled": true, "field": "name", "size": 9.0, "color": [0, 0, 0, 255], "halo": false, "bold": false, "font": font
}))
.unwrap()
};
let l = read(" Liberation Sans Narrow \n".into());
assert_eq!(l.default.font.as_deref(), Some("Liberation Sans Narrow"));
assert_eq!(serde_json::to_value(&l).unwrap()["font"], "Liberation Sans Narrow");
// Not a name (a number, a list, an object): no font, and the rest of the labels still read.
for blank in [
serde_json::json!(" "),
serde_json::Value::Null,
serde_json::json!(7),
serde_json::json!(["Arial"]),
serde_json::json!({"a": 1}),
] {
let l = read(blank);
assert_eq!(l.default.font, None);
assert!(serde_json::to_value(&l).unwrap().get("font").is_none());
}
assert_eq!(read("x".repeat(10_000).into()).default.font.map(|f| f.chars().count()), Some(MAX_FONT_NAME));
}

/// A project file is read with at most `MAX_LABEL_CLASSES` classes, and only that many count
/// in a `Labeling` built with more. A class on
/// a field the data doesn't have is pointed at Default's, and kept.
Expand Down
Loading
Loading