search

scour-project / scour

Scour - An SVG Optimizer / Cleaner
Apache License 2.0
770 stars 60 forks source link

Fix #256 - renameID bug plus too few IDs allocatable for remapping #257

Closed nthykier closed 4 years ago

nthykier commented 4 years ago

This PR solves #256, which is really two issues combined:

  1. There was a bug in renameID where if two IDs were swapped (AA <=> a), then all references would end up in one of the IDs (rather than correctly split between the two).
  2. scour would only allocate 26 characters (a-z) for IDs. This branch bumps it to 52 (a-zA-Z), which solves the size issue in #256. Though in theory it can be increased even further (e.g. 0-9 can be used from character 2 and later in the ID) - this is left as future work.

Closes: #256

nthykier commented 4 years ago

@Ede123 Did you have a chance to review this?

Ede123 commented 4 years ago

Sorry, my availability varies these days and I'm usually busy on the weekends. I'll try to find some time to review (hopefully soon).

Ede123 commented 4 years ago

Thanks for this, I've just merged the bug fix in the first commit in f0788d5c0d49ee93e2aad44654cbc4ab22ffa70b .

I have to think a bit about the second commit (Support uppercase letters in generated IDs) - while it's obviously true we can save some bytes by using upper case characters it adds ambiguity ("a" vs "A" might be a different code point but it's still the same letter). Also, as you mention yourself, if we wanted to maximize the number of possible IDs we could allow a lot more characters and squeeze even more bytes out of it (at the cost of legibility).

Feel free to open a new issue about this so we can discuss a bit, if you think optimizing ID generation is a worthwhile task.