Skip to content

feat: change defaults of master and layout arg in add_slide(), add layout_default()(#635) #644

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

markheckmann
Copy link
Contributor

@markheckmann markheckmann commented Mar 29, 2025

Summary

Hi @davidgohel , this PR closes #635:

  1. add_slide(): remove default settings for args layout and master
  2. layout_default(): new function to set a default layout for add_slide()

revdeps: Luckily, there are only 2 revdep problems, both easy to fix (however, 11 pkgs failed to check). Please find the complete revdep results at the bottom. Once this PR is accepted, I am happy to submit patch PRs for both packages (flextable & autoslider.core) affected by the {officer} 0.6.9 release, if you like.

Changes

1. add_slide(): removing default settings for args layout and master

  • add_slide() requires arg layout now
  • arg master is only required, if the layout is not unique across masters

before

x <- read_pptx() |> add_slide() # uses defaults only matching the pptx template

after

x <- read_pptx() |> add_slide()
> Error in `add_slide()`:
> ! `layout` is `NULL`
>Either pass a `layout` or set a default layout via `layout_default()`

Created on 2025-03-29 with reprex v2.0.2

2. layout_default() to set (and unset) a default layout for add_slide()

x <- read_pptx()|> 
  add_slide("Title Slide") |> 
  layout_default("Two Content") |> 
  add_slide() |> 
  add_slide() |> 
  layout_default("Title and Content") |> 
  add_slide() |> 
  add_slide() 

print.rpptx now indicates if a default layout is set:

x
#> pptx document with 5 slides
#> Available layouts and their associated master(s):
#> (*) = Default layout
#>              layout       master  
#> 1       Title Slide Office Theme  
#> 2 Title and Content Office Theme *
#> 3    Section Header Office Theme  
#> 4       Two Content Office Theme  
#> 5        Comparison Office Theme  
#> 6        Title Only Office Theme  
#> 7             Blank Office Theme

Created on 2025-03-29 with reprex v2.0.2

revdep checks

## revdepcheck results

We checked 71 reverse dependencies, comparing R CMD check results across CRAN and dev versions of this package.

 * We saw 2 new problems
 * We failed to check 11 packages

Issues with CRAN packages are summarised below.

### New problems
(This reports the first line of each new failure)

* autoslider.core
  checking examples ... ERROR
  checking tests ...
  checking running R code from vignettes ...

* flextable
  checking examples ... ERROR
  checking tests ...

### Failed to check

* esquisse    (NA) 
* GWSDAT      (NA)
* jsmodule    (NA)
* reappraised (NA)
* reservr     (NA)
* robmedExtra (NA)
* rvg         (NA)
* statgenGWAS (NA)
* TestAnaAPP  (NA)
* ubiquity    (NA)
* visvaR      (NA)

I checked the source code of all packages listed under failed to check manually to see if add_slide() was used without an explicit layout name. This was not the case, so there should be no problems in these packages.

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 98.64865% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.54%. Comparing base (a28db25) to head (d87fe37).

Files with missing lines Patch % Lines
R/read_pptx.R 94.73% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #644      +/-   ##
==========================================
+ Coverage   86.42%   86.54%   +0.11%     
==========================================
  Files          43       43              
  Lines        7235     7283      +48     
==========================================
+ Hits         6253     6303      +50     
+ Misses        982      980       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@davidgohel
Copy link
Owner

@markheckmann thanks, I will not be able to work on the PR this week, hopefully next week

I will try to avoid this breaking changes. We can totally afford changing flextable or autoslider.core but there are lot of scripts in companies that will be affected and we must not break their codes.

@markheckmann
Copy link
Contributor Author

markheckmann commented Apr 12, 2025

okay, then I suggest the following modification to avoid potential breaking changes for user code. @davidgohel Please let me know, in case you have any doubts or further suggestions


New approach

We maintain the current add_slide() behavior - which uses layout = "Title and Content" and master = "Office Theme" as defaults - by setting "Title and Content" as the default layout during read_pptx(), if the layout exists. Then, the behavior of add_slide() would be same as before. The only difference would be, that the layout default is not part of the add_slide() function signature anymore, but is set inside the rpptx object when calling read_pptx().

This approach should not cause any revdep issues and also no problem in user code. I will make the changes and check the former.

To avoid breaking changes in old code, `read_pptx()` automatically sets
the default layout `"Slide and Content"`, if it exists. This makes sure,
that the behavior of `add_slide()` does not change.
@markheckmann
Copy link
Contributor Author

markheckmann commented Apr 12, 2025

Updated PR

Hi @davidgohel , I updated the PR as follows:

Summary

  1. add_slide(): default settings for args layout and master removed, while adding default layout "Title And Content" (if it exists) to rpptx object to keep default behavior of add_slide()
  2. layout_default(): new function to set a remove default layout. The default layout is used by add_slide()
  3. No revdep issue any more. Due to the updates, the previous issues (for flextable and autoslider.core) have disappeared. There should be no breaking changes in user code now.

Changes

1. add_slide(): removing default settings for args layout and master. Makes use of default layout.

  • add_slide() requires arg layout if no default layout is set
  • arg master is only required, if the layout is not unique across masters
  • calling read_pptx()will set layout "Title And Content" as default layout, if it exists. This avoids breaking changes in user code.
library(officer)

# read_pptx() sets default layout  to "Title and Content" (if layout exists like in the pptx template)
x <- read_pptx()

# the default layout is marked by an asterisk ...
x
#> pptx document with 0 slides
#> Available layouts and their associated master(s):
#> (*) = Default layout
#>              layout       master  
#> 1       Title Slide Office Theme  
#> 2 Title and Content Office Theme *
#> 3    Section Header Office Theme  
#> 4       Two Content Office Theme  
#> 5        Comparison Office Theme  
#> 6        Title Only Office Theme  
#> 7             Blank Office Theme

# ... and used by add_slide() so its default behavior remains the same: a slide with layout "Title and Content" is added
x <- x |> add_slide()

# Better error message if layout does not exist
x |> add_slide("xxx")
#> Error:
#> ! Layout "xxx" does not exist
#> ℹ See `layout_summary()` for available layouts.

Created on 2025-04-12 with reprex v2.1.1

2. layout_default() to set (and unset) a default layout used by add_slide()

library(officer)

x <- read_pptx()|>
  add_slide() |>  # uses default layout "Title and Content" => same behavior as before, avoids breaking changes for user
  add_slide("Title Slide") |> # add slide with layout "Title Slide"
  add_slide() |>  # again add slide with default layout "Title and Content"
  layout_default("Comparison") |>  # change default layout to "Comparison"
  add_slide() # add slide with layout "Comparison"

# note that comparison is marked as default now
x
#> pptx document with 4 slides
#> Available layouts and their associated master(s):
#> (*) = Default layout
#>              layout       master  
#> 1       Title Slide Office Theme  
#> 2 Title and Content Office Theme  
#> 3    Section Header Office Theme  
#> 4       Two Content Office Theme  
#> 5        Comparison Office Theme *
#> 6        Title Only Office Theme  
#> 7             Blank Office Theme

x <- x |> layout_default()  # remove default

# Without a default layout, the layout arg is required
x |> add_slide() 
#> Error in `add_slide()`:
#> ! `layout` is `NULL`
#> ✖ Either pass a `layout` or set a default layout via `layout_default()`

# passig a layout adds a slide as usual, if no default is iset
x <- x |> add_slide("Comparison")

Created on 2025-04-12 with reprex v2.1.1

3. revdep checks

The behavior of add_slide() has not changed if a layout named "Title and Content" exists (as in the package pptx template). Hence, there should be not backward compatibility issue.

## revdepcheck results

We checked 73 reverse dependencies, comparing R CMD check results across CRAN and dev versions of this package.

 * We saw 0 new problems
 * We failed to check 4 packages

Issues with CRAN packages are summarised below.

### Failed to check

* esquisse    (NA)
* reservr     (NA)
* rvg         (NA)
* statgenGWAS (NA)

I checked the source code of all packages listed under failed to check manually. There should be no problems in these packages.

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.

feat: change defaults of master and layout arg in add_slide()
3 participants