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

Document add_special_note #4404

Closed
jostephd opened this issue Sep 29, 2019 · 6 comments

Comments

@jostephd
Copy link
Member

commented Sep 29, 2019

From changelog:

In [effect]apply_to=profile, [add_special_note] and [remove_special_note] are supported.

Add these to https://wiki.wesnoth.org/EffectWML.

(and possibly to https://wiki.wesnoth.org/SingleUnitWML too? Or is that implied?)

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Note quite sure on the latter case; my impression is that [unit] implicitly supports most things from [unit_type], but I don't know if we need to explicitly document each one. There might be situations where the same thing is done differently in [unit] and [unit_type]?

As for the former, I just added that, but I'm uncertain about this part:

It must be an exact match, character-for-character, and be in the same textdomain.

I'd say it should work that way, but I'm not entirely sure if it does. It depends on how std::find works on a list of t_strings, which I believe depends on the implementation of t_string::operator==.

Also, looking at this implementation again, I think we should actually change it to use just [special_note] with an optional remove= key... that allows reusing the {NOTE_*} macros...

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

Note quite sure on the latter case; my impression is that [unit] implicitly supports most things from [unit_type], but I don't know if we need to explicitly document each one. There might be situations where the same thing is done differently in [unit] and [unit_type]?

Yeah, on a closer look, the top of https://wiki.wesnoth.org/SingleUnitWML says:

However [the [unit] tag] takes many of the same keys [as the [units][unit_type] tag) and thus can generally override the inherited properties from the associated [unit_type].

So maybe we should just list in [unit] the [unit_tag] keys/subtags that can't be used in [unit], or that have a different meaning.

As for the former, I just added that, but I'm uncertain about this part:

Thanks.

I'd say it should work that way, but I'm not entirely sure if it does. It depends on how std::find works on a list of t_strings, which I believe depends on the implementation of t_string::operator==.

Well, so long as it will remove the special note when the exact _ "foo" argument is given in the WML source, I think it's okay. Surely t_string::operator== will consider two instances of _ "foo" equal?

Can we just build a WML test case to check, or could this involve implementation-defined behavior of the C++ standard library?

Also, looking at this implementation again, I think we should actually change it to use just [special_note] with an optional remove= key... that allows reusing the {NOTE_*} macros...

Or we could define a {REMOVE_NOTE_ARCANE} macro in terms of {INTERNAL:SPECIAL_NOTES_ARCANE}, but your way is nicer :)

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Well, so long as it will remove the special note when the exact _ "foo" argument is given in the WML source, I think it's okay. Surely t_string::operator== will consider two instances of _ "foo" equal?

Can we just build a WML test case to check, or could this involve implementation-defined behavior of the C++ standard library?

I'm pretty sure the operator== would consider these two strings equal:

#textdomain test
string1=_"Hello World"
string2=_"Hello World"

I'm not sure whether it would consider these two strings equal, though:

#textdomain test
string1=_"Hello World"
#textdomain trial
string2=_"Hello World"

It might even depend on the current language, if operator== simply compares the translated text. I think in the case of note removal it would be ideal to consider the latter case to be unequal.

It's probably possible to create a WML test case to check this.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

t_string equality is implemented here:

wesnoth/src/tstring.cpp

Lines 541 to 544 in 2c2d1f8

bool t_string_base::operator==(const t_string_base& that) const
{
return that.translatable_ == translatable_ && that.value_ == value_;
}

The textdomain id is embedded in value_.

There's also a translated_value_ member, so I think the comparison depends on textdomain but not on UI language. Shall we add that to the wiki and close this issue?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

That sounds like the behaviour I expected.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

Thanks! Nothing more to do here.

@jostephd jostephd closed this Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.