linebender / parley

Rich text layout library
Apache License 2.0
158 stars 18 forks source link

[deps] Revert fontique dep `roxmltree` to 0.18.1 #43

Closed waywardmonkeys closed 2 months ago

waywardmonkeys commented 2 months ago

The update to 0.19.0 broke the masonry tests in the xilem repo on Ubuntu.

waywardmonkeys commented 2 months ago

This reverts #42.

DJMcNab commented 2 months ago

This is a not a very compelling fix. Can you test linebender/xilem#221 from 9a519ba644eda13783a7326539dd0305dc206ba8 instead, which should have this same tree.

I would much prefer us to just fix this failure, if at all possible. cc @xorgy

waywardmonkeys commented 2 months ago

I bisected the tree by running it in CI outside of the Linebender org (my own fork). The memmap2 update and previous commits are okay. Only this last one that updated roxmltree makes things sad.

It does show that we really need actual tests for things.

waywardmonkeys commented 2 months ago

It was several tests failing with:

---- widget::align::tests::centered stdout ----
thread 'widget::align::tests::centered' panicked at /home/runner/.cargo/git/checkouts/parley-7d89eb78db3c5efe/9e2be05/src/layout/line/greedy.rs:516:37:
range end index 1 out of range for slice of length 0

And that line of code was:

    for (i, run_data) in layout.runs[state.runs.clone()].iter().enumerate() {

My guess is that the fontconfig failed to load and then things went wrong after that.

dfrg commented 2 months ago

At a glance, this might have to do with changes to DTD parsing in roxmltree. Changing the fontconfig backend to use Document::parse_with_options with allow_dtd set to true might fix it. If not, we’ll likely need to consider a different xml library.

dfrg commented 2 months ago

The specific line is here: https://github.com/linebender/parley/blob/9e2be05f4bd8935758dc0074638c2ec6eaff3806/fontique/src/backend/fontconfig/config.rs#L20

dfrg commented 2 months ago

This seems to fix it. See #44 and linebender/xilem#224

waywardmonkeys commented 2 months ago

This isn’t needed now!