Skip to content

Commit

Permalink
Fix spec using labelExpr and tickBand results in error (#73)
Browse files Browse the repository at this point in the history
* Fix build instruction

* Write out failed image when baseline doesn't exist

* Add failing test that crashes with:

Failed to deserialize text info: missing field `text`

* Allow test to be missing in text width calculation

treat as empty string and return zero in this case.
  • Loading branch information
jonmmease authored Jun 26, 2023
1 parent 3a57f1e commit 2fbc7dd
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 19 deletions.
2 changes: 1 addition & 1 deletion DEVELOP.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ This will produce `target/release/vl-convert`, which should be uploaded to the G
### Build Python wheels
Build the Python wheels with:
```
maturin -m vl-convert-python/Cargo.toml --release -i python3.11 -i python3.10 -i python3.9 -i python3.8 -i python3.7 --strip
maturin build -m vl-convert-python/Cargo.toml --release -i python3.11 -i python3.10 -i python3.9 -i python3.8 -i python3.7 --strip
```

This will produce a collection of wheel files in `target/wheels`, which should be uploaded to the GitHub Release below. These wheels must also be uploaded to PyPI with:
Expand Down
1 change: 1 addition & 0 deletions vl-convert-python/tests/test_specs.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ def test_svg(name, as_dict):
("stacked_bar_h", 2.0),
("remote_images", 1.0),
("maptile_background", 1.0),
("no_text_in_font_metrics", 1.0),
],
)
@pytest.mark.parametrize("as_dict", [False])
Expand Down
18 changes: 13 additions & 5 deletions vl-convert-rs/src/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ struct TextInfo {
weight: Option<String>,
family: Option<String>,
size: f64,
text: Value,
text: Option<Value>,
}

impl TextInfo {
Expand Down Expand Up @@ -136,8 +136,9 @@ impl TextInfo {
let mut escaped_text = String::new();

let text = match &self.text {
Value::String(s) => s.to_string(),
text => text.to_string(),
Some(Value::String(s)) => s.to_string(),
Some(text) => text.to_string(),
None => "".to_string(),
};

for char in text.chars() {
Expand Down Expand Up @@ -181,10 +182,17 @@ pub fn op_text_width(text_info_str: String) -> Result<f64, AnyError> {
Err(err) => bail!("Failed to deserialize text info: {}", err.to_string()),
};

if let Some(text) = text_info.text.as_str() {
if text.trim().is_empty() {
// Return width zero for empty strings and missing text
match &text_info.text {
Some(Value::String(text)) => {
if text.trim().is_empty() {
return Ok(0.0);
}
}
None => {
return Ok(0.0);
}
_ => {}
}

let svg = text_info.to_svg();
Expand Down
36 changes: 23 additions & 13 deletions vl-convert-rs/tests/test_specs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,9 @@ fn make_expected_svg_path(name: &str, vl_version: VlVersion, theme: Option<&str>
})
}

fn load_expected_svg(name: &str, vl_version: VlVersion, theme: Option<&str>) -> String {
fn load_expected_svg(name: &str, vl_version: VlVersion, theme: Option<&str>) -> Option<String> {
let spec_path = make_expected_svg_path(name, vl_version, theme);
fs::read_to_string(&spec_path).unwrap_or_else(|_| panic!("Failed to read {:?}", spec_path))
fs::read_to_string(&spec_path).ok()
}

fn write_failed_svg(name: &str, vl_version: VlVersion, theme: Option<&str>, img: &str) -> PathBuf {
Expand All @@ -129,7 +129,7 @@ fn write_failed_svg(name: &str, vl_version: VlVersion, theme: Option<&str>, img:

fn check_svg(name: &str, vl_version: VlVersion, theme: Option<&str>, img: &str) {
let expected = load_expected_svg(name, vl_version, theme);
if img != expected {
if Some(img.to_string()) != expected {
let path = write_failed_svg(name, vl_version, None, img);
panic!(
"Images don't match for {}.svg. Failed image written to {:?}",
Expand All @@ -156,9 +156,9 @@ fn load_expected_png_dssim(
name: &str,
vl_version: VlVersion,
theme: Option<&str>,
) -> DssimImage<f32> {
) -> Option<DssimImage<f32>> {
let spec_path = make_expected_png_path(name, vl_version, theme);
dssim::load_image(&Dssim::new(), spec_path).unwrap()
dssim::load_image(&Dssim::new(), spec_path).ok()
}

fn to_dssim(img: &[u8]) -> DssimImage<f32> {
Expand Down Expand Up @@ -190,16 +190,24 @@ fn write_failed_png(name: &str, vl_version: VlVersion, theme: Option<&str>, img:

fn check_png(name: &str, vl_version: VlVersion, theme: Option<&str>, img: &[u8]) {
let expected_dssim = load_expected_png_dssim(name, vl_version, theme);
let img_dssim = to_dssim(img);

let attr = Dssim::new();
let (diff, _) = attr.compare(&expected_dssim, img_dssim);

if diff > 0.0001 {
println!("DSSIM diff {diff}");
if let Some(expected_dssim) = expected_dssim {
let img_dssim = to_dssim(img);

let attr = Dssim::new();
let (diff, _) = attr.compare(&expected_dssim, img_dssim);

if diff > 0.0001 {
println!("DSSIM diff {diff}");
let path = write_failed_png(name, vl_version, None, img);
panic!(
"Images don't match for {}.png. Failed image written to {:?}",
name, path
)
}
} else {
let path = write_failed_png(name, vl_version, None, img);
panic!(
"Images don't match for {}.png. Failed image written to {:?}",
"Baseline image does not exist for {}.png. Failed image written to {:?}",
name, path
)
}
Expand Down Expand Up @@ -267,6 +275,7 @@ mod test_svg {
"line_with_log_scale",
"numeric_font_weight",
"float_font_size",
"no_text_in_font_metrics"
)]
name: &str,
) {
Expand Down Expand Up @@ -311,6 +320,7 @@ mod test_png_no_theme {
case("remote_images", 1.0),
case("maptile_background", 1.0),
case("float_font_size", 1.0),
case("no_text_in_font_metrics", 1.0),
)]
fn test(
name: &str,
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
30 changes: 30 additions & 0 deletions vl-convert-rs/tests/vl-specs/no_text_in_font_metrics.vl.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"$schema": "https://vega.github.io/schema/vega-lite/v5.9.3.json",
"description": "https://github.com/vega/vl-convert/issues/72",
"data": {
"values": [
{
"column_name": "Prior",
"previous_sum": 1.2,
"log2_bayes_factor": 2.4,
"value": "inal score"
}
]
},
"layer": [
{
"mark": {"type": "bar", "width": 60},
"encoding": {
"x": {
"type": "nominal",
"axis": {
"labelExpr": "datum.value == 'Prior' || datum.value == 'Final score' ? '' : datum.value",
"tickBand": "extent"
},
"field": "column_name"
},
"y": {"type": "quantitative", "field": "previous_sum"}
}
}
]
}

0 comments on commit 2fbc7dd

Please sign in to comment.