Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Don't destroy all newlines in Spans #341

Closed
wants to merge 2 commits into from

Conversation

TheLostLambda
Copy link
Contributor

This is the one-line fix to #339, but it was obviously there intentionally, so I'm open to discussing the merits of forcing the Text abstraction as the way to do multiple lines.

It's just that the current version makes applying one style to several lines much more difficult than it should be and makes double newlines extra messy.

Additionally, I've not tested it on Windows, but how does this newline filtering work on DOS files with the \r\n line ending?

@Byron
Copy link
Contributor

Byron commented Jul 22, 2020

I also fell for this when upgrading dua to v0.10. Besides the surprising behaviour, it's unclear what is supposed to be done instead in order to get an empty line. I found myself adding Spans::from("") to get newlines, which now is as expensive as allocating a vec with one element. Allowing to keep newlines would help with that, too.

@fdehau
Copy link
Owner

fdehau commented Aug 2, 2020

@TheLostLambda

This is the one-line fix to #339, but it was obviously there intentionally

Yes you are right, I've put this there to enforce the contract that these three new text primitives are meant to expose. Removing this will produce unexpected results if new lines are used in the context of List (the only other places where Text is used for now).

It's just that the current version makes applying one style to several lines much more difficult than it should be and makes double newlines extra messy.

I'm open to a discussion (which should probably happen on the issue for visibility) about but I need detailed examples of what are the pain points. My first impression is that the issue is not the design of the primitives themselves but more the lack of flexibility in how you can build them. I've thought about a builder with a style stack and maybe macros during the implementation but I haven't found much time to give it more thoughts.

Additionally, I've not tested it on Windows, but how does this newline filtering work on DOS files with the \r\n line ending?

That's a good point. I'll probably need to revisit the behavior on Windows.

@Byron

Besides the surprising behavior, it's unclear what is supposed to be done instead in order to get an empty line. I found myself adding Spans::from("") to get newlines

You shouldn't have to add anything to get new lines, each line is simply one item of the Vec that can be used to build a Text and you can get empty lines with Spans::from(""). I suspect that I'm missing something but without examples it will be hard for me to get an understanding of what you are struggling with.

which now is as expensive as allocating a vec with one element

I wouldn't worry too much about this unless you have numbers as there are way more expensive operations in tui at the moment. Beside you should be able to use Spans::from(Vec::new()) to save an allocation (I could probably add Spans::empty() for this use case though).

TLDR: I would be more than happy to change things to make the text authoring more pleasant to work with but I need more feedback :).

@TheLostLambda
Copy link
Contributor Author

Heya to anyone following this! I'm closing this in favour of #361 which fixes the same problem but in a more elegant way and without breaking the new text abstraction. Definitely let me know what you think of that new PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants