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

Fix \r\n-style line endings not being properly handled for script errors #20871

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

abcdefg30
Copy link
Member

Otherwise the \r characters remain on their own (without \n characters) and display as artifacts.

Mailaender
Mailaender previously approved these changes May 20, 2023
@PunkPun
Copy link
Member

PunkPun commented May 20, 2023

This doesn't feel correct. The vast majority of the codebase doesn't have or use \r so removing it here doesn't make sense. It should be removed the moment a string enters our codebase.

@abcdefg30
Copy link
Member Author

This doesn't feel correct. The vast majority of the codebase doesn't have or use \r so removing it here doesn't make sense. It should be removed at the exact flawed implementation

The flawed implementation is this function assuming that all strings use just \n for indicating new lines. I don't think we should abandon the general solution and fix every specific case.

@PunkPun
Copy link
Member

PunkPun commented May 20, 2023

it's a correct assumption though. Why would text contain \r ? We go to great lengths to not have any \r in the engine

@Mailaender
Copy link
Member

This is in line with

var briefing = WidgetUtils.WrapText(missionData.Briefing?.Replace("\\n", "\n"), description.Bounds.Width, descriptionFont);

where a similar correction is done in UI code.

@PunkPun
Copy link
Member

PunkPun commented May 20, 2023

If we are going to handle \r, it should either be engine wide or at the source. Not somewhere in the middle like this PR

IMO it should be moved to ScriptErrorLogic.

@penev92
Copy link
Member

penev92 commented May 20, 2023

Ideally it would be moved to somewhere that handles all text that we display in-game.

@PunkPun
Copy link
Member

PunkPun commented May 20, 2023

we remove the \r when streaming files & reading yaml. So basically in most places you can't find \r and it doesn't make sense to handle at a UI level

@penev92
Copy link
Member

penev92 commented May 20, 2023

it doesn't make sense to handle at a UI level
The issue is literally that our UI can't handle visualizing it.

On the other hand, it is not ScriptErrorLogic's job to know or think about the presentation layer, UI, how to visualize things, etc.

And yes, the most prominent source of "\r\n" would likely be .NET exceptions, but it could also be any library that we use, any text that is somehow read by the game to be displayed and perhaps even user text pasted into the chat?

@PunkPun
Copy link
Member

PunkPun commented May 30, 2023

user text pasted into the chat

Then we should also add \r removal in chat input

@anvilvapre
Copy link
Contributor

I would lean towards PunkPun opinion. Best to strip at input, before start, or support it throughput.

@abcdefg30
Copy link
Member Author

Seems like I got outvoted (here and on Discord), so changed.

@abcdefg30 abcdefg30 changed the title Fix Utils.WrapText not accounting for \r\n-style line endings Fix \r\n-style line endings not being properly handled for script errors Jun 14, 2023
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

LGTM, Untested

@Mailaender Mailaender merged commit 49c837e into OpenRA:bleed Jun 26, 2023
3 checks passed
@Mailaender
Copy link
Member

Changelog

@abcdefg30 abcdefg30 deleted the wrapBackRText branch June 27, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants