removes Note: zip::zip() is deprecated, please use zip::zipr() instead

[pachadotdev]

No description provided.

[codecov]

Codecov Report

Merging #458 into master will not change coverage.
The diff coverage is 55.55%.

@@           Coverage Diff           @@
##           master     #458   +/-   ##
=======================================
  Coverage   60.23%   60.23%           
=======================================
  Files          30       30           
  Lines        7142     7142           
=======================================
  Hits         4302     4302           
  Misses       2840     2840

Impacted Files	Coverage Δ
R/writexlsx.R	59.42% <55.55%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ead0038...78094d5. Read the comment docs.

[mgavin]

You'll have to also change the full.names and include.dirs options to zipr to TRUE.
help(zip::zipr) says...

For ‘zipr’ (and ‘zipr_append’), each specified file or directory
in files is created as a top-level entry in the zip archive.

while...

For ‘zip’ (and ‘zip_append’), the root of the archive is supposed
to be the current working directory. The paths of the files are
fully kept in the archive. Absolute paths are also kept.

so line 562 in WorkbookClass.R should be
zip::zipr(zipfile = tmpFile, files = list.files(tmpDir, full.names = TRUE, recursive = TRUE, include.dirs = TRUE, all.files = TRUE), compression_level = cl)

otherwise it comes out a mess

[mgavin]

Furthermore, it affects reading the file after it's saved...
Yeah, this pull request ain't working out for me on Ubuntu 18.04.1, LibreOffice Calc, and

platform x86_64-pc-linux-gnu
version.string R version 3.5.3 (2019-03-11)
nickname Great Truth

[pachadotdev]

hi @mgavin, I'm using this modification in a server (Ubuntu Server 18.04) to export the output to xlsx, and it show no problem to read the downloaded file in LibreOffice and Microsoft Excel. I'll dig the problem a bit more

[mgavin]

I think it was a multitude of things. Like, when looking at the underlying xml that gets zipped up into an xlsx, my sheet1.xml had a tag with an attribute like "maxcols = 1025", and then openxlsx would turn that into 1025 column tags... So I'm thinking things got lost in translation, and zipping the xml back up turned it into something unreadable.

But I think the list.files options should be changed, because zip kept the relative pathing, while zipr makes every file/directory a top level entry.

from help(zip::zip)

The different between 'zipr' and 'zip' is how they handle the relative paths of the input files.

to test: (without zipr)

> library(openxlsx)
> wb <- createWorkbook()
> saveWorkbook(wb, "sampleWorkbook", overwrite = TRUE)
Warning: Workbook does not contain any worksheets. A worksheet will be added.
Note: zip::zip() is deprecated, please use zip::zipr() instead
> zip::zip_list("sampleWorkbook")$filename
 [1] "[Content_Types].xml"                    
 [2] "_rels/.rels"                            
 [3] "docProps/app.xml"                       
 [4] "docProps/core.xml"                      
 [5] "xl/_rels/workbook.xml.rels"             
 [6] "xl/printerSettings/printerSettings1.bin"
 [7] "xl/styles.xml"                          
 [8] "xl/theme/theme1.xml"                    
 [9] "xl/workbook.xml"                        
[10] "xl/worksheets/_rels/sheet1.xml.rels"    
[11] "xl/worksheets/sheet1.xml"               

(with zipr, as in the commit)

> library(openxlsx)
> wb <- createWorkbook()
> saveWorkbook(wb, "sampleWorkbook2")
Warning: Workbook does not contain any worksheets. A worksheet will be added.
> zip::zip_list("sampleWorkbook2")$filename
 [1] "[Content_Types].xml"  ".rels"                "app.xml"             
 [4] "core.xml"             "workbook.xml.rels"    "printerSettings1.bin"
 [7] "styles.xml"           "theme1.xml"           "workbook.xml"        
[10] "sheet1.xml.rels"      "sheet1.xml"          

(and with zipr, with the changes to the options)
Actually, while I was testing this, I got a different list of stuff, even from the original.
SO, to get the original list of files (to how zip::zip put in the files), just make the line
zip::zipr(zipfile = tmpFile, files = list.files(tmpDir, full.names = FALSE, recursive = FALSE, include.dirs = FALSE, all.files=TRUE, no.. = TRUE), compression_level = cl)
changing recursive to FALSE, and no.. to TRUE

> library(openxlsx); wb <- createWorkbook(); saveWorkbook(wb, "sampleWorkbook11"); zip::zip_list("sampleWorkbook11")$filename
Warning: Workbook does not contain any worksheets. A worksheet will be added.
 [1] "[Content_Types].xml"                    
 [2] "_rels/"                                 
 [3] "_rels/.rels"                            
 [4] "docProps/"                              
 [5] "docProps/app.xml"                       
 [6] "docProps/core.xml"                      
 [7] "xl/"                                    
 [8] "xl/_rels/"                              
 [9] "xl/_rels/workbook.xml.rels"             
[10] "xl/drawings/"                           
[11] "xl/drawings/_rels/"                     
[12] "xl/printerSettings/"                    
[13] "xl/printerSettings/printerSettings1.bin"
[14] "xl/styles.xml"                          
[15] "xl/tables/"                             
[16] "xl/tables/_rels/"                       
[17] "xl/theme/"                              
[18] "xl/theme/theme1.xml"                    
[19] "xl/workbook.xml"                        
[20] "xl/worksheets/"                         
[21] "xl/worksheets/_rels/"                   
[22] "xl/worksheets/_rels/sheet1.xml.rels"    
[23] "xl/worksheets/sheet1.xml"               

Okay, the lists aren't exactly the same... but the paths are there.
Conclusion: The options should be changed