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

Allow custom system chat labels #18044

Merged
merged 1 commit into from
Dec 24, 2020
Merged

Conversation

Mailaender
Copy link
Member

Closes #17551.

teinarss
teinarss previously approved these changes May 8, 2020
@dragunoff
Copy link
Contributor

I tried tackling that in #17563 but decided it's worth first figuring out a path forward for translations and for configurable strings (and whether they can be treated as the same thing, see #17563 (comment)).

And I think that we should not be mixing visual styles with text strings in ChromeMetrics. Perhaps a new class should be created to handle them 🤔

@pchote
Copy link
Member

pchote commented May 17, 2020

We already know what that new class is: the translation strings file.

I wish that somebody could decide to take the lead on planning and (then only after the plan has been reviewed for technical feasibility) implementing the code-side string translation backend. We have wasted so much time motivation in arguing and rejecting workarounds that all come back to this one thing.

@reaperrr
Copy link
Contributor

I wish that somebody could decide to take the lead on planning and (then only after the plan has been reviewed for technical feasibility) implementing the code-side string translation backend. We have wasted so much time motivation in arguing and rejecting workarounds that all come back to this one thing.

That's one of those things I would have tried to tackle ages ago if it didn't exceed my abilities.

I'd gladly volunteer to help out with any 'grunt work' involved, but as long as nobody else tackles that first hurdle, I can't help with the rest.

@Mailaender
Copy link
Member Author

Moved it to the translation system.

@pchote
Copy link
Member

pchote commented May 17, 2020

My comment about reviewing the plan for technical feasibility before making any changes was important: we know that the current stub translation code is not suitable, so it would be a mistake to start moving anything to it now.

@Mailaender
Copy link
Member Author

My explorative implementation here proofs that it is indeed already suitable for this task.

@pchote
Copy link
Member

pchote commented May 17, 2020

This is missing the point that the current stub implementation is hazardous to project maintainability due to having no safe handling for format string variables (which will essentially guarantee un-lintable runtime crashes when translations get out of sync) and no integration with existing formats for crowd-sourced translations. It also ignores the other benefits that would come from a proper translation system such as support for distinguishing zero/one/few/many and genders.

@Mailaender
Copy link
Member Author

This is just moving a hard-coded string into a .yaml so it can be translated at a per mod level.

@abcdefg30
Copy link
Member

Tbh, I think the best way forward here is to move this back to metrics. As weird as it is, using the incomplete/broken language stuff seems even weirder to me. One problem with metrics is still that this would break in the future if we add translation support, but not as bad as when placing this in the language files. Otherwise it would likely be better to close this PR.

@Mailaender
Copy link
Member Author

Where would you place the hard-coded string instead?

@abcdefg30
Copy link
Member

I can only repeat #18044 (comment): We already established putting it in metrics isn't great, but using the current language "system" is even worse. So I'd put it into metrics unless someone else has a better idea. (An we should really think about removing the stub language stuff.)

@Mailaender Mailaender force-pushed the custom-system-chat branch 2 times, most recently from c930c3e to 3316371 Compare December 19, 2020 09:45
@Mailaender
Copy link
Member Author

You were both adding so much worry bearing afterwards that I got confused. Reverted.

mods/d2k/metrics.yaml Outdated Show resolved Hide resolved
@pchote pchote merged commit 13a7de4 into OpenRA:bleed Dec 24, 2020
@Mailaender Mailaender deleted the custom-system-chat branch December 24, 2020 12:07
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.

"Battlefield Control" chat label is hard-coded
6 participants