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

Refactor ally dialogue in UtBS #6158

Open
Wedge009 opened this issue Oct 5, 2021 · 10 comments
Open

Refactor ally dialogue in UtBS #6158

Wedge009 opened this issue Oct 5, 2021 · 10 comments
Labels
Campaign UtBS The mainline Under the Burning Suns campaign Enhancement Issues that are requests for new features or changes to existing ones.

Comments

@Wedge009
Copy link
Member

Wedge009 commented Oct 5, 2021

#6155 highlights that the way the dialogue of a potential ally is handled is not good. Basically it appears in many cases strings are printed in quadruplicate to accommodate the four different potential ally characters. Suggest refactoring this not just to avoid the redundant strings and corresponding translation effort but also to avoid complications with female characters as @CelticMinstrel mentioned.

@Wedge009 Wedge009 added Enhancement Issues that are requests for new features or changes to existing ones. Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign labels Oct 5, 2021
Wedge009 added a commit to Wedge009/wesnoth that referenced this issue Oct 6, 2021
With four potential allies in the campaign, there is a lot of dialogue duplication. This refactoring attempts to consolidate the text.
Additionally, the ally ID was recorded in the variable ally_name which can be confusing, so rename it to ally_id instead.
Wedge009 added a commit to Wedge009/wesnoth that referenced this issue Oct 6, 2021
With four potential allies in the campaign, there is a lot of dialogue duplication. This refactoring attempts to consolidate the text.
Additionally, the ally ID was recorded in the variable ally_name which can be confusing, so rename it to ally_id instead.
Wedge009 added a commit to Wedge009/wesnoth that referenced this issue Oct 8, 2021
With four potential allies in the campaign, there is a lot of dialogue duplication. This refactoring attempts to consolidate the text.
Additionally, the ally ID was recorded in the variable ally_name which can be confusing, so rename it to ally_id instead.
Improving Under the Burning Suns automation moved this from To do to Done Nov 17, 2021
Asheviere pushed a commit to Asheviere/wesnoth that referenced this issue Feb 18, 2022
With four potential allies in the campaign, there is a lot of dialogue duplication. This refactoring attempts to consolidate the text.
Additionally, the ally ID was recorded in the variable ally_name which can be confusing, so rename it to ally_id instead.
@MultiSeptim
Copy link

As a translator (czech), I don't mind multiple similar text strings (because they are easy to translate), but the inappropriate use of variable names, which are difficult to incorporate into translation. And Czech still uses Latin, I can't imagine what the troubles will be in other scriptures.

Wedge009 added a commit to Wedge009/wesnoth that referenced this issue Mar 14, 2022
…nslated text, the use of variables adds challenges to some translations.

The particular discussion which led to this reversion relates to the Czech translation.

Revert "Refactor handling of allies in UtBS. Resolves wesnoth#6158."
Revert "Add po hints where self-referential troll names have been replaced with variables."

This reverts commit 541f0a1.
This reverts commit 4a3ddfb.
@Wedge009
Copy link
Member Author

Re-opening due to reversion of #6165 via d5b95e6. I suspect it's unlikely that avoiding text redundancy and avoiding variables for translation efforts can be resolved together.

I want to highlight that the refactoring was not the only place where variables are used in translated text. So I consider that the question of what to do in those circumstances may also be relevant. A couple of examples:

[message]
speaker=Kai Krellis
message= {WHISPER ( _ "Thank you, $spy_unit.name|. I will keep that in mind, but it may be hard for us to stop him with that teleporting trick he has.")}
[/message]

male_message= _ "Should $unit.name| read the book?"
female_message= _ "female^Should $unit.name| read the book?"

For more background, see the discussion in hrubymar10/wesnoth-cs#209.

@Wedge009 Wedge009 reopened this Mar 14, 2022
Improving Under the Burning Suns automation moved this from Done to In progress Mar 14, 2022
@CelticMinstrel
Copy link
Member

Reopened due to a discussion in hrubymar10/wesnoth-cs#209.

@stevecotton
Copy link
Contributor

stevecotton commented Mar 14, 2022

I think d5b95e6 reverted too much stuff, as it included reverting the variable-name change from ally_name to ally_id.

Edit: I'd mistakenly assumed this was also affecting 1.16, hadn't expected the translation team to already be working on master.

@Wedge009
Copy link
Member Author

I think d5b95e6 reverted too much stuff, as it included reverting the variable-name change from ally_name to ally_id.

You previously made the point to try to make some sort of compatibility, despite the fact this was only applied to 1.17.x. That's what #6301 was supposed to include. The renaming made sense to me, but if you want to keep it then do you still require the (unfinished) 'compatibility' work to go with it? If not, it's probably better to just rename the variables separate to the text consolidation.

Edit: I'd mistakenly assumed this was also affecting 1.16, hadn't expected the translation team to already be working on master.

Pretty sure #6165 was only applied to master, not 1.16.

@stevecotton
Copy link
Contributor

The renaming made sense to me, but if you want to keep it then do you still require the (unfinished) 'compatibility' work to go with it? If not, it's probably better to just rename the variables separate to the text consolidation.

@knyghtmare has put on the roadmap that he'll rewrite S12 for 1.17.5, so I'm happy if the current S12 doesn't have compatibility with 1.16.x save files. Does that finish the compatibility work with the currently-prepared code?

Sorry if I'm missing details here.

@Wedge009
Copy link
Member Author

Details in #6165 discussion. The blocker was S12 not being conducive to the compatibility code I attempted. I've reinstated the renaming separately in 170c124.

@cooljeanius
Copy link
Contributor

The renaming made sense to me, but if you want to keep it then do you still require the (unfinished) 'compatibility' work to go with it? If not, it's probably better to just rename the variables separate to the text consolidation.

@knyghtmare has put on the roadmap that he'll rewrite S12 for 1.17.5, so I'm happy if the current S12 doesn't have compatibility with 1.16.x save files. Does that finish the compatibility work with the currently-prepared code?

Sorry if I'm missing details here.

Is this still on the roadmap? ISTR him saying he'd given up on it...

@Wedge009
Copy link
Member Author

You are referring to the campaign rewrite?

For this issue specifically, I recall I had to revert the consolidation effort because of translation overhead. The discussion is in my (reverted) PR and the linked Czech translation discussion.

@cooljeanius
Copy link
Contributor

You are referring to the campaign rewrite?

Well, Knyght rewriting S12 of the campaign specifically; I think it was issue #6142 (currently postponed...)

@stevecotton stevecotton added Campaign UtBS The mainline Under the Burning Suns campaign and removed Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign labels Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Campaign UtBS The mainline Under the Burning Suns campaign Enhancement Issues that are requests for new features or changes to existing ones.
Development

No branches or pull requests

5 participants