Skip to content
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

Regression: with bookdown 0.22 example (exm) environment misses the colon (:) between Example and its number in odt, docx and epub #1202

Open
N0rbert opened this issue Jul 9, 2021 · 13 comments · May be fixed by #1206
Assignees
Labels
bug an unexpected problem or unintended behavior next to consider for next release
Milestone

Comments

@N0rbert
Copy link

N0rbert commented Jul 9, 2021

I have the following minimal Rmd bookdown example file named index.Rmd:

---
documentclass: book
site: bookdown::bookdown_site
output:
  bookdown::odt_document2: default
  bookdown::epub_book: default
  bookdown::word_document2: default
---

# Example code listing

See Example \@ref(exm:c-code).

::: {custom-style="ListingCaption"}
```{example, c-code}
Simple C program
```
:::
```c
int main()
{
    exit(0);
}
```

Text after example.

With 0.21 it was rendered as "Example 1.1: Simple C program", but with 0.22 and latest version from github it renders as "Example 1.1 Simple C program". It breaks readability. See image below for odt-output:

problem

I think that it is a regression bug.

Below is the table with Example separators comparison.

Version docx odt epub
0.21 : : :
0.22+ no no no

Please confirm and fix this bug in the next version.


> xfun::session_info('bookdown')
R version 3.4.4 (2018-03-15)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 18.04.5 LTS, RStudio 1.4.1717

Locale:
  LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C               LC_TIME=ru_RU.UTF-8       
  LC_COLLATE=en_US.UTF-8     LC_MONETARY=ru_RU.UTF-8    LC_MESSAGES=en_US.UTF-8   
  LC_PAPER=ru_RU.UTF-8       LC_NAME=C                  LC_ADDRESS=C              
  LC_TELEPHONE=C             LC_MEASUREMENT=ru_RU.UTF-8 LC_IDENTIFICATION=C       

Package version:
  base64enc_0.1.3   bookdown_0.22.11  digest_0.6.27     evaluate_0.14    
  glue_1.4.2        graphics_3.4.4    grDevices_3.4.4   highr_0.9        
  htmltools_0.5.1.1 jsonlite_1.7.2    knitr_1.33        magrittr_2.0.1   
  markdown_1.1      methods_3.4.4     mime_0.11         rlang_0.4.11     
  rmarkdown_2.9     stats_3.4.4       stringi_1.6.2     stringr_1.4.0    
  tinytex_0.32      tools_3.4.4       utils_3.4.4       xfun_0.24        
  yaml_2.2.1       
@cderv
Copy link
Collaborator

cderv commented Jul 12, 2021

Thanks for the report @N0rbert

I have look into where it comes from and the culprit is #1120

Sorry we missed out this and caused a regression.

I'll fix that quickly

@cderv cderv added the bug an unexpected problem or unintended behavior label Jul 12, 2021
@cderv cderv added this to the v0.23 milestone Jul 12, 2021
@cderv
Copy link
Collaborator

cderv commented Jul 12, 2021

I now remember where this change came from. The : was not supposed to show for any of the theorem environment

bookdown:::theorem_abbr
#>     theorem       lemma   corollary proposition  conjecture  definition 
#>       "thm"       "lem"       "cor"       "prp"       "cnj"       "def" 
#>     example    exercise  hypothesis 
#>       "exm"       "exr"       "hyp"

It was an issue in the code initially that was "fixed" during this PR : https://github.com/rstudio/bookdown/pull/1120/files#r605115658

A <span> should have been included in the first place, and the : sep was unset.

I understand this is a breaking change to you.

@yihui do you remember why a : sep was unset specifically for theorem env ?
See change in https://github.com/rstudio/bookdown/pull/1120/files#r605115658
I wonder if we could just use : for non HTML/PDF output in order to not change older output ?

@cderv
Copy link
Collaborator

cderv commented Jul 19, 2021

We got here another case where putting back the : broke something: https://community.rstudio.com/t/how-to-change-the-figure-table-caption-style-in-bookdown/110397/2

@yihui
Copy link
Member

yihui commented Jul 20, 2021

do you remember why a : sep was unset specifically for theorem env ?

@cderv I think it is uncommon to use : in theorem/proof environments: https://bookdown.org/yihui/bookdown/markdown-extensions-by-bookdown.html#theorems In LaTeX/PDF and HTML output, the environment name and number are in bold text. In other output formats, they do not have any special styling, which is probably why @N0rbert said it affected readability. If we can make them bold like PDF and HTML output, I guess it should be much better and more consistent; otherwise we may have to use the following idea of yours as the last resort.

I wonder if we could just use : for non HTML/PDF output in order to not change older output ?

@cderv
Copy link
Collaborator

cderv commented Jul 20, 2021

If we can make them bold like PDF and HTML output,

I think we can do that using markdown syntax by adding ** around the label only for those environment.

diff --git a/R/ebook.R b/R/ebook.R
index 4f38b42e..22974f75 100644
--- a/R/ebook.R
+++ b/R/ebook.R
@@ -116,13 +116,14 @@ resolve_refs_md = function(content, ref_table, to_md = output_md()) {
     for (j in ids) {
       m = sprintf('\\(\\\\#%s\\)', j)
       if (grepl(m, content[i])) {
-        id = ''; sep = ':'
+        id = ''; sep = ':'; bold = FALSE
         type = gsub('^([^:]+).*$', '\\1', j)
         if (type %in% theorem_abbr) {
           id = sprintf('<span id="%s"></span>', j)
           sep = ''
+          bold = TRUE
         }
-        label = label_prefix(type, sep = sep)(ref_table[j])
+        label = label_prefix(type, sep = sep, bold = bold)(ref_table[j])
         content[i] = sub(m, paste0(id, label, ' '), content[i])
         break
       }
diff --git a/R/html.R b/R/html.R
index 6b04a4cd..a422bd6b 100644
--- a/R/html.R
+++ b/R/html.R
@@ -693,7 +693,7 @@ parse_fig_labels = function(content, global = FALSE) {


 # given a label, e.g. fig:foo, figure out the appropriate prefix
-label_prefix = function(type, dict = label_names, sep = '') {
+label_prefix = function(type, dict = label_names, sep = '', bold = FALSE) {
   label = i18n('label', type, dict)
   supported_type = c('fig', 'tab', 'eq')
   if (is.function(label)) {
{
   }
   function(num = NULL) {
     if (is.null(num)) return(label)
-    paste0(label, num, sep)
+    label = paste0(label, num, sep)
+    if (bold) sprintf("**%s**", label) else label
   }
 }

The aim of resolve_refs_md is to replace in the markdown file the reference part by the labels. Then it is process by Pandoc again. So it seems to work as expected.

What do you think ?

@cderv
Copy link
Collaborator

cderv commented Jul 20, 2021

If we want to offer a mechanism to customize / opt-out from this, we could extend the mechanism of passing a function also for those environments.
We only offer it for figure, table and equation for now: #1120 (review)

@yihui
Copy link
Member

yihui commented Jul 20, 2021

The PR #1206 looks good to me. @N0rbert What do you think of the bold style? It will look like this:

Example 1.1 Simple C Program

Will that help?

@N0rbert
Copy link
Author

N0rbert commented Jul 21, 2021

If possible, @yihui , please add colon (:) instead of bold style:

Example 1.1: Simple C Program

as it was in previous versions.

@yihui
Copy link
Member

yihui commented Jul 21, 2021

I think we can provide an option to add the colon, but by default, I hope not to include the colon, to be consistent with other output formats (PDF and HTML). Will that be okay to you?

@N0rbert
Copy link
Author

N0rbert commented Jul 21, 2021

Providing an option is okay for me. Thanks!

@cderv
Copy link
Collaborator

cderv commented Jul 21, 2021

@yihui should it be an option specific for the colon or extending the mechanism to pass a function to customise labels ?

With this mechanism I believe it could be customized any way a user wants it to be.

@yihui
Copy link
Member

yihui commented Jul 21, 2021

should it be an option specific for the colon or extending the mechanism to pass a function to customise labels ?

The latter (if possible) would be better. Either way is fine to me.

@cderv
Copy link
Collaborator

cderv commented Jul 21, 2021

I think I can extend the mechanism I put in place for fig, tab and eq for now. Otherwise, I'll add an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior next to consider for next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants