From 8f9edeeddc11537dcc84af8e08ed8424a5623f1b Mon Sep 17 00:00:00 2001 From: dipterix Date: Tue, 5 Nov 2024 11:08:00 -0500 Subject: [PATCH] Address `CRAN` issues --- .Rbuildignore | 4 ++-- R/class_electrode_proto.R | 3 ++- R/ext_media.R | 10 ++++---- R/plot_volume-slices.R | 10 +++++--- cran-comments.md | 48 ++++++++++++++++++++++++++++++++++++--- 5 files changed, 61 insertions(+), 14 deletions(-) diff --git a/.Rbuildignore b/.Rbuildignore index 0144adb3..f4545563 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -21,8 +21,8 @@ ^inst/threeBrainJS/old ^inst/threeBrainJS/src ^inst/threeBrainJS/dist/[0-9]+.threebrain-[a-zA-Z0-9]+.js -^inst/threeBrainJS/dist/threebrain-main\.js\.mapp$ -^inst/threeBrainJS/dist/threebrain-worker\.js\.mapp$ +^inst/threeBrainJS/dist/threebrain-main\.js\.map$ +^inst/threeBrainJS/dist/threebrain-worker\.js\.map$ ^inst/threeBrainJS/build\.sh ^inst/threeBrainJS/package ^inst/threeBrainJS/webpack diff --git a/R/class_electrode_proto.R b/R/class_electrode_proto.R index a6980204..e45d6fe2 100644 --- a/R/class_electrode_proto.R +++ b/R/class_electrode_proto.R @@ -511,7 +511,8 @@ ElectrodePrototype <- R6::R6Class( if(with_texture && !is.null(self$.last_texture)) { texture_file <- file.path(tempdir(check = TRUE), sprintf("%s.png", private$id)) grDevices::png(filename = texture_file, units = "px", width = 256, height = 256) - graphics::par(mar = c(0,0,0,0)) + oldpar <- graphics::par(mar = c(0, 0, 0, 0)) + on.exit({ graphics::par(oldpar) }) self$preview_texture(...) grDevices::dev.off() texture_file <- normalizePath(texture_file) diff --git a/R/ext_media.R b/R/ext_media.R index 7ea782d4..b8675fe0 100644 --- a/R/ext_media.R +++ b/R/ext_media.R @@ -33,11 +33,11 @@ video_content <- function(path, duration = Inf, time_start = 0, asp_ratio = 16 / if( local ){ # try to download video because path is probably an URL url <- path - timeout <- getOption("timeout", 60) - on.exit({ - options("timeout" = timeout) - }) - options("timeout" = 6000) + + # 60s is too short to download a video + oldopt <- options("timeout" = 6000) + on.exit({ options(oldopt) }) + path <- tempfile(fileext = '.mp4') utils::download.file(url, destfile = path) temp <- TRUE diff --git a/R/plot_volume-slices.R b/R/plot_volume-slices.R index 39a8cb34..a72dad63 100644 --- a/R/plot_volume-slices.R +++ b/R/plot_volume-slices.R @@ -54,6 +54,10 @@ plot_slices <- function( # overlays <- list.files(root_path, pattern = "\\.nii.gz$", full.names = TRUE) # overlay_alpha <- 0.3 + # Make sure `par` is reset on exit + oldpar <- graphics::par(no.readonly = TRUE) + on.exit({ graphics::par(oldpar) }) + title_position <- match.arg(title_position) if( is.character(volume) ) { @@ -182,8 +186,6 @@ plot_slices <- function( pos <- rbind(t(as.matrix(expand.grid(x, x, KEEP.OUT.ATTRS = FALSE))), 0, 1) - oldpar <- graphics::par(no.readonly = TRUE) - if(!length(nc) || is.na(nc[[1]])) { nc <- grDevices::n2mfrow(npts, asp = 1/n_plots)[[2]] } else { @@ -223,6 +225,7 @@ plot_slices <- function( padding_top <- 0.8 } + # The function calls on.exit({ graphics::par(oldpar) }) so no need to reset here graphics::par( bg = pal[[1]], fg = pal[[length(pal)]], @@ -230,7 +233,6 @@ plot_slices <- function( col.axis = pal[[1]], mar = c(0,0,0,0) ) - on.exit({ do.call(graphics::par, oldpar) }) } # Calculate plt @@ -244,6 +246,8 @@ plot_slices <- function( ratio <- pin[[1]] / pin[[2]] plt <- c( 0, 1, 0.5 - ratio / 2, 0.5 + ratio / 2 ) } + + # The function calls on.exit({ graphics::par(oldpar) }) so no need to reset here adjust_plt <- function(reset = FALSE) { if( reset ) { graphics::par("plt" = c(0, 1, 0, 1)) diff --git a/cran-comments.md b/cran-comments.md index de656525..18ee5301 100644 --- a/cran-comments.md +++ b/cran-comments.md @@ -3,7 +3,49 @@ ## 2024-11-01 **Version 1.2.0 (current)** -To address the `CRAN` issues: +To address the `CRAN` issues (2024-11-05) + +``` +\dontrun{} should only be used if the example really cannot be executed (e.g. because of missing additional software, missing API keys, ...) by the user. That's why wrapping examples in \dontrun{} adds the comment ("# Not run:") as a warning for the user. Does not seem necessary. Please replace \dontrun with \donttest. +Please unwrap the examples if they are executable in < 5 sec, or replace dontrun{} with \donttest{}. +-> create_group.Rd; freesurfer_brain.Rd; geom_freemesh.Rd +``` + +Thanks, currently there are three `\dontrun{}` because these examples do require users to + +* download additional data and software that are not licensed under, nor built into this package. +* run the code in interactive sessions + +I have also tried my best to provide toy-examples if possible. This package originally has lots of `dontrun`s and I have converted most of them to `donttest` back in version `0.1.2` (see comments down below). I believe these three `dontrun` cases have been left since then. + + +``` +Please make sure that you do not change the user's options, par or working directory. If you really have to do so within functions, please ensure with an *immediate* call of on.exit() that the settings are reset when the function is exited. +e.g.: +... +oldpar <- par(no.readonly = TRUE) # code line i +on.exit(par(oldpar)) # code line i + 1 +... +par(mfrow=c(2,2)) # somewhere after +... +e.g.: -> R/class_electrode_proto.R; R/ext_media.R; R/plot_volume-slices.R +If you're not familiar with the function, please check ?on.exit. This function makes it possible to restore options before exiting a function even if the function breaks. Therefore it needs to be called immediately after the option change within a function. +``` + +Thanks, I have added `on.exit` to all functions that change `options` and `par`. + +Just in case you miss it, there are multiple lines changing `par` in the function `plot_slices` (`R/plot_volume-slices.R`), and I make sure the `par` remain unchanged via the following two lines at the very beginning. + +```r +oldpar <- graphics::par(no.readonly = TRUE) +on.exit({ graphics::par(oldpar) }) +``` + + + + + +To address the previous `CRAN` issues (2024-11-01) ``` URL: https://cran.r-project.org/web/packages/threeBrain/index.html @@ -22,7 +64,7 @@ Package `ravetools` has been updated and on `CRAN` now. The dependency is cleare This package (`threeBrain`) is developed out of fun and used in my thesis and later projects. I am the solo developer in this project (wrote 99.99% code). Other contributors are explicitly claimed in the `DESCRIPTION`. -There is one external `JavaScript` library `three-brain-js`. The code is located at `inst/threeBrainJS`. I am also the main maintainer and contributor of that project. The distribution included is a compiled bundle that is released under `MPL-2.0` as a whole. As required, the license file has been included when `mpn` compiles the bundles, see `inst/threeBrainJS/dist`. There might some other external programs, but they can't claim the authorship of the release bundle. Their corresponding license files are included too. +There is one external `JavaScript` library `three-brain-js`. The code is located at `inst/threeBrainJS`. I am also the main maintainer and contributor of that project. The distribution included is a compiled bundle that is released under `MPL-2.0` as a whole. As required, the license file has been included when `npm` compiles the bundles, see `inst/threeBrainJS/dist`. There might some other external programs, but they can't claim the authorship of the release bundle. Their corresponding license files are included too. Self check: 0 errors | 0 warnings | 1 note @@ -130,7 +172,7 @@ Please fix and resubmit. #### Solution: -* exported internal functions needed by exaples +* exported internal functions needed by examples * changed dontrun to donttest