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

[WIP] More flexible dims #993

Merged
merged 21 commits into from
Apr 21, 2024
Merged

[WIP] More flexible dims #993

merged 21 commits into from
Apr 21, 2024

Conversation

JanMarvin
Copy link
Owner

Still a little hackish solution to the flexibility problem mentioned in #796 , but it shows what is possible. For whatever reason the enforce option does not work with wb_add_fill() probably because empty cells are still initialized even though they are not filled.

Also there is still a bug, for now I simply assumed that all data columns are beneath each other to fill their positions. In the example below the column header is right to the data and hence the row has to be filled wider, which is not yet implemented. Hence the first data part is filled incorrectly.

library(openxlsx2)

# write a workbook with a few colored cells
wb <- wb_workbook()$
  add_worksheet()$
  add_fill(dims = "A1:B2;C3:D4;E1:F2;G5:H6;I2:K2", color = wb_color("yellow"))

# force a dataset into a specific cell dimension
options("openxlsx2.enforce_dims" = TRUE)
wb$add_data(dims = "I2:J2;A1:B2;G5:H6", x = matrix(1:8, 4, 2))
options("openxlsx2.enforce_dims" = FALSE)

if (interactive()) wb$open()
Screenshot 2024-04-13 at 13 26 30

@JanMarvin JanMarvin added the enhancement 😀 New feature or request label Apr 13, 2024
@JanMarvin JanMarvin added this to the future milestone Apr 13, 2024
src/helper_functions.cpp Outdated Show resolved Hide resolved
@JanMarvin
Copy link
Owner Author

library(openxlsx2)

# write a workbook with a few colored cells
wb <- wb_workbook()$
  add_worksheet()$
  add_fill(dims = "A1:B2;C3:D4;E1:F2;G5:H6;I2:K2", color = wb_color("yellow"))

# force a dataset into a specific cell dimension
options("openxlsx2.enforce_dims" = TRUE)
wb$add_data(dims = "I2:J2;A1:B2;G5:H6", x = matrix(1:8, 4, 2))
options("openxlsx2.enforce_dims" = FALSE)

wb$add_font(dims = "B2;G6;J2", bold = TRUE)

if (interactive()) wb$open()
Screenshot 2024-04-13 at 19 09 48

Not sure if this has any impact on our usual behavior. Enforce only works with data, therefore it might be a good idea to add it as option.

R/wb_functions.R Outdated Show resolved Hide resolved
R/wb_functions.R Outdated Show resolved Hide resolved
R/write.R Show resolved Hide resolved
R/write.R Outdated Show resolved Hide resolved
src/helper_functions.cpp Outdated Show resolved Hide resolved
src/helper_functions.cpp Outdated Show resolved Hide resolved
src/helper_functions.cpp Outdated Show resolved Hide resolved
@JanMarvin JanMarvin force-pushed the more_flexible_dims branch 4 times, most recently from 732b9ad to 3b72547 Compare April 18, 2024 13:46
@JanMarvin
Copy link
Owner Author

@olivroy This PR is closing in, just minor things from my side. It would be nice if the disabled test would work, but at the moment I’m not in the mood to figure this out. Guess we can live without it, at least for the moment. Returning different combinations of (squared) ranges and cells is a little tricky.

But finally we can and probably should extend wb_dims() with this PR. We should be able to select multiple columns from a data frame, but we might be able to extend it even further.

  1. Maybe provide column and row values, but return only specific cells
wb_dims(cols = 1:2, rows = c(1,3), cells = TRUE)
#> “A1,B3”

or

  1. highlight a dataframe based on a condition
wb_dims(x = mtcars[mtcars > 4], cells = TRUE)

Making wb_dims() more flexible has been requested on SO

What might be an issue is speed. I’ve tested this PR only with small dims, only a few cells not hundreds or thousands, but we’ll see…

Copy link
Collaborator

@olivroy olivroy left a comment

Choose a reason for hiding this comment

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

Seems interesting!

So if I understand correctly, dims = "A1;B2:C3" didn't work before this PR?

@@ -216,9 +220,16 @@ write_data2 <- function(
na.strings = na_strings(),
data_table = FALSE,
inline_strings = TRUE,
dims = NULL
dims = NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe adding dims to write_xlsx() could be its own PR if there are too manu moving parts. Would need to check rows, cols, start_row etc. ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting, I wasn't aware that we do not support dims in write_xlsx(), but maybe that's not so important. Presumably people that use write_xlsx() only want a data frame or vector written as xlsx file and do not care that much about styling.

@@ -1059,6 +1097,7 @@ write_data <- function(
remove_cell_style = FALSE,
na.strings = na_strings(),
inline_strings = TRUE,
enforce = FALSE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding another argument everywhere seems a bit verbose..

I don't know if this is good, but possibly via options is better? or

adding it to user facing function save in temp env var, then apply it where needed?

Probably not a good suggestion

Copy link
Owner Author

@JanMarvin JanMarvin Apr 19, 2024

Choose a reason for hiding this comment

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

If I understand you correctly: I made it an argument, because with the option you have to actively set it if needed and it didn’t work with wb_add_fill() either way.

@JanMarvin
Copy link
Owner Author

Seems interesting!

So if I understand correctly, dims = "A1;B2:C3" didn't work before this PR?

Yes previously this didn’t work and now it will work only if you have a scalar and a matrix with 4 values. There are obviously cornerstones that I haven’t figured out yet, but this PR is a huge step in the right direction.

@JanMarvin
Copy link
Owner Author

library(openxlsx2)
wb <- wb_workbook()$add_worksheet()$
  add_data(dims = "A1;B2:C3", x = 1:5, enforce = TRUE)

This works too, but I see the strength of different dim ranges more in evenly stretched columns:

library(openxlsx2)
wb <- wb_workbook()$
  add_worksheet()$
  add_data(dims = "A1", x = mtcars)$
  add_fill(dims = wb_dims(x = mtcars, cols = c("cyl", "wt")), color = wb_color("green"))

Where we previously could only color a single column, we are now able to color both. If wb_dims(x = mtcars, cols = c("cyl", "wt")) returns something like "B2:B33,F2:F33".

@JanMarvin
Copy link
Owner Author

JanMarvin commented Apr 20, 2024

This works:

wb <- wb_workbook()$add_worksheet()$
  add_data(dims = "A1:B5", x = matrix(1:10, ncol = 2),
           enforce = TRUE, col_names = FALSE)

wb <- wb_workbook()$add_worksheet()$
  add_data(dims = "A1:B1;A3:B6", x = matrix(1:10, ncol = 2),
           enforce = TRUE, col_names = FALSE)

This does not work:

wb <- wb_workbook()$add_worksheet()$
  add_data(dims = "A1:A5,B1:B5", x = matrix(1:10, ncol = 2),
           enforce = TRUE, col_names = FALSE)

So if you want to write a data frame or matrix with enforce, make sure that dims is row wise large enough. It's not enough if the data would fit column wise.

These are identical:

wb <- wb_workbook()$add_worksheet()$
  add_data(dims = "A1:B5", x = 1:10,
           enforce = TRUE, col_names = FALSE)

wb <- wb_workbook()$add_worksheet()$
  add_data(dims = "A1:A5,B1:B5", x = 1:10,
           enforce = TRUE, col_names = FALSE)

These not:

wb <- wb_workbook()$add_worksheet()$
  add_data(dims = "A1:B1;A3:B6", x = 1:10,
           enforce = TRUE, col_names = FALSE)

@JanMarvin JanMarvin merged commit 886a141 into main Apr 21, 2024
9 checks passed
@JanMarvin JanMarvin deleted the more_flexible_dims branch April 21, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 😀 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants