PDF formatter soft line breaks

[martijnversluis]

No description provided.

[isaiahdahl]

Still am seeing this issue:

It doesn’t seem to work when the \ follows a closing chord bracket.

does [G]\ not soft break

Does [G] \ soft break

It also doesn't seem to be working with the ChordOverWords parser.

Noticed a few other regressions from the refactor:
it was generating:

it now generates:

I pushed a commit with a few small fixes to some concrete things I had discovered but otherwise I'll just make a list here of things I'm seeing:

• chordpro parsing not soft line breaking when '\' follows a ']'
• ChordsOverWords parse has no soft line break support
• soft line break had logic to make the first character of the next line capitalized, that got removed
• when item as Comment is used it was trying to access item.content, and item.content is undefined. The actual data lives in item.value so there's a related bug there.
• the line rendering spacing is kind of funky, it seems to be adding extra space between lines sometimes, and definitely adding extra space between lines after comments
• if the starting lyric of a line doesn't have a chord, and was moved to a subsequent line from the soft line break, it should remove the leading space.
• commas should be parsed as softlinebreaks in the grammar.

I pushed a fix for setting the current column back to 1 when it renders the first time, and made a tweak to the tag/comment logic so you can actually see comments rendered. Also set softLineBreaks to true in the pdf-dev.js file so that can be played with by default.

Let me know if you have any thoughts or questions to my thoughts!

[isaiahdahl]

@martijnversluis What about giving "SoftLineBreak()" a content value in the model so that the soft line break is either got nothing (when it's a backslash) or it has content that could be a comma ',' then the renderer's can decide to render the content of the SoftLineBreak when it encounters it. thoughts?

[martijnversluis]

@martijnversluis What about giving "SoftLineBreak()" a content value in the model so that the soft line break is either got nothing (when it's a backslash) or it has content that could be a comma ',' then the renderer's can decide to render the content of the SoftLineBreak when it encounters it. thoughts?

I considered it, but got it working without it. That would've meant we had to render soft line breaks. Instead, we now replace a comma with a soft line break and a ChordLyricsPair with the comma (and optional space) as lyrics.

[isaiahdahl]

@martijnversluis What about giving "SoftLineBreak()" a content value in the model so that the soft line break is either got nothing (when it's a backslash) or it has content that could be a comma ',' then the renderer's can decide to render the content of the SoftLineBreak when it encounters it. thoughts?

I considered it, but got it working without it. That would've meant we had to render soft line breaks. Instead, we now replace a comma with a soft line break and a ChordLyricsPair with the comma (and optional space) as lyrics.

With this approach, would the comma always render?

When the softline break logic actually breaks to the next line it should capitalize the first letter, which makes the comma not needed gramatically. This might be difficult to achieve.

[martijnversluis]

I think all the issues with PDF formatting have been fixed in this PR.

However, there is currently a weird build error both in this branch and on master and PR branches. In most cases there is an error malloc(): invalid size, in some cases the build just crashes.

[isaiahdahl]

So there's still a number of issues that either weren't issues before or got introduced with other adjustments.

I decided to take a stab at the the pdf formatted myself (along with some help) and really drilled into the example "kingdom" chart comparing what the current PC renderer does, I think I made some pretty good progress, but some approaches are drastically different. I wanted to try a few things differently.

https://github.com/bettermusic/ChordSheetJS/tree/pdf-formatter-slb-isaiah

All tests pass except for the pdf one because I couldn't figure out how you determined the exptectedText & expectedLines.

Here's a list of some of the things I saw and addressed:

• comma line breaking logic isn't actually working
• not a huge fan of how commas end up pushed into white space

this branch:

my branch:

PC chart:

configuration in our branches doesn't have as much between column spaces so the second "A - men" break isn't needed

• when a line breaks that only has chords, there is extra line height
• there is extra space underneath comments

this branch:

My branch:

PC chart:

• chords breaking to their own line creates extra space

this branch:

my branch:

PC chart:

Moving Forward

outstanding issues even in my own branch that would still be there before being able to replace all of the PraiseCharts PDF generating to run through this library would be:

• capitalize first word on soft line break when first chordLyricPair on the broken line doesn't have lyrics in it.
  

• some kind of logic to better keep sections together. We have things in place so you don't end up with a section title as the last item in a a column and then the next lines are on the next page.
  ie:
  

• logic to not break a single chord lyric line to a new column if it could have a pair. The PC rendered tries to make sure that we don't orphan a chord lyric line so that it's easier to read musically jumping from page to page column to column.

• chord diagram rendering

• conditional rendering of some header elements

• custom font that doesn't bold rhythm markers [|] [\]

And that would just get us to a product that I could roll out to everyone to replace what we have. But then I'd still have features (weren't in this PR's scope) of improvement we'd plan to add to make them even better.

• use # and b symbols instead of text
• chord suffix's in a suffix font style
• re-writing so that we use {soc: Chorus 1} {sov: Verse 1 } section headers and {comment: } is used for actual md comments and rendered accordingly.
• support for songmap

Would love to sync up over a call if you have time.

[isaiahdahl]

No I'm still getting

yarn build                                                          INT ✘  1m 12s 
yarn run v1.22.19
$ yarn build:code-generate && yarn build:sources && yarn build:bundle && yarn build:check-types
$ yarn build:suffix-normalize && yarn build:chord-suffix-grammar && yarn build:pegjs && yarn build:scales
$ shx rm -rf src/normalize_mappings/suffix-normalize-mapping.ts && tsx src/normalize_mappings/generate-suffix-normalize-mapping.ts
 👷 Building suffix normalize mapping from suffix-mapping.txt
 ✨ Successfully built suffix-normalize-mapping.ts
$ yarn tsx script/generate_chord_suffix_grammar.ts
$ /home/isaiah/projects/@bettermusic/ChordSheetJS/node_modules/.bin/tsx script/generate_chord_suffix_grammar.ts
 👷 Building suffix normalize mapping from suffix-mapping.txt
 ✨ Successfully built src/parser/chord/suffix_grammar.pegjs
$ yarn build:pegjs:chord && yarn build:pegjs:chordpro && yarn build:pegjs:chords-over-words
$ tsx script/combine_files.ts src/parser/chord/base_grammar.pegjs src/parser/chord/suffix_grammar.pegjs src/parser/chord/combined_grammer.pegjs && peggy --plugin ts-pegjs -o src/parser/chord/peg_parser.ts src/parser/chord/combined_grammer.pegjs
Error importing "ts-pegjs"
Cannot find module '/home/isaiah/projects/@bettermusic/ChordSheetJS/node_modules/prettier/parser-typescript' imported from /home/isaiah/projects/@bettermusic/ChordSheetJS/node_modules/ts-pegjs/dist/tspegjs.mjs
Did you mean to import prettier/parser-typescript.js?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I can see it got further in the CI though... Something with my node version and what's in the yarn.lock is causing grief. I happen to be running 18.19.1, but then I also encounter the exact same error when running 20.18.0...

Can you try cleaning node_modules and pushing a new yarn.lock file and tell me what version of node you're running? It's gotta be somehow related to that.

I'll keep messing with it, clearly is something related to my machine/env, since CI isn't encountering it across all those versions.

[isaiahdahl]

🎉

[isaiahdahl]

🤦‍♂️ Wasn't actually using the renderChord() function when rendering the chords so when if the song key and/or capo was changed it wouldn't actually render the proper chord, also wasn't properly doing the normalizing etc.

Adjusted so it does that, and also updated the yarn dev:pdf to have a key and capo dropdown for ongoing testing of that.

[martijnversluis]

Nice! I think we should merge this beast and start new PRs for follow-ups 👍

[isaiahdahl]

Boom! 💥