refactor: setup_device()

[vincentarelbundock]

The tinyplot() function is currently over 1100 lines long, and I think it would be nice to add new features.

Would you be open to a few small PRs like this one, to split up the function in smaller, more manageable blocks?

[grantmcdermott]

Yeah, it has gotten unwieldy. I'm definitely open to PRs like this, but I will say that I've found the plot system is sensitive to environmental scoping (since everything is drawn immediately). Reworking draw-legend.R into a standalone function was quite a pain, for example, and I had to introduce several auxiliary flags to make sure things were being passed correctly in and out.

So caveat emptor. But again, please free to go for it.

[vincentarelbundock]

Yeah, I can image this being fiddly.

Do you have some impressionistic sense of how much I can trust the test suite. For example, with this PR I tried 3-4 interactive graphs, 3-4 save to files, and the test suite passes. Is that enough?

[grantmcdermott]

Do you have some impressionistic sense of how much I can trust the test suite. For example, with this PR I tried 3-4 interactive graphs, 3-4 save to files, and the test suite passes. Is that enough?

Test coverage is pretty thorough, albeit only implemented for Linux (as you probably recall).

In fact, the only place I'd say there are gaps is with the file writing/saving logic, i.e. partly what you're adjusting here :-/
I just wasn't sure how to effectively incorporate this into the tinysnapshot workflow.

Maybe we could try something simple like the following... We'd need to include {png} in Suggests and then add the following test to inst/tinytest/test-misc.R:

# test file saving and dimensions
if (requireNamespace("png", quitely = TRUE)) {
  f = function () {
    tmp_path = tempfile(fileext = ".png")
    tinyplot(
      Sepal.Length ~ Petal.Length, data = iris,
      file = tmp_path, width = 4, height = 4
    )
    obj = png::readPNG(tmp_path, info = TRUE)
    unlink(tmp_path)
    dims = attr(obj, "dim")
    return(dims)
  }
  expect_equal(f(), c(1200, 1200, 4), label = "png_size")
}

Do you mind incorporating this change as part of your PR? I'm happy to merge, assuming this new test passes and given your manual checks too.

[grantmcdermott]

Thanks!

[zeileis]

Thanks to both of you for the all the work and active discussions. I just returned after being away with the family for the weekend: Are there any issues/PRs I can still help with?

Modularizing the building blocks of tinyplot() would indeed be great. Then I would like to contribute a type = "spine" as I mentioned a couple of times. Regarding the problems with environments and passing around certain arguments or attributes: We could set up a dedicated environment within the package (say tinyplot_env) and

• Empty the environment at the beginning (possibly unless add = TRUE).
• Copy data and other variables there.
• Make all modifications there.
• Use all the prepared information for drawing at the end.

Maybe this could even be taken to the point where we can store a copy of the environment and re-plot it later. But I haven't thought this through.