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

[RFC] Generalize some fallbacks #29

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kimikage
Copy link
Contributor

When I was working on PR #22, I thought some of the fallbacks weren't pretty good.
However, as it stands, Graphics.jl does not really play a role as an abstraction layer, so I have doubts about the necessity of making these changes.

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #29 (430abe0) into master (545a672) will decrease coverage by 3.36%.
The diff coverage is 6.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   45.25%   41.89%   -3.37%     
==========================================
  Files           1        1              
  Lines         137      148      +11     
==========================================
  Hits           62       62              
- Misses         75       86      +11     
Impacted Files Coverage Δ
src/Graphics.jl 41.89% <6.45%> (-3.37%) ⬇️

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 545a672...4331b78. Read the comment docs.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -672,8 +682,12 @@ source color.
@mustimplement set_source(gc::GraphicsContext, src)

function set_source(gc::GraphicsContext, c::Color)
rgb = convert(RGB, c)
set_source_rgb(gc, rgb.r, rgb.g, rgb.b)
c isa AbstractRGB && return set_source_rgb(gc, red(c), green(c), blue(c))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe slightly cleaner with dispatch but no big deal either way

Copy link
Contributor Author

@kimikage kimikage Feb 10, 2021

Choose a reason for hiding this comment

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

I almost forgot the original intent, but the specialization can introduce ambiguity in the backend implementations.

Of course, it is possible to dispatch "private" methods.


Transform a coordinate `c` from the device space to the user space.

See also: [`device_to_user`](@ref), [`user_to_device!`](@ref)
"""
device_to_user!(gc::GraphicsContext, c::Vector{Float64}) = c # TODO: generalization
function device_to_user!(gc::GraphicsContext, c::AbstractVector{T}) where T <: Real
length(c) == 2 || throw(ArgumentError("Only 2-D vectors are supported."))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
length(c) == 2 || throw(ArgumentError("Only 2-D vectors are supported."))
length(c) == 2 || throw(ArgumentError("Only length-2 vectors are supported."))


Transform a coordinate `c` from the user space to the device space.

See also: [`user_to_device`](@ref), [`device_to_user!`](@ref)
"""
user_to_device!(gc::GraphicsContext, c::Vector{Float64}) = c # TODO: generalization
function user_to_device!(gc::GraphicsContext, c::AbstractVector{T}) where T <: Real
length(c) == 2 || throw(ArgumentError("Only 2-D vectors are supported."))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
length(c) == 2 || throw(ArgumentError("Only 2-D vectors are supported."))
length(c) == 2 || throw(ArgumentError("Only length-2 vectors are supported."))

@kimikage
Copy link
Contributor Author

IMO, it is better to add tests before making any changes. However, implementing a new backend for testing is not very productive, and it may contain bugs.
I think it might be a good idea to add examples with Cairo.jl to the documentation. Of course, Cairo.jl also has its examples, though.

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.

2 participants