Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
preproc: Inherit parent textdomain on slowpath macro substitution (bu…
…g #22962)

Fixes bug #22962, reported on the forums as affecting the Swamplings
add-on from 1.10: <http://r.wesnoth.org/r41129>

This is most likely a regression from 1.8.x, as it doesn't affect 1.8.6
and the reporter on the forums claims it affected 1.10.x.

Direct substitutions do not require the instantiation of a new
preprocessor_streambuf, instead inheriting the current context's
streambuf, along with its textdomain. preprocessor_data() is then
constructed with an overruled textdomain which leads to a no-op as it is
identical to the streambuf's current textdomain.

Nested substitutions _do_ require a new streambuf created by means of
its copy constructor, which leaves the new streambuf's textdomain set to
the default of PACKAGE (defined as "wesnoth"). Then a new
preprocessor_data context is instantiated and instructed to overrule the
initial textdomain with the macro's textdomain. However,
preprocessor_data's base class constructor (`preprocessor`) has already
copied the streambuf's initial textdomain id to its previous textdomain
record, causing it to restore that textdomain on destruction instead of
the correct one.

Thus, when performing slowpath substitutions we need to make sure the
preprocessor_data and preprocessor constructors see the parent context's
streambuf textdomain instead of the wesnoth default. Later, the nested
preprocessor_data will restore the correct textdomain and emit a
trailing <FE>line directive for the parent if necessary.

The alternative of having the preprocessor_streambuf copy constructor
copy the original streambuf's textdomain would also fix this bug, but it
would break the intended behavior for slowpath file inclusions, which
always default to wesnoth. Although most WML containing translatable
strings have a #textdomain directive at the start nowadays (and this is
in fact enforced by wmllint), it's probably a bad idea to change the
current behavior in this case for UMC, at least for the 1.12.x stable
series.

I do not expect regressions from this commit and it certainly won't
cause compatibility issues or behavior changes outside i18n -- at least
for add-ons that are doing things right and not relying on translatable
strings having the wrong value.

DM, DW, DiD, EI, HttT, LoW, Liberty, SoF, THoT, TRoW, TSG, data/core,
and data/multiplayer all present preprocessor output differences before
and after this fix, but they amount to no changes to the parser's
output; unlike SotBE and UtBS, which benefit from the fix:

 * data/campaigns/Son_Of_The_Black_Eye/scenarios/07_The_Desert_of_Death.cfg:439
   had the {TURNS_RUN_OUT} core macro substitution bound to the
   campaign's own textdomain instead of wesnoth for some reason.

 * data/campaigns/Under_the_Burning_Suns/scenarios/05_A_Subterranean_Struggle.cfg:1749
   onwards (the ENEMY_ATTACK and ALLY_REINFORCEMENTS macros) bound
   several strings to the wesnoth textdomain instead of the campaign's
   textdomain.

I also tested this commit with my own add-on, After_the_Storm, which
presented similar results as DM et al above. Finally, Swamplings is
properly fixed by this change.
  • Loading branch information
irydacea committed Nov 18, 2014
1 parent 1bc5aa9 commit 1482e8d
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 0 deletions.
4 changes: 4 additions & 0 deletions changelog
Expand Up @@ -4,6 +4,10 @@ Version 1.12.0:
files.
* Language and i18n:
* Updated translations:
* WML engine:
* Fixed nested macros emitting incorrect bindings to the default 'wesnoth'
textdomain at the end of a substitution instead of the parent textdomain,
thus breaking localization of subsequent strings (bug #22962).

Version 1.12.0:
* Language and i18n:
Expand Down
3 changes: 3 additions & 0 deletions src/serialization/preprocessor.cpp
Expand Up @@ -1082,6 +1082,9 @@ bool preprocessor_data::get_chunk()
std::ostringstream res;
preprocessor_streambuf *buf =
new preprocessor_streambuf(target_);
// Make the nested preprocessor_data responsible for
// restoring our current textdomain if needed.
buf->textdomain_ = target_.textdomain_;
{ std::istream in(buf);
new preprocessor_data(*buf, buffer, val.location, "",
val.linenum, dir, val.textdomain, defines);
Expand Down

0 comments on commit 1482e8d

Please sign in to comment.