improving multirow handling in collapse_rows()

[unDocUMeantIt]

the current implementation of valign="top" resulted in totally distorted PDF tables on our part when collapsing two columns simultanously, with many lables being printed way to high. using row_group_label_position="first" as an alternative solved that issue but was ignored when producing HTML tables with the same script code.

the proposed patch produces simpler LaTeX code that solved the issue with valign="top" on our end. the PDF tables look like the ones rendered with row_group_label_position="first".

[dmurdoch]

Could you add a simple example showing the issue? This could be added in a comment here, or possibly in a test in tests/testthat in your PR (though the tests don't generally get rendered, so that might not be so useful).

[unDocUMeantIt]

i actually had reported the underlying issue a while ago, so this should fix #671 i stand corrected. i'll produce an example ;)

[dmurdoch]

Regarding #671: would it be possible to also fix that, or are these completely separate issues?

[unDocUMeantIt]

i'll look into this. actually, i'm puzzled right now and will need to investigate why #671 is behaving differently from the tables we had fixed with this patch.

FWIW, the problem occurred when using businessPlanR::kable_bpR() on transaction objects. unfortunately, i can't copy&paste the large tables from the actual business plan, but i'll try to replicate an example that shows the issue.

[unDocUMeantIt]

as it turns out, the example from #671 is actually capable of demonstrating the issue i was addressing with this patch: the final example, which used to produce a correct table, now places the first column lables too high (see below).

i have extended the example to include both latex_hline="full" and a custom hline:

---
#title: kableExtra demo
#latex_engine: pdflatex
output:
  pdf_document:
    latex_engine: pdflatex
    extra_dependencies: ["array", "booktabs", "colortbl", "float", "longtable", "multirow", "xcolor"]
---

```{r setup, include=FALSE}
library(kableExtra)

x <- data.frame(
    A=rep(letters[1:2], each=10),
    B=rep(letters[3:12], each=2),
    C=1:20
)
# we'll reuse this basic kable object
x_kbl <- kbl(
    x,
    format="latex",
    booktabs=TRUE,
    escape=FALSE
)
```

# `collapse_rows()` after `kable_styling()`

It fails completely:

```{r, echo=FALSE}
x_kbl_kc <- kable_styling(
    x_kbl,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_kc <- collapse_rows(
    x_kbl_kc,
    columns=1:2,
    valign="top"
)
x_kbl_kc
```

\newpage

# `collapse_rows()` before `kable_styling()`, `latex_hline="full"`

Almost works, but top alignment collapsed rows is off:

```{r, echo=FALSE}
x_kbl_ck <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top"
)
x_kbl_ck <- kable_styling(
    x_kbl_ck,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_ck
```

The odd allignment is due to the fact that `multirow` is being thrown off by extra space added between data rows by lines.

\newpage

# `collapse_rows()` before `kable_styling()`, custom hline 2

This works fine:

```{r, echo=FALSE}
x_kbl_ck <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top",
    latex_hline="custom",
    custom_latex_hline=2
)
x_kbl_ck <- kable_styling(
    x_kbl_ck,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_ck
```

\newpage

# `collapse_rows()` without hline

Used to fixes top alignment in earlier versions of kableExtra, but now shows exactly the format error described in https://github.com/haozhu233/kableExtra/pull/851 :

```{r, echo=FALSE}
x_kbl_nh <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top",
    latex_hline="none"
)
x_kbl_nh <- kable_styling(
    x_kbl_nh,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_nh
```

to run it:

rmarkdown::render(
  input="demo.rmd",
  output_format=NULL,
  output_file="demo.pdf",
  output_dir="/tmp",
  intermediates_dir="/tmp",
  clean=FALSE
)

here's some comparing screenshots of the interesing pages. on the left (first) side are the results if rendered with kableExtra version 1.4.0, on the right side the results rendered with this patch applied:

it clearly shows the problem and also that the patch apparently fixed it.

unfortunately, while it solves this particular problem, it also intruduces a new one -- current kableExtra seems to deal well with the table if custom hlines are requested grouping by column 2 only, but the patch takes this away:

finally, both current kableExtra and the patched version still fail to create well fomatted tables in latex_hline="full" condition, to varying degrees (but both suck):

so maybe a revised patch could solve the problems seen in the first to examples, i.e. use the LaTeX code from the patch if latex_hline="none" and keep the unpatched code otherwise? it won't solve all possible issues, but at least the new problems seem to be better (i.e., occuring less often) than the current ones.

[dmurdoch]

I'm not going to be able to look at this in detail for a few days. If nobody has commented on it or merged it in a week, feel free to remind me.

[unDocUMeantIt]

this shouldn't be merged until i made the adjustments i mentioned (apply the new LaTeX code only when latex_hline="none"). no need to hurry.

[unDocUMeantIt]

i hope with the latest commit is now a major improvement to collapse_rows(). i have added collapse_rows(latex_hline="major") and collapse_rows(latex_hline="linespace") to the example and fixed them, too:

---
#title: kableExtra demo
#latex_engine: pdflatex
output:
  pdf_document:
    latex_engine: pdflatex
    extra_dependencies: ["array", "booktabs", "colortbl", "float", "longtable", "multirow", "xcolor"]
---

```{r setup, include=FALSE}
library(kableExtra)

x <- data.frame(
    A=rep(letters[1:2], each=10),
    B=rep(letters[3:12], each=2),
    C=1:20
)
# we'll reuse this basic kable object
x_kbl <- kbl(
    x,
    format="latex",
    booktabs=TRUE,
    escape=FALSE
)
```

# `collapse_rows(latex_hline="full")` after `kable_styling()`

```{r, echo=FALSE}
x_kbl_kc <- kable_styling(
    x_kbl,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_kc <- collapse_rows(
    x_kbl_kc,
    columns=1:2,
    valign="top",
    latex_hline="full"
)
x_kbl_kc
```

\newpage

# `collapse_rows(latex_hline="full")` before `kable_styling()`

```{r, echo=FALSE}
x_kbl_ck <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top",
    latex_hline="full"
)
x_kbl_ck <- kable_styling(
    x_kbl_ck,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_ck
```

\newpage

# `collapse_rows(latex_hline="custom", custom_latex_hline=2)` before `kable_styling()`

```{r, echo=FALSE}
x_kbl_ck <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top",
    latex_hline="custom",
    custom_latex_hline=2
)
x_kbl_ck <- kable_styling(
    x_kbl_ck,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_ck
```

\newpage

# `collapse_rows(latex_hline="major")`

```{r, echo=FALSE}
x_kbl_nh <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top",
    latex_hline="major"
)
x_kbl_nh <- kable_styling(
    x_kbl_nh,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_nh
```

\newpage

# `collapse_rows(latex_hline="linespace")`

```{r, echo=FALSE}
x_kbl_nh <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top",
    latex_hline="linespace"
)
x_kbl_nh <- kable_styling(
    x_kbl_nh,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_nh
```

\newpage

# `collapse_rows(latex_hline="none")`

```{r, echo=FALSE}
x_kbl_nh <- collapse_rows(
    x_kbl,
    columns=1:2,
    valign="top",
    latex_hline="none"
)
x_kbl_nh <- kable_styling(
    x_kbl_nh,
    full_width=FALSE,
    latex_options=c("striped", "HOLD_position")
)
x_kbl_nh
```

this is what i get with the patched function:

[unDocUMeantIt]

I'm not going to be able to look at this in detail for a few days. If nobody has commented on it or merged it in a week, feel free to remind me.

i hereby kindly remind you ;)

[dmurdoch]

Thanks for the reminder. Looks like your code passes all our checks.

Could I ask for one more edit? Some of the new lines are quite long; could you add some linebreaks to make them easier to read? Aim for lines around 72 chars long or shorter, and the "Files changed" diff on this page shouldn't need to wrap any lines. Please don't adjust any existing long lines, just the new ones.

In addition, line 333 with the -ifelse(...) code looks likely to confuse; please also take the minus sign into the value part, or out into the text preceding it (if we can assume span - 1 is always positive).

[unDocUMeantIt]

i hope the formatting is ok now. just let me know if you need anything else.