75 protect

[jmbarbone]

related to #75

Also removes a few openxlsx2::: calls while debugging the below. We should need to include those in anything. devtools::load() works well enough to attach all package objects.

I'm also seeing some opportunity to make sure that we initialize the objects correctly. In some cases we use NULL for an initial value and others might be character(). Could be helpful to keep these aligned and make sure when we revert anything that we change the value to the default on initialization. For example, when we unprotect a workbook, do we set the field to "", character() or NULL? This way we have a bit more parity when creating new workbooks and modifying existing ones.

wb_protect() changes

I had to make some changes for $protect() because we were getting different results with the direct method call and the wrapper:

on main

library(openxlsx2)
ts <- Sys.time()
waldo::compare(
  wb_workbook(datetimeCreated = ts) |>  wb_protect(TRUE),
  wbWorkbook$new(datetimeCreated = ts)$protect(TRUE)
)
#> `old$workbook` is length 22
#> `new$workbook` is length 21
#> 
#>      names(old$workbook) | names(new$workbook)     
#> [19] "fileRecoveryPr"    | "fileRecoveryPr"    [19]
#> [20] "webPublishObjects" | "webPublishObjects" [20]
#> [21] "extLst"            | "extLst"            [21]
#> [22] "apps"              -                         
#> 
#> old$workbook$workbookProtection vs new$workbook$workbookProtection
#> - "<workbookProtection lockStructure=\"0\" lockWindows=\"0\"/>"
#> + "<workbookProtection/>"
#> 
#> `old$workbook$apps` is a character vector ('<DocSecurity>1</DocSecurity>')
#> `new$workbook$apps` is absent

^{Created on 2022-05-01 by the reprex package (v2.0.1)}

on pr

library(openxlsx2)
ts <- Sys.time()
waldo::compare(
  wb_workbook(datetimeCreated = ts) |>  wb_protect(TRUE),
  wbWorkbook$new(datetimeCreated = ts)$protect(TRUE)
)
#> v No differences

^{Created on 2022-05-01 by the reprex package (v2.0.1)}

I don't know what exactly caused that, so I just cleaned it up a bit and now they return the same.

wb_protect_worksheet() changes

I couldn't help myself and modified the params. We now have a single properties param that we can list the individual properties to lock. The code is vectorized for checks and formatting. I'm wondering if we may want to add another if statement where when protect = FALSE the properties are reversed so we can unprotect the properties.

[JanMarvin]

agree with the character vs null comment. though we have to check a very lot of functions and not only local, but with Excel too. Sometimes something looks strange on theR side, but is required by Excel for whatever reason.

and enabling Mac and windows builds lead to a hanging Mac build (6+ hours) that used all the remaining minutes 🙈

[jmbarbone]

and enabling Mac and windows builds lead to a hanging Mac build (6+ hours) that used all the remaining minutes 🙈

Oh, fantastic! Maybe we can add a timeout to the actions to prevent this? https://stackoverflow.com/questions/59073731/set-default-timeout-on-github-action-pipeline#59076067.

[JanMarvin]

I'll look into this, the last time this happened it was also caused by the mac ci. The quota will reset in 5 days ...

[JanMarvin]

Had a look at the failing tests and fixed these, but currently protect can cause broken Excels:

For example, when we unprotect a workbook, do we set the field to "", character() or NULL?

wb$workbook is a list. Nulling from the entry and writing back to this list changes the order in which Excel will expect the xml nodes. For workbook and for parts of worksheet, we simply write as xml what is in the list elements. This either leaves us with the option to hardcode the order in which we return values, which might or might not be a good idea. Do an in place replacement of named null or try something entirely different. For now returning character() ensures the correct order of nodes :-\

[JanMarvin]

It appears that there's another problem, wb_protect and wb_protect_worksheet somehow behave different. The latter is a void function and I'm unsure why. Maybe the worksheets need different cloning or deep cloning?

wb <- wb_workbook()
wb$add_worksheet("S1")
write_datatable(wb, 1, x = iris[1:30, ])
wb$worksheets[[1]]$sheetProtection
# Formatting cells / columns is allowed , but inserting / deleting columns is protected:
wb_protect_worksheet(wb, "S1",
                     protect = TRUE,
                     properties = c("formatCells", "formatColumns", "insertColumns", "deleteColumns")
)
wb$worksheets[[1]]$sheetProtection

needs a bit more love

[JanMarvin]

It appears that there's another problem, wb_protect and wb_protect_worksheet somehow behave different. The latter is a void function and I'm unsure why. Maybe the worksheets need different cloning or deep cloning?

wb <- wb_workbook()
wb$add_worksheet("S1")
write_datatable(wb, 1, x = iris[1:30, ])
wb$worksheets[[1]]$sheetProtection
# Formatting cells / columns is allowed , but inserting / deleting columns is protected:
wb_protect_worksheet(wb, "S1",
                     protect = TRUE,
                     properties = c("formatCells", "formatColumns", "insertColumns", "deleteColumns")
)
wb$worksheets[[1]]$sheetProtection

This is indeed fixed with deep cloning. Do we need to apply this to all worksheet touching wrappers?

[JanMarvin]

The remaining issue here is can we keep or enforce a certain order of wb$worksheet[[x]]$... entries after nulling entries such as sheetProtection. If the order is incorrect Excel will complain. This impacts most remove functions and functions that currently have no remove (such as page_break).

Right now I can only think of something like selecting the names, removing the entry, increasing the length of the list (this will add a new null entry at the end), name the new entry and reorder by known names. But it's been a long week already ...

[JanMarvin]

FYI: I'm not entirely happy with this. It creates workbooks that Excel will open without hesitation, but it also limits us to use the structure in this field. If we miss an entry here, even though we import it when loading, we will no longer write it. Either it is in this list or it will be lost.

[JanMarvin]

Added a few minor changes and merged!