Skip to content

Conversation

@JanMarvin
Copy link
Contributor

@JanMarvin JanMarvin commented Aug 2, 2025

Hello @cbailiss ,

thanks for creating the package! I stumbled upon it some time ago and while I reference it in the openxlsx2 book, I haven't had a chance to look into the code a lot. Since pivottabler uses openxlsx— which I maintain, but only to keep it alive for its many dependencies on CRAN—and this package is somehow out of date, I created this PR to switch the backend to openxlsx2.

A major difference between openxlsx and openxlsx2 is the removal of the styles object. In openxlsx2 all styles live in the styles manager and there are different underlying functions to do style related things, e.g. wb_add_font(), wb_add_fill(). To keep the changes minimal, I added a function to apply all styles in PivotOpenXlsxStyle.

This way it should be possible to simply provide functions for both backends, openxlsx and openxlsx2 if you want this. I understand that switching dependencies as suggested in this PR is a major change and if you have any concerns, please let me know, or simply close the PR.

I did some testing with the styling vignette and writing styles seems to work even with the most complicated one: http://www.pivottabler.org.uk/articles/v07-styling.html

[edit] tried to fix the wording.

@cbailiss
Copy link
Owner

cbailiss commented Aug 4, 2025

Hello @JanMarvin
Thanks for raising the PR - very kind of you to do the work here. I will review this coming weekend.

@JanMarvin
Copy link
Contributor Author

Thanks!

There was one thing I wasn’t sure about, the column width and row height appear not to be set by default. Maybe I missed something, if not, it should be possible to set it with a conversion factor from the html width to spreadsheet size

@JanMarvin
Copy link
Contributor Author

FYI: In the discussion in the linked issue, there is a more general helper for conversion of openxlsx style objects to openxlsx2. But I guess this level of detail is not required here, because the creation of the openxlsx styles is happening by pivottabler and using just parts of what is possible with openxlsx.

@cbailiss cbailiss merged commit 1017413 into cbailiss:master Aug 10, 2025
4 checks passed
@JanMarvin JanMarvin deleted the openxlsx2 branch August 10, 2025 21:10
@cbailiss
Copy link
Owner

Thanks @JanMarvin - in principle I would prefer to use the current version of your package, i.e. openxlsx2. So I've merged this PR though I do have a couple of questions:

Q1. Styles
I note you said the styles object has been removed. I was originally using the openxlsx style object to enable reuse of styles across different cells assuming this was more efficient/ran quicker. I can't remember if I actually checked whether that approach was quicker because it was so long ago. However, I see the new apply_style() function is setting each of the style properties explicitly every time it is called - so I am wondering, is openxlsxStyle$apply_style() going to be slower than the older openxlsx::addStyle()?
Or is openxlsxStyle$apply_style() now doing explicitly the same thing that openxlsx::addStyle() was previously doing internally so likely no difference in performance?

Q2. Column Width
The default column width in a worksheet previously was 80 pixels, now it appears to be 64 pixels. Is there a way to easily set the old default? I am not necessarily saying 80 is better, but users of the pivottabler package may have code that expects 80 to be the default.

@JanMarvin
Copy link
Contributor Author

Hi @cbailiss ,

thanks for reaching out. It is a bit late where I live, but I can go into detail later.

Q1: Honestly, no idea what openxlsx does internally. I might have known some time ago, but it has been quite some time since I last looked into the details. But I agree, the styles of the object were written when the file was saved. Therefore, yes I assume that it would have been quicker. openxlsx2 does some internal checks to see if a style is already available in the worksheet, but even constructing a style and ditching it moments later, requires a little bit of time.
But when writing the PR, I was wondering, if the loop over the cells is really necessary (my TODO comment). All the cell and layout information should be available in the moment the table is written to the spreadsheet. I didn't want to rewrite the remaining parts, because ... time :) - no, because I wanted to simply allow adding openxlsx as a legacy backend too. At the moment a check of the incoming workbook format should be enough. If you do not want to keep openxlsx around, I would suggest something along the lines of this:

  1. check for unique styles (table header, different cell styles)
  2. write the table first
  3. write the styles (it should be enough to loop over the unique styles and apply them in a single call to the cells needed. it should be possible to construct a wb_dims() object using the column and row information, e.g. "A1,B2,C3")
  4. add additional layout, e.g. merged cells
    Let me know if I should look into it some time later this week.

Q2: Sure, we can set the column width. This is possible irrespective of the now slimmer column width in openxlsx2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants