Group by style may produce ungroupable groups

[slyrz]

As the title suggests, in some cases the --group-by-style option may produce groups that should be ungrouped by svgcleaner (40cc1b7).

Input

<svg height="50" width="50"  viewBox="0 0 50 50" xmlns="http://www.w3.org/2000/svg">
  <ellipse cx="25" cy="25" rx="10" ry="10" style="fill:#000000" />
  <ellipse cx="25" cy="25" rx="10" ry="10" style="fill:#000000" />
  <ellipse cx="25" cy="25" rx="10" ry="10" style="fill:#000000" />
</svg>

Output

• svgcleaner --indent 2 --group-by-style true <files>
• svgcleaner --indent 2 --group-by-style true --multipass true <files>

<svg height="50" viewBox="0 0 50 50" width="50" xmlns="http://www.w3.org/2000/svg">
  <g>
    <ellipse cx="25" cy="25" rx="10" ry="10"/>
    <ellipse cx="25" cy="25" rx="10" ry="10"/>
    <ellipse cx="25" cy="25" rx="10" ry="10"/>
  </g>
</svg>

• svgcleaner --indent 2 --group-by-style false <files>

<svg height="50" viewBox="0 0 50 50" width="50" xmlns="http://www.w3.org/2000/svg">
  <ellipse cx="25" cy="25" rx="10" ry="10"/>
  <ellipse cx="25" cy="25" rx="10" ry="10"/>
  <ellipse cx="25" cy="25" rx="10" ry="10"/>
</svg>

One should expect --multipass true to fix this, but it keeps the group untouched.

[RazrFalcon]

You right. It's a bug.

The problem is that you using a default fill. It will be removed by --remove-default-attributes, but it actually only 'hides' it. So --ungroup-groups will still see it and will keep a group...

The whole concept of hidden attributes a bit broken...

I'll try to remove default attributes completely in hope it will help.

[RazrFalcon]

Should be fixed by the latest commit. Try it out.

[slyrz]

Weird, I just wanted to run the new version on master and I get Error: Document didn't have any nodes. on every SVG file I try.

Even weirder, the error remains when I reset the git repository to the version 0.8
commit (40cc1b7) and build again. It's the version I installed 3 days ago and that's working on my computer.

To make the error reproducible, this script ends with the above error message:

#!/usr/bin/env bash
cd `mktemp -d`
wget -q https://github.com/RazrFalcon/svgcleaner/archive/v0.8.0.zip
unzip v0.8.0.zip
cd svgcleaner-0.8.0
cargo build

cat > input.svg <<- EOM
<svg height="50" width="50"  viewBox="0 0 50 50" xmlns="http://www.w3.org/2000/svg">
  <ellipse cx="25" cy="25" rx="10" ry="10" style="fill:#000000" />
  <ellipse cx="25" cy="25" rx="10" ry="10" style="fill:#000000" />
  <ellipse cx="25" cy="25" rx="10" ry="10" style="fill:#000000" />
</svg>
EOM

./target/debug/svgcleaner input.svg output.svg

Am I doing something wrong? As I said, my installed version works and it's 0.8 and just 3 days old:

$ stat -c "%y" `which svgcleaner`; svgcleaner -V
2017-01-17 12:26:56.613874159 +0100
svgcleaner 0.8.0

[RazrFalcon]

Weird indeed. Try:
cargo run -- input.svg output.svg

[slyrz]

So svgcleaner parses the CLI arguments correctly and handles the right input and output file, but parsing the input file fails. The problem seems to be purely in the svgdom crate.

The following code using svgdom = "0.3" fails with Document didn't have an SVG element:

extern crate svgdom;

use svgdom::{Document, ParseOptions};

const DATA: &'static [u8] = br#"
<svg xmlns="http://www.w3.org/2000/svg" version="1.1">
  <rect x="25" y="25" width="200" height="200" fill="lime" stroke-width="4" stroke="pink" />
</svg>
"#;

fn main() {
    Document::from_data_with_opt(DATA, &ParseOptions::default()).unwrap();
}

[RazrFalcon]

Yes. It's svgdom error message, but it's works fine for me. Even yours example.

Have no idea what can be wrong.

1. What encoding do you use?
2. What OS?
3. What Rust version?

[slyrz]

Okay this is very weird (but it's also weird that the cargo install svgcleaner from 3 days ago works just fine). I use the latest stable Rust version rustc 1.14.0 (e8a012324 2016-12-16) on a stock Ubuntu 16.04.1 with Linux 4.4.0-59-generic. Encoding is UTF-8.

I'm digging into this right now. For the standalone example in my previous comment, the error is produced by this line. The processed token stream is:

ElementStart(svg)
Attribute(xmlns, Stream { text: http://www.w3.org/2000/svg, pos: 0, end: 26, has_parent: true, parent_pos: 13 })
Attribute(version, Stream { text: 1.1, pos: 0, end: 3, has_parent: true, parent_pos: 50 })
ElementEnd(>)
Whitespace(
  )
ElementStart(rect)
Attribute(x, Stream { text: 25, pos: 0, end: 2, has_parent: true, parent_pos: 67 })
Attribute(y, Stream { text: 25, pos: 0, end: 2, has_parent: true, parent_pos: 74 })
Attribute(width, Stream { text: 200, pos: 0, end: 3, has_parent: true, parent_pos: 85 })
Attribute(height, Stream { text: 200, pos: 0, end: 3, has_parent: true, parent_pos: 98 })
Attribute(fill, Stream { text: lime, pos: 0, end: 4, has_parent: true, parent_pos: 109 })
Attribute(stroke-width, Stream { text: 4, pos: 0, end: 1, has_parent: true, parent_pos: 129 })
Attribute(stroke, Stream { text: pink, pos: 0, end: 4, has_parent: true, parent_pos: 140 })
ElementEnd(/>)
Whitespace(
)
ElementEnd(</)
EndOfStream

[slyrz]

I think the phf crate is broken. It got updated a day ago, it might be the culprit. Your libsvgparser can't find neither the "svg" ElementId nor the "rect" ElementId in this function.

[RazrFalcon]

Thank! After updating the phf also got this error. I have downgraded phf version. Run cargo update and everything should be fixed.

Sadly, I can't fix bug in current version in crates. I'll fill a bug to the phf. Maybe they can fix this issue.

[slyrz]

I tried the new version now and no more empty groups! The output of svgcleaner looks perfect now. Thank you!