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

Wrapping titles and footnotes automatically #46

Merged
merged 25 commits into from
Sep 30, 2022
Merged

Conversation

Melkiades
Copy link
Contributor

@Melkiades Melkiades commented Sep 22, 2022

The points to discuss are wether the current implementation:

  • fits the desired needs (@shajoezhu).
  • needs to propagate or have more parameters (please notice that to see the effects of the implementation it is needed to use toString as it is done in the newly designed tests).
  • works fine also in exports (such as pdfs) as this is a working solution for direct prints to the terminal only atm (this cannot be solved yet as it depend on the parameter propagation in rtables version of toString and where it is used)
  • should deal with relevant parameters (nchar_max) to do so.
  • is safe for extreme cases (one is already considered), and the various input checks (do we need checkmate?) -> Opened an issue for checking inputs and considered extreme cases where words are extremely large in the following comments.

@Melkiades Melkiades self-assigned this Sep 22, 2022
@Melkiades Melkiades added the enhancement New feature or request label Sep 22, 2022
R/tostring.R Show resolved Hide resolved
@Melkiades Melkiades marked this pull request as draft September 22, 2022 14:08
@Melkiades Melkiades changed the title Indenting titles and footnotes automatically Wrapping titles and footnotes automatically Sep 22, 2022
Copy link
Collaborator

@gmbecker gmbecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether we want to indent after wrapping or not is something of an open question, I'd say focus on the wrapping aspect of it for now and we can add the indent if needed once its in.

R/tostring.R Outdated Show resolved Hide resolved
R/tostring.R Outdated Show resolved Hide resolved
R/tostring.R Outdated Show resolved Hide resolved
R/tostring.R Outdated Show resolved Hide resolved
R/tostring.R Outdated Show resolved Hide resolved
@Melkiades Melkiades marked this pull request as ready for review September 26, 2022 12:59
@Melkiades
Copy link
Contributor Author

Melkiades commented Sep 26, 2022

@gmbecker I could proceed in propagating this in tt_toString.R if you agree to the status of things. I also added some documentation to the function with some details about the idea of newline character (I was not aware you could do that tbh).
From this we need to:

  • propagate two vars (wrap and max_width)
  • decide how to proceed with indentation
  • Should I include also ref_footnotes in the current wrapping? is it for references?
  • create a new issue to discuss possible workarounds for long paths or long "words" that could exceed the inserted width.

@gmbecker
Copy link
Collaborator

gmbecker commented Sep 26, 2022

@gmbecker I could proceed in propagating this in tt_toString.R if you agree to the status of things. I also added some documentation to the function with some details about the idea of newline character (I was not aware you could do that tbh). From this we need to:

  • propagate two vars (wrap and max_width)

I don't think we can call it wrap because it doesn't control word-wrapping for cell values (its correct that it doesn't, as we talked about, I just thinkt he name needs to change. tf_wrap maybe? Lets discuss it tomorrow.

  • decide how to proceed with indentation
  • Should I include also ref_footnotes in the current wrapping? is it for references?

Yes they should wrap to the same width as the rest of the title/footer materials

  • create a new issue to discuss possible workarounds for long paths or long "words" that could exceed the inserted width.

I'd say we just do it. Do a check after attempted wrapping and if it didn't wrap, and is still too long, just do a blind hard break at the width.

@Melkiades Melkiades added the sme Tracks changes for the sme board label Sep 27, 2022
@Melkiades
Copy link
Contributor Author

Melkiades commented Sep 27, 2022

I encountered a weird case to me:

a <- "sub titl e is"
strwrap(a, 4)
[1] "sub"  "titl" "e"    "is"  

while I would expect "sub" "titl" "e is". Is this the right behavior? Also, this makes a bit useless my last fix that aimed at getting rid of the lonely e in this case.
I actually found an alternative to this that solves the problem as needed:

stringi::stri_wrap(a, 4)
[1] "sub"  "titl" "e is"  

Let me know if it is fine for me to add this to the suggests @gmbecker

DESCRIPTION Show resolved Hide resolved
@Melkiades Melkiades enabled auto-merge (squash) September 29, 2022 13:34
@gmbecker
Copy link
Collaborator

sub titl e is"

I encountered a weird case to me:

a <- "sub titl e is"
strwrap(a, 4)
[1] "sub"  "titl" "e"    "is"  

Right, so this is related to a note in the docs:

Note that messages are wrapped AT the target column indicated by

'width' (and not beyond it).

From an R-devel posting by J. Hosking [email protected].

x <- paste(sapply(sample(10, 100, replace = TRUE),
           function(x) substring("aaaaaaaaaa", 1, x)), collapse = " ")
sapply(10:40,
      function(m)
       c(target = m, actual = max(nchar(strwrap(x, m)))))
> ?strwrap
> x <- paste(sapply(sample(10, 100, replace = TRUE),
+                 function(x) substring("aaaaaaaaaa", 1, x)), collapse = " ")
>      sapply(10:40,
+             function(m)
+             c(target = m, actual = max(nchar(strwrap(x, m))))
+ )
       [,1] [,2] [,3] [,4] [,5] [,6] [,7] [,8] [,9] [,10] [,11] [,12] [,13]
target   10   11   12   13   14   15   16   17   18    19    20    21    22
actual   10   10   11   12   13   14   15   16   17    18    19    20    21
       [,14] [,15] [,16] [,17] [,18] [,19] [,20] [,21] [,22] [,23] [,24] [,25]
target    23    24    25    26    27    28    29    30    31    32    33    34
actual    22    23    24    25    26    27    28    29    30    31    32    33
       [,26] [,27] [,28] [,29] [,30] [,31]
target    35    36    37    38    39    40
actual    34    35    36    37    38    39
> ?strwrap
> x
[1] "aaaa aaaaaaaaa aaaaaaaaa aaaa aaaaaaaaa aaaaaaaa aaaaaaaaaa aaaaaa a aaaaaa aaaaaaaa aaaaa aaaaaaaaa a aa aaaaa aaaa aaaa aaaaaa aaa aaaaaaa aaaaaaa aaaaa aaaaaaa aaaa aaaaa aaaaaaaaa aaaaaaaaaa aaaa aaaaaaaaa aaaaaaa aaaaaaaaa aaaaa aaaaaaaaaa aaaaaa a aaaaa aaaaaaaaaa aaaaaa a aaaaaaa aaaaaaaa a aaaaaa aaaaaaaaa aaa aaaaa aa aaaaaaa aaaaa aaaaaaa aaaaaaa aaaaaaa aaaaa aaaa aaaaa aaaaaa aaaaaa aaaaaaaaa aaaaaaaaa aaaa aaaaaaaaaa aaaaaaaaaa a aa aa aa aaaaaaaa aaaaa aaaaa aaaaaaaaaa aaaaaaaaa aaa aaaaaaaaaa aaaaaaaaa aa aa a aaaaaaa aa aaaaaaaaa aaaaa aa aaaaaaaaaa aaa aaaa aaaaaaaa aaaaaaa aaa aaaaaaaaa aa aaaaaaaaa aaaaaaa aaaaaaaaaa aaaaaa aaaa aaaaaa aaaaaaaa aaaa aaaaaaaaaa"

So basically what that means, from what I can tell, is that the nth character is going to bet he first after the wrapping, rather than the last before it, where n is the specified width. The reason titl is still there is because strwrap leaves intacts words that are "too big" as we found before. Given that we know this about base::strwrap we should be able to work around it with no problem.

Basically we need to do our manual breaks as we're doing now, and then specify width as 1 larger for the purposes of base::strwrap and everything should work out.

while I would expect "sub" "titl" "e is". Is this the right behavior? Also, this makes a bit useless my last fix that aimed at getting rid of the lonely e in this case. I actually found an alternative to this that solves the problem as needed:

stringi::stri_wrap(a, 4)
[1] "sub"  "titl" "e is"  

Let me know if it is fine for me to add this to the suggests @gmbecker

I'd rather we work around base::strwrap, rather than adding a strong dependency on stringi (it would not be able to be a Suggests moving forward, I don't think)

@Melkiades
Copy link
Contributor Author

Ok, I did not know about the workaround. It makes perfect sense now. stringi removed.

@gmbecker
Copy link
Collaborator

gmbecker commented Sep 29, 2022

@Melkiades please rebase and make any necessary changes for this PR to be compatible with the table_inset functionality I just added

I will need to take over this PR and push it over the finish line if it isn't quite there by your EOD tomorrow (ie friday morning/midday US time)

Copy link
Collaborator

@gmbecker gmbecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much there, couple of tweaks.

R/tostring.R Outdated Show resolved Hide resolved
NEWS.md Show resolved Hide resolved
R/tostring.R Outdated Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
@Melkiades
Copy link
Contributor Author

I think we are there. I modified slightly the .footer_inset_helper so to use another variable instead of x so the wrapping preprocessing can still be done.

R/tostring.R Outdated Show resolved Hide resolved
@Melkiades Melkiades merged commit 30c6dee into main Sep 30, 2022
@Melkiades Melkiades deleted the 45_newline_title_fn@main branch September 30, 2022 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sme Tracks changes for the sme board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants