search

JuliaGraphics / Luxor.jl

Simple drawings using vector graphics; Cairo "for tourists!"
http://juliagraphics.github.io/Luxor.jl/
Other
588 stars 71 forks source link

Issues 250 exception #251

Closed oheil closed 1 year ago

codecov[bot] commented 1 year ago

Codecov Report

Base: 73.24% // Head: 74.64% // Increases project coverage by +1.40% :tada:

Coverage data is based on head (f9416f8) compared to base (d9faff1). Patch coverage: 96.66% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #251 +/- ## ========================================== + Coverage 73.24% 74.64% +1.40% ========================================== Files 32 33 +1 Lines 6436 6465 +29 ========================================== + Hits 4714 4826 +112 + Misses 1722 1639 -83 ``` | [Impacted Files](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics) | Coverage Δ | | |---|---|---| | [src/drawings.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL2RyYXdpbmdzLmps) | `77.77% <96.55%> (+6.10%)` | :arrow_up: | | [src/Luxor.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL0x1eG9yLmps) | `100.00% <100.00%> (+25.00%)` | :arrow_up: | | [src/precompile.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL3ByZWNvbXBpbGUuamw=) | `100.00% <0.00%> (ø)` | | | [src/point.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL3BvaW50Lmps) | `91.98% <0.00%> (+0.06%)` | :arrow_up: | | [src/polygons.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL3BvbHlnb25zLmps) | `79.80% <0.00%> (+0.12%)` | :arrow_up: | | [src/curves.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL2N1cnZlcy5qbA==) | `95.98% <0.00%> (+0.80%)` | :arrow_up: | | [src/basics.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL2Jhc2ljcy5qbA==) | `92.27% <0.00%> (+2.31%)` | :arrow_up: | | [src/noise.jl](https://codecov.io/gh/JuliaGraphics/Luxor.jl/pull/251/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL25vaXNlLmps) | `51.44% <0.00%> (+3.74%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

cormullion commented 1 year ago

Thanks for this, I appreciate your efforts!

I'll have a look at this over the holidays. I think this is calling get_current_drawing_save() rather a lot - four times every time we change the color? - but don't know what else to do yet.,,

Have a good holiday!

oheil commented 1 year ago

I think this is calling get_current_drawing_save() rather a lot

OK, I will check and perhaps there is room for further improvements ...

oheil commented 1 year ago

It's 5 times ! ;-)

oheil commented 1 year ago

I think it's more an issue with:

function setcolor(col::AbstractString)
    temp = parse(RGBA, col)
    # set Luxor settings
    set_current_redvalue(temp.r)
    set_current_greenvalue(temp.g)
    set_current_bluevalue(temp.b)
    set_current_alpha(temp.alpha)
    # and set Cairo context too
    Cairo.set_source_rgba(get_current_cr(), temp.r, temp.g, temp.b, temp.alpha)
    return (temp.r, temp.g, temp.b, temp.alpha)
end

This should be changed to something like:

function setcolor(col::AbstractString)
    temp = parse(RGBA, col)
    # set Luxor settings
    set_current_color(temp.r,temp.g,temp.b,temp.alpha)
    # and set Cairo context too
    Cairo.set_source_rgba(get_current_cr(), temp.r, temp.g, temp.b, temp.alpha)
    return (temp.r, temp.g, temp.b, temp.alpha)
end
oheil commented 1 year ago

What do you think about renaming all those not exported functions to _name, e.g.: set_current_redvalue(r) => _set_current_redvalue(r)

I have done it here already locally, just needs uploaded to branch.

oheil commented 1 year ago

Would be best in a separate PR, so you may check it here: https://github.com/oheil/Luxor.jl/tree/pretty-and-optimize