Newline '\n' in rendered strings not enough for working in terminal 'raw mode'

[FriedlandAaron]

Hey! First of all, I wanted to say thanks for the awesome library! I was about to start planning how I implement something similar myself, so I'm really glad I stumbled upon your code :)

On to my use case:
I'm building a TUI application (using Termion as a backend), and I wanted to add some nice looking text blocks to make different game state screens as well as graphical transitions. I tried rendering a string and printing it, but the lines that make up the rendered string were not aligned, cutting up my text blocks length-wise. Anyways, after some time debugging, I found out the reason: when using the 'raw mode' provided by the backend, one of the differences from normal mode is that '\n' only moves the terminal cursor one line down, instead of doing a full line break and setting the cursor to the beginning of the line.

Currently, I can work around that pretty easily by just replacing '\n' with'\r\n' using the replace() method. However, I thought to raise the issue here, and see if you'd be interested in adding support for 'raw mode' strings. I would be happy to take a crack at it too, if you're interested and willing to accept contributions to the code.

What are your thoughts?

[dominikwilkowski]

Hey thanks for logging the issue. Yeah we should fix that. Adding carriage returns wold probably be good. Thought maybe we add it as an option to not break the output right now?
Maybe something like --raw-mode?
If you're keen you can send a PR. Otherwise I probably have time in the next few weeks.

[FriedlandAaron]

Hey!
Adding it as an option sounds like a good idea, and it also ensures nothing changes for everyone else using the code by setting the default to false. Another idea would be to just make add_line create carriage returns by default, instead of just '\n'. I'm trying to think of how this would affect users, but I'm not sure it would, since functionally everything would be the same (?) Unless there are users who parse/test/modify the string and specifically touch the line breaks.

I would be glad to send a PR, will try to get around to it probably sometime in the next week or two. Is there a specific workflow you prefer I follow in regards to submitting a PR?

[dominikwilkowski]

People are using the output and making changes to the strings. Let's not make any changes to the output.

So to ship this we need:

• add a flag --raw-mode and the shortcut -r (both still available)
• add raw_mode to both APIs (Node and rust)
• add tests for both APIs

Then we can push out a new minor release.

[FriedlandAaron]

Pr open and ready for review: #80.
Let me know if everything looks fine.

Some other small things I encountered while working on this feature, that I thought I should raise and perhaps could be addressed in the future:

• Regarding alignment, adding newlines when aligning isn't consistent with adding newlines in the rest of the rendering process (mutating existing vectors vs. adding new vectors with newlines). This could be confusing if someone wants to manipulate the resulting text vectors.
• Regarding the spaceless option in the node implementation, the flag is defined as --spaceless, but then the inside logic flips it around and refers to the option as space. This is more of a developer thing, as it wouldn't affect end users, but it would make sense to have consistency between the public API and the internal implementation.

I could address those issues myself sometime, if you would be interested.

[dominikwilkowski]

Glad you added your observations here. Meant to ask.

Regarding alignment, adding newlines when aligning isn't consistent with adding newlines in the rest of the rendering process (mutating existing vectors vs. adding new vectors with newlines). This could be confusing if someone wants to manipulate the resulting text vectors.

Talk to me more about this.

Regarding the spaceless option in the node implementation, the flag is defined as --spaceless, but then the inside logic flips it around and refers to the option as space. This is more of a developer thing, as it wouldn't affect end users, but it would make sense to have consistency between the public API and the internal implementation.

Yeah that option has been a pain in the butt for a while. It's a negative flag to enable and I couldn't find another way to express it back then and now we have this API that is not worth break for this little thing. What would you suggest? Just name it the same internally? I don't really write node anymore these days so lost a bit of interest but force myself to keep both code bases in sync for compat reasons.

[FriedlandAaron]

Regarding alignment, adding newlines when aligning isn't consistent with adding newlines in the rest of the rendering process (mutating existing vectors vs. adding new vectors with newlines). This could be confusing if someone wants to manipulate the resulting text vectors.

Talk to me more about this.

In the rust implementation, the render function adds new lines by pushing String::New() onto the vector, creating a new string that holds the char \n. Later on, when the function checks alignment and modifies the string vector, it manually adds a number of \n chars to an existing string in the vector (and specifically in the rust implementation, the Top alignment works differently by pushing a new string into the vector, which consists of multiple \n chars). I'm pretty sure this is similar in the node implementation.

Why is this undesired? Well, there are 2 reasons that come to mind for now:

• Refactoring is a bit more annoying (e.g. your suggestion regarding setting newline char before). Because adding newlines is a bit different in different places, refactoring requires changing all those places to be compatible with the raw mode flag being used.
• Tests are a bit more annoying to write, since it isn't quite clear beforehand how many newlines there should be in the expected result. Having consistency in the text vector (for example, each newline has it's own place in the vector) makes it easier to reason with the resulting text.

I hope this makes my thoughts clear. If not, I'll gladly try to explain again.

[FriedlandAaron]

Regarding the spaceless option in the node implementation, the flag is defined as --spaceless, but then the inside logic flips it around and refers to the option as space. This is more of a developer thing, as it wouldn't affect end users, but it would make sense to have consistency between the public API and the internal implementation.

Yeah that option has been a pain in the butt for a while. It's a negative flag to enable and I couldn't find another way to express it back then and now we have this API that is not worth break for this little thing. What would you suggest? Just name it the same internally? I don't really write node anymore these days so lost a bit of interest but force myself to keep both code bases in sync for compat reasons.

I think just changing the inner implementation to refer to space as spaceless, and to check for false instead of true, should do the trick. That would mean there would be no need to change the API.

[dominikwilkowski]

Ok got you.

I agree with you on point one: we should make the implementation of adding new lines consistent for all alignment methods. I haven't looked at this for a while.

... String::New() onto the vector, creating a new string that holds the char \n ...
I first read this as "String::new() contains \n by default" and I had to test it to make sure that's not right (because that would make using a variable for a new line difficult)

fn main() {
    let mut foo = String::new();
    println!("{:?}", foo.contains("\n"));
    foo += "\n";
    println!("{:?}", foo.contains("\n"));
}
// will print:
// false
// true

I think you meant this differently.

cfonts/rust/src/render.rs

Lines 182 to 190 in 1a807b0

	match options.align {
	Align::Top => output.push(String::from("\n\n\n")),
	Align::Bottom => output[0] = String::from("\n\n\n\n") + &output[0],
	Align::Left | Align::Center | Align::Right => {
	output[0] = String::from("\n\n") + &output[0];
	let last_index = output.len() - 1;
	output[last_index] = format!("{}\n\n", output[last_index]);
	}
	}

So here we check if thee is vertical space to be added. That seems consistent to me?

So overall I agree with your point I just don't know in detail what you're proposing :)

[FriedlandAaron]

I first read this as "String::new() contains \n by default" and I had to test it to make sure that's not right (because that would make using a variable for a new line difficult)

You're right, my bad. I got confused. String::new() just adds an empty string. I just realized, the add_line() function is meant to add another (empty) string to the vector, and the newlines get introduced later when you join() them to create the combined string.

So currently, this is what happens:

• addline function adds an empty string to the text's string vector
• alignment code block adds newline characters, either to existing strings in the vector, or in the case of rust and Top alignment, adds a string to the vector containing multiple newline characters
• join function creates the final string by joining all strings in the vector with a newline character

I've given it some thought, and I'm not sure these things should be changed just yet. They should happen at some point, but I feel like they are potentially breaking changes, since they are modifying the structure of the text vector. Most users are probably just using the full string, so they wouldn't be affected, but anyone who is processing the text vector would find a different looking vector if we made these changes.

I propose the following:

• merge the current solution (replace \n for \r\n at the end of the rendering pipeline) and release a new minor version
• open up issues detailing the problems we discussed here and their solutions, for future implementation (making sure it is mentioned that these would be breaking changes)

I know this isn't the cleanest, but it has the major benefit of not changing anything about the current implementation and potentially breaking stuff.

Let me know your thoughts.

[dominikwilkowski]

Sorry for the laps of time ... Who knew time just progresses while you're busy with other things.
Anyway what speaks against a variable at the top of the code called line_break that defaults to \n and is changed to \r\n when the option is detected and then used everywhere where new lines are added? Could be a function as well of course... not too fuzzed about that detail.

[FriedlandAaron]

Hey, no worries. Different time zones and personal priorities are going to create asynchronous collaboration. That's just how it is.
Anyways, I just pushed a commit changing the implementation so that a variable named line_break is defined towards the beginning of the render function, and changed based on the relevant flags. It then replaces all usage of '\n' in the rest of the function.

[dominikwilkowski]

Looks great. Thank you for that work.

[dominikwilkowski]

Ok everything is ready and #80 is merged
Will do a release for all targets tomorrow which then should close this issue. Thanks for your help @FriedlandAaron

[dominikwilkowski]

FYI the tests broke because prettier failed the files because of the mix between LF and CRLF. Had to set git to handle it before checkout and now it all works... Took a minute

[FriedlandAaron]

FYI the tests broke because prettier failed the files because of the mix between LF and CRLF. Had to set git to handle it before checkout and now it all works... Took a minute

Thanks for that. I would love to hear a bit more what you did to fix it, if you feel like explaining.

Ok everything is ready and #80 is merged
Will do a release for all targets tomorrow which then should close this issue. Thanks for your help @FriedlandAaron

Thank you so much for letting me contribute! I'm very excited and happy to be a part of your project, and I enjoyed the discussions and collaboration.

[dominikwilkowski]

I would love to hear a bit more what you did to fix it, if you feel like explaining.

I had to configure git to not convert line endings on windows:

cfonts/.github/workflows/testing.yml

Lines 162 to 165 in ec003b1

	- if: matrix.os-node.os == 'windows-latest'
	run: |
	git config --global core.autocrlf false
	git config --global core.eol lf

For that to work I had to remove the working dir config:

cfonts/.github/workflows/testing.yml

Lines 160 to 162 in e00f0fa

	defaults:
	run:
	working-directory: ./nodejs

And add it to each command manually:

cfonts/.github/workflows/testing.yml

Lines 195 to 205 in ec003b1

	- name: Yarn install dependencies
	run: cd nodejs && yarn install --frozen-lockfile
	- name: Build files
	run: cd nodejs && yarn build
	- name: Tree files
	run: npx tree-cli -l 5 --ignore "node_modules/, .git/"
	- name: Yarn test
	run: cd nodejs && yarn test

I also had to remove node 12 and 14 from the tests as Github actions has moved to ARM machines which don't support the old node versions anymore:

cfonts/.github/workflows/testing.yml

Lines 131 to 136 in ec003b1

	# - os: macOS-latest
	# node: 12
	# Not available for ARM
	# - os: macOS-latest
	# node: 14
	# Not available for ARM

[FriedlandAaron]

Thanks for the explanation! Who knew newlines could be such a pain in the behind? 😆

By the way, I just noticed I didn't update any of the readme files 🤦‍♂️ You want me to submit some quick edits?

[dominikwilkowski]

No stress. I got the readme already updated but haven't pushed it yet. Thanks though.

[dominikwilkowski]

Ok v1.2.0rust and v3.2.0nodejs has been pushed out. It takes some time to hydrate to all channels (brew, macports etc) but it's out there now :)