Remove unused code

[rikhuijzer]

Correct me if I'm wrong, but I cannot think of any way how these lines of code are actually doing anything :)

[bjarthur]

can issatisfied() really never return false?

[rikhuijzer]

can issatisfied() really never return false?

I don't think that the output of issatisfied() could alter the behavior of the program, because it isn't written to a variable nor is it returned in the function call. The only possibility would be if issatisfied() is not a pure function call because that means it would change state somewhere else. However, the definition of issatisfied is

# src/table.jl line 111 - 115
function issatisfied(ctx::Context, width, height)
    eps = 1e-4
    return (minwidth(ctx) == nothing || width + eps >= minwidth(ctx)) &&            
    (minheight(ctx) == nothing || height + eps >= minheight(ctx))
end

where eps is the only non-const occurrence of eps that I can find, so it must be a local variable, and minwidth and minheight are defined as

# src/container.jl line 128 - 129
minwidth(cont::Container) = cont.minwidth
minheight(cont::Container) = cont.minheight

which is are both getters (and not setters) on mutable struct Context <: Container.

[rikhuijzer]

When applying this PR, all tests still pass on Julia 1. Julia 1.0 fails on the compatibility requirements. So, I'd say this refactor is good to go. Is there anything I can do?

@bjarthur Are the problems with Cairo also the reason why Gadfly and Compose development seems to be slowed down? I think Gadfly is the best Julia plotting library, so its a bit weird that development slowed down

[bjarthur]

@Mattriks @tlnagy thoughts on this PR? i'm not familiar enough with this section of code to know if there are unforseen ramifcations.

@rikhuijzer did the Gadfly tests also pass? the actual tests of Compose are quite limited, and we really rely mostly on Gadfly's tests to ensure Compose works. in particular, the regression tests.

re. dev slowing down. @Mattriks has been carrying the torch mostly lately. i doubt cairo is an issue. i actually mostly use the SVG backend which is pure julia and no cairo, in part because of the interactivity. i too prefer gadfly over the others (though i love UnicodePlots!). most people like Plots though, though i wonder whether that is simply because the name implies that it's officially endorsed. would be great to have more contributions from you!

[Mattriks]

The table-jump.jl code is not used (see these lines) so changing it seems a moot point. I'll have a look at the table.jl code sometime soon to see if those lines could be improved.

Gadfly development has only slowed down because I've been busy with other work over the past 4 months (see Gadfly commits). Hoping to get back to it in the new year!

[Mattriks]

My assessment is that in table.jl there's a typo. This line should be:

feasible = feasible && issatisfied(ctx, w_solution[j], h_solution[i])

That way, feasible = true until issatisfied(....) = false, then feasible remains false, triggering the warning after the loop (updated):

feasible || @warn "Graphic may not be drawn correctly at the given size."

In Gadfly, I was able to trigger this warning when I made a plot too small:

p = plot(x=1:5, y=rand(5), Geom.line)
draw(PNG(4cm, 4cm), p) # works
draw(PNG(3cm, 3cm), p) # warning

[rikhuijzer]

@bjarthur For me, tests for Gadfly.jl and Cairo.jl fail locally because Cairo fails to precompile. This is probably because I'm running NixOS. I still have to find a workaround for this, and am planning to set-up a (Docker) dev container, which I'm not particularly fond of.

would be great to have more contributions from you!

Thank you. I must say that PRs like this are helping me in understanding Gadfly/Compose better. Thank you also for pointing me towards the documentation.

@Mattriks thanks for looking into it in detail! Now the code makes much more sense.

I've made the PR such that it includes the fix as offered by Mattriks. If merging, please do squash because the commits are a mess again :smile:

[Mattriks]

I ran the Gadfly tests with this PR and found 8 testscripts which generate the warning, mainly due to the size of the test plots being slightly undersized.

Here's one that looks ok to me, but still generates the warning:

p5 = evalfile(dirname(pathof(Gadfly))*"/../test/testscripts/scale_alpha_discrete.jl")
draw(PNG(), p5)

However the warning does seem to hint at some uncertainty!

I also noticed in Jupyter that when I don't include the draw statement, the warning is printed in triple (I don't know the reason). I wonder if the warning needs to add some helpful info about resizing?

[rikhuijzer]

I won't be able to work on this PR today. If you want, you should be able to edit my changes directly (I've set the checkmark in GitHub to allow edits by maintainers.) Alternatively, close this PR and create your own one :) Please don't let me being unavailable today hinder your work :smile:

[lobingera]

@rikhuijzer I read this just by coincidence, could you please file an issue to Cairo.jl on precompilation.