Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite colors into shortest possible name #270

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nthykier
Copy link
Contributor

Improve the code for rewriting colors into recognising that some
colors are shorter by name. This commit enables scour to rewrite
rgb(255, 0, 0) into red which is slightly shorter than #f00
(ditto for tan and pink).

When the color name ties in length with the hexcode, then scour will
leave it as-is if the input file used a variant of same length
(e.g. blue, cyan and aqua will be left as-is). But if scour is
rewriting the color code, it will prefer the hex code variant.

Signed-off-by: Niels Thykier niels@thykier.net

@Ede123
Copy link
Member

Ede123 commented Feb 23, 2021

Would this still create a group and propagate the fill to the parent for the following?

<rect fill="#f00" x="10" y="10" width="10" height="10"/>
<rect fill="red" x="20" y="20" width="10" height="10"/>

Improve the code for rewriting colors into recognising that some
colors are shorter by name.  This commit enables scour to rewrite
`rgb(255, 0, 0)` into `red` which is slightly shorter than `#f00`
(ditto for `tan` and `pink`).

When the color name ties in length with the hexcode, then scour will
leave it as-is if the input file used a variant of same length
(e.g. `blue`, `cyan` and `aqua` will be left as-is).  But if scour is
rewriting the color code, it will prefer the hex code variant.

Signed-off-by: Niels Thykier <niels@thykier.net>
@nthykier
Copy link
Contributor Author

nthykier commented Feb 23, 2021

Would this still create a group and propagate the fill to the parent for the following?

[...]

Neither the old nor the new code gets this consistently right because that is governed by another part of the code. For reference, the new code gets your particular example correct. However, I would expect both the old and the new code to fail that test with e.g. cyan vs aqua vs #0ff.

For that to (always) work, then we would also have to change:

            newColorValue = convertColor(oldColorValue)
            oldBytes = len(oldColorValue)
            newBytes = len(newColorValue)
            if oldBytes > newBytes:
                element.setAttribute(attr, newColorValue)
                numBytes += (oldBytes - len(element.getAttribute(attr)))

Notably, the if oldBytes > newBytes: which should be rewritten as if oldBytes >= newBytes and oldColorValue != newColorValue:

@Ede123
Copy link
Member

Ede123 commented Feb 23, 2021

Yep, you're right, as long as we convert consistently (even if it's not shorter but the same length) we should be fine optimization-wise. We should probably add a few testcases for these border cases as well...

Remaining questions I'm mulling about:

  • The related preference is named --disable-simplify-colors. You already adjusted the description, but I figure the name is not really suitable anymore. Maybe we should consider renaming it (keeping the old name as fall-back as we did for other preferences), e.g. --disable-shorten-colors?
  • I have to admit seeing RGB colors and color names mixed in a document makes my skin crawl. Is this just me? Objectively speaking going for the shortest name seems reasonable, so I probably should bite the bullet here, but I wanted to put it out there for consideration at least. 😉

It replaces `--disable-simplify-colors` (although the old name is
still accepted).

Signed-off-by: Niels Thykier <niels@thykier.net>
This enables scour to consistently perform other optimizations that
rely on string equality to determine if two attributes are identical.

Signed-off-by: Niels Thykier <niels@thykier.net>
Signed-off-by: Niels Thykier <niels@thykier.net>
@nthykier
Copy link
Contributor Author

Ok, I have:

  • Added --disable-shorten-colors as an alias of --disable-simplify-colors
  • Made scour always normalize to the same variant of a color (except when we are not shorting colors).
  • Fixed a bug in handling of upper case hex code writing two scour runs to be normalized to the shortest possible code.

@nthykier
Copy link
Contributor Author

nthykier commented Feb 23, 2021

As for mixing color codes with color names - scour is here to write short svg files, not beautiful ones. :)

Also, if your beef with this is about consistency, then we are trading one consistency (form) for another (shortest length). 😄

Signed-off-by: Niels Thykier <niels@thykier.net>
s = s.lower()
color = colors.get(s)
# Short cut: if we know the color (either by name or hex code) then skip the
# parsing logic. This makes
Copy link

Choose a reason for hiding this comment

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

This looks like an incomplete thought.

@eweitz
Copy link

eweitz commented Dec 13, 2021

As someone who independently developed a rough version of this, I'd find this functionality useful.

Could a Scour maintainer chime in to note what could move this PR forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants