Prevent strwidth() and par() calls from leaking graphics devices

[nanxstats]

Fixes #227

This is new try (after PR #228) to fix the issue reported in #227 where calls to graphics::strwidth() and graphics::par() unintentionally generate Rplots.pdf files in some environments due to opening a new default graphics device that is never closed.

The discussion in that PR was very helpful in getting this "more proper" solution.

• Add a with_graphics_device() helper to run graphics::strwidth()/graphics::par() without implicitly leaving the default device open by opening a null-output device only when needed and closing it on exit.
• Refactor code under R/ and tests/ to use the helper.

Performance comparison:

microbenchmark::microbenchmark(
  r2rtf:::with_graphics_device(graphics::strwidth("Hello world!", units = "inches", cex = 1, font = 1, family = "sans")),
  graphics::strwidth("Hello world!", units = "inches", cex = 1, font = 1, family = "sans"),
  times = 10000
)

#>   min    lq     mean median    uq    max neval cld
#> 4.305 4.715 5.132995  5.002 5.371 33.579 10000   a
#> 1.968 2.214 2.418569  2.337 2.501 26.855 10000   b

Note that changing this might affect some strwidth() calculation results slightly.

[Copilot]

Pull request overview

This PR addresses issue #227 by preventing unintended Rplots.pdf file generation when graphics::strwidth() and graphics::par() are called without an active graphics device. The solution introduces a helper function that manages graphics device lifecycle by opening a null-output device only when needed and ensuring it's closed on exit.

Key changes:

• Introduces a with_graphics_device() helper that wraps graphics function calls to prevent device leaks
• Refactors all graphics::strwidth() and graphics::par() calls in R code and tests to use the new helper
• Removes Rplots.pdf from .gitignore since it should no longer be generated

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
R/safe_graphics.R	New file implementing device lifecycle management helpers: with_graphics_device(), open_null_device(), close_device(), and file_null()
R/rtf_strwidth.R	Updated to wrap graphics::par() and graphics::strwidth() calls with with_graphics_device()
tests/testthat/test-independent-testing-rtf_strwidth.R	Updated test code to wrap graphics::strwidth() calls with with_graphics_device()
tests/testthat/test-independent-testing-rtf_nrow.R	Updated test code to wrap graphics::strwidth() calls with with_graphics_device()
.gitignore	Removed Rplots.pdf entry (no longer needed) and sync_bitbucket.R (appears unrelated)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[elong0527]

Cool, thanks for fixing the issue!

[yihui]

I need more time to understand the rtf_strwidth() function and will review the PR tomorrow.

[yihui]

Just to make sure I understand rtf_strwidth() correctly: this function measures the width of strings under different settings of cex, font, and family. It seems that the only possible way to do it in base R is through a graphical device. My question is, is this measurement dependent on the current device? My intuition is no (and I hope so, otherwise we'll be in trouble). If that's the case, I don't quite understand the complexity of this PR. Why not just

pdf(NULL)
on.exit(dev.off())

in rtf_strwidth()?

[yihui]

is this measurement dependent on the current device? My intuition is no (and I hope so, otherwise we'll be in trouble)

Unfortunately, a quick test shows that I was wrong:

> png()
> strwidth('hello world', 'inches', family = 'Times')
[1] 0.7637533
> dev.off()

> pdf()
> strwidth('hello world', 'inches', family = 'Times')
[1] 0.7621667
> dev.off()

That means rtf_strwidth() is dependent on the current device, which will make its output unpredictable...

[yihui]

BTW, although I don't think any of you would consider switching to another testing framework, I'd like to mention that I also ran into this problem several years ago and provided an option in {testit} to automatically clean up new files generated from tests (including this Rplots.pdf): https://github.com/yihui/testit/blob/1f11885b/R/testit.R#L168-L172

[nanxstats]

Yes, it seems clear that the original intention for strwidth() and par() is for users to only use them within a graphical device context. So a simple solution like pdf()-dev.off() would make sense - although that would change the existing calculations a bit, which may impact some line breaking and pagination results. Maybe this is acceptable.

And yes, the current output is dependent on the graphical device context and thus unpredictable. The goal of this PR is to eliminate the annoying Rplot.pdf behavior but not fixing things fundamentally - because that would involve either introducing systemfonts::string_width() as a hard dependency (the target is zero), or re-implementing a similar function from scratch by adding SystemRequirements: freetype2 and writing C wrappers. I can try that too, but it will add additional burdens of compiling this package from source.

Theoretically, calculating this should NOT require a graphical device. The parameters that truly matter are the font, font size, and DPI. See a proper solution in rtflite (Python) calling Pillow's ImageFont.getlength method which ultimately uses FreeType to do this calculation.

Here is a table comparing these possible solutions:

Dimension	PR#285	pdf()	FreeType2	systemfonts
Avoids Rplots.pdf leak	Yes	Yes	Yes	Yes
Backward compatible results	Yes	No	No	No
Absolutely correct results	No	No	Yes	Yes
Predictable across environments	No	No	Yes	Yes
Zero dependencies	Yes	Yes	Yes	No
No compile-time burden	Yes	Yes	No	No

[nanxstats]

I'm merging this as this seems to be the optimal solution at the moment without moving mountains.

[yihui]

After thinking a bit more on this, I feel we should make rtf_strwidth() independent to the current device (since otherwise the results would be unpredictable). One way to do that is to find the single most accurate device (in base R) in terms of string width measurements and lock to it (i.e. open that device before measuring, and close it afterwards). I think we are currently implicitly assuming pdf() in our tests because it's R's default device, but I'm not sure if it's the best one. By accuracy, I mean using systemfonts::string_width() as the benchmark.

If pdf() turns out to be the best one, we are all good (and this PR can be simplified). If not, I'm not sure what we should introduce a slightly breaking change next.

[nanxstats]

That would be an improvement and a good vibe coding project.

Besides predictable results, if we also care about correct results, we can combine the systemfonts/FreeType core facility and calculate them with the mapped metric-compatible fonts in rtflite. strwidth() is a less ideal starting point for this due to the lack of font support (locked-in by Hershey fonts).