Upgrade svg_fmt_color to svg rgba spec

[Mattriks]

Fixes #318

• I've updated the documentation to reflect these changes
• I've added and/or updated the unit tests
• I've run the regression tests
• I've built the Compose docs and confirmed these changes don't cause new errors
• I've run the Gadfly tests with this PR

This PR

• upgrades svg_fmt_color() to include a method for the svg rgba spec.
• This means opacity can be specified twice: in fill(c) and fillopacity(a), producing the same result as in the svg backend (alphas multiply).

[codecov-io]

Codecov Report

Merging #319 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   78.73%   78.76%   +0.03%     
==========================================
  Files          14       14              
  Lines        1171     1168       -3     
==========================================
- Hits          922      920       -2     
+ Misses        249      248       -1

Impacted Files	Coverage Δ
src/property.jl	100% <ø> (ø)	⬆️
src/svg.jl	78.43% <100%> (+0.07%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a3839a...c414a3f. Read the comment docs.

[Mattriks]

In Gadfly, there are 2 tests that fail: line 15 & 16 in testscripts/density_dark.jl. Note that all the generated images for density_dark.jl are fine. The issue is that these tests test for hex strings in the svg string. But in Compose the FillPrimitive automatically converts any color to a RGBA color, e.g. type fill("blue"). This means that in Compose svg.jl any fill(color) will be converted to the rgba spec, rather than to a hex string. Hence in Gadfly the density_dark.jl test needs to be updated, after this PR has been merged (it's good to go!).

Note also that there is a second issue with the tests in density_dark.jl. The actual test suite gets repeated 6 times, which doesn't seem what was intended. I think this occurs because of the changes in scope.

[tlnagy]

In Gadfly, there are 2 tests that fail: line 15 & 16 in testscripts/density_dark.jl. Note that all the generated images for density_dark.jl are fine. The issue is that these tests test for hex strings in the svg string. But in Compose the FillPrimitive automatically converts any color to a RGBA color, e.g. type fill("blue"). This means that in Compose svg.jl any fill(color) will be converted to the rgba spec, rather than to a hex string. Hence in Gadfly the density_dark.jl test needs to be updated, after this PR has been merged (it's good to go!).

I would be okay with updating the tests to not check hex codes directly

[Mattriks]

Would anyone object to deleting these lines? The images that the test generates still act as a test.

[tlnagy]

What wrong with duplicating this line

Compose.jl/src/svg.jl

Line 89 in 71ffc8e

string("rgba(", join(floor.(Int,[c.r,c.g,c.b].*255),","), ",", svg_fmt_float(c.alpha),")")

over in Gadfly to use instead of the hex codes? Those lines are really testing whether using the dark theme works, which is something we might miss with just the image generation (except when we run full regression testing). We really need to refactor how we do testing in Gadfly. @bjarthur, thoughts?

[Mattriks]

Nothing wrong with upgrading those lines, just checking if people thought differently from me 🙂

[bjarthur]

i'm fine with either deleting those lines or updating them to check for rgba. i've tried to make regression testing painless enough to do with each PR, which i believe it is, just takes time. in what way do you think gadfly testing needs refactoring?

[Mattriks]

I also think the testing in Gadfly works well.

Going back to the density_dark.jl tests, the reason that the test suite within the script gets repeated is because evalfile only returns the last expression, and so the density_dark_tested flag doesn't get passed on to the next backend. Suggestions?

[bjarthur]

just delete those lines. it's the only testscript which has @test inside, right? so deleting them would make things more uniform across testscripts, and we'd just have to rely on the regression testing to make sure it worked right, which is fine.

[bjarthur]

you could instead move them to the bottom of runtests.jl, or some other file which runtest.jl includes. this is probably the best option, as we might want to add other tests like this as well.

[bjarthur]

actually couldn't the loop in runtest.jl be re-written as:

@testset "Gadfly" begin
    for filename in testfiles
        Random.seed!(1)
        p = evalfile(joinpath(testdir, "$(filename).jl"))
        @test typeof(p) in [Plot,Compose.Context]
        for (backend_name, backend) in backends
            @info string(filename,'.',backend_name)
            r = draw(backend(filename), p)
            @test typeof(r) in [Bool,Nothing]
        end
    end
end

this would run the testscript only once, which would not only run the @test block in density_dark once, but tests overall should be faster this way as each Plot is only created once, and not re-created for each backend.

[bjarthur]

@Mattriks @tlnagy this PR has caused some problems.

[Mattriks]

Sounds like a MS issue to me! I like the rgba spec, and think that most julia users probably do too. So my thoughts would be to patch it somehow for windows using Sys.iswindows. I will experiment and get back to you with my suggestions.

[tlnagy]

It's not just a MS issue. I can reproduce with the latest Inkscape build on Ubuntu 18.04. We're gonna have to fix this regression. It's unfortunate that the RGBA standard isn't more widely supported.

The best option might be option ii from here #318 (comment)