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

Special notes #4305

Merged
merged 3 commits into from Sep 12, 2019

Conversation

@CelticMinstrel
Copy link
Member

commented Sep 2, 2019

As mentioned in my comment on #4297, this adds a [special_note] tag as a way to make special notes neater.

For backwards compatibility, the old {SPECIAL_NOTES_*} macros are deprecated and new {SPECIAL_NOTE_*} macros have been added that produce a [special_note] tag... I admit this is confusing, but there's not a lot of options here, so...

I even updated wmllint to handle this, though I'm not 100% confident in my changes there.

On the one hand, this should render #4297 obsolete (because the strings have been reverted to once more omit the bullet), but on the other hand, it moves a whole slew of strings from "wesnoth" to "wesnoth-help" by adding a gettext domain declaration in help_topic_generators.cpp. I figure this is acceptable since it's a dev branch, though.

I only migrated core units to use the new system, but I'm willing to do it for all the campaigns as well.

@Wedge009

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

I don't think I'm qualified to review every detail but from what I can see it looks right to me. As mentioned in #4297 I think this is a good way to go to handle the new bullet point format, plus the bullet points are only in 1.15 for now so it makes sense that this is only in master. If it means the strings get moved from wesnoth to wesnoth-help, so be it, it's a once-off change that makes sense (the bullet points addition already changed those strings anyway). And I suppose it makes sense that these strings be in wesnoth-help as well.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

@Wedge009 - Just noting, the strings that are moved to wesnoth-help aren't the special notes strings (they were already in that textdomain) but rather any strings used by the help topic generators in the C++.

@Wedge009

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Ah, no worries, sounds like it's not a problem at all to begin with.

@Wedge009 Wedge009 added the Help label Sep 4, 2019

@jostephd
Copy link
Member

left a comment

Thanks for working on this.

Looks good.

I even updated wmllint to handle this, though I'm not 100% confident in my changes there.

I did a quick test and wmllint reports errors when one but not both of ABILITY_DRAINS / SPECIAL_NOTE_DRAINS is present. I tested both cases.

I didn't review the C++ textdomain change.


#define SPECIAL_NOTES_SPIRIT
_"
• Spirits have very unusual resistances to damage, and move quite slowly over open water."#enddef
#deprecated 1 Use {SPECIAL_NOTE_SPIRIT} instead (note the singular), which generates a [special_note] tag.

This comment has been minimized.

Copy link
@jostephd

jostephd Sep 7, 2019

Member

🤔

I don't really like the idea of having {SPECIAL_NOTES_FOO} and {SPECIAL_NOTE_FOO}, the names are too similar. I feel like we're condemning some UMC author in the future to hours of hair-pulling to figure out why they are getting a weird error when they add a special note to their unit. These macros are few, as you say, but they're pretty commonly used: 43% of the units in ageless have special notes.

I don't have an alternative name to suggest, sorry.

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Sep 7, 2019

Author Member

Yeah, I understand what you mean. Unfortunately, I don't think it's possible to craft a macro that would work both in the ""+{...} context and also on a line by itself.

This comment has been minimized.

Copy link
@jostephd

jostephd Sep 9, 2019

Member

I'm not proposing to create a macro that would work in both contexts. I'm proposing to name the new macros differently so the names of the old and new macro won't be as easy to conflate with each other.

@@ -20,7 +20,8 @@
It is an irony that, with all their knowledge, and their unassuming monopoly thereof, the collective community of magi could likely rule society, were they ever to try. However, their true love is neither money, nor power, and those who see the study of magic as a means to such ends often lack the very conviction required for true mastery.
Physically frail, and lacking familiarity with combat, magi do possess certain arts which are of great utility in battle."+{SPECIAL_NOTES}+{SPECIAL_NOTES_MAGICAL}
Physically frail, and lacking familiarity with combat, magi do possess certain arts which are of great utility in battle."
{SPECIAL_NOTE_MAGICAL}

This comment has been minimized.

Copy link
@jostephd

jostephd Sep 7, 2019

Member

This is definitely an improvement over the previous system, and I support it as it stands. However, I wonder if it's possible to embed the [special_note] use into the ABILITY_CURES macro directly, as in [ability] id=cures special_note="..." [/ability]? (Or maybe a subtag rather than a key) This way, it won't be possible to have just one of SPECIAL_NOTES_CURES/ABILITY_CURES without the other (superseding the wmllint checks).

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel Sep 7, 2019

Author Member

I do want to do that, actually. That wouldn't entirely obsolete this system however, since not all special notes are connected to a special ability.

I was thinking I could merge this and then go on to add special_note= to abilities, but would you prefer I just do that in this PR instead?

This comment has been minimized.

Copy link
@jostephd

jostephd Sep 9, 2019

Member

I don't mind whether the further change will be done in this PR or separately. Thanks for asking.

@@ -23,7 +23,8 @@
[abilities]
{ABILITY_LEADERSHIP}
[/abilities]
description= _ "In direct contrast with other Dunefolk commanders, Warmasters focus heavily on rousing their troops and maintaining high morale. These generals are often found at the forefront of battle, leading by example and fighting and bleeding alongside their soldiers. While Warmasters usually leave the finer details of strategy to their Captains, they are certainly capable tacticians as well; many a battle has been won on an inventive or surprise maneuver that is often the creative byproduct of combat experience rather than formal training. Said to be a veteran of a thousand battles, a Warmaster is an expert leader that should never be underestimated."{SPECIAL_NOTES}+{SPECIAL_NOTES_LEADERSHIP}
description= _ "In direct contrast with other Dunefolk commanders, Warmasters focus heavily on rousing their troops and maintaining high morale. These generals are often found at the forefront of battle, leading by example and fighting and bleeding alongside their soldiers. While Warmasters usually leave the finer details of strategy to their Captains, they are certainly capable tacticians as well; many a battle has been won on an inventive or surprise maneuver that is often the creative byproduct of combat experience rather than formal training. Said to be a veteran of a thousand battles, a Warmaster is an expert leader that should never be underestimated."{SPECIAL_NOTES}

This comment has been minimized.

Copy link
@jostephd

jostephd Sep 7, 2019

Member

You left {SPECIAL_NOTES} at the end of the description here.

@@ -20,7 +20,9 @@
usage=fighter
description= _ "On the surface, it is not entirely clear what distinguishes a Luminary from other healers among the Dunefolk. Certainly, a Luminary may be marginally more knowledgeable, well-traveled, or skilled in combat compared to Herbalists or Apothecaries, but the difference is usually modest at best. Nevertheless, ‘Luminary’ is a formal title granted to the highest order of Dunefolk healers and grants these men both the greatest esteem and the greatest envy.
There is some speculation that a secret order exists among the Luminaries. These so called ‘Eminents’ hoard a great deal of forbidden knowledge and advise many Dunefolk leaders from the shadows. Such a thing has never been proven, but the fact remains that some Luminaries seem to hold much more influence than their abilities should afford."{SPECIAL_NOTES}+{SPECIAL_NOTES_CURES}+{SPECIAL_NOTES_REGENERATES}
There is some speculation that a secret order exists among the Luminaries. These so called ‘Eminents’ hoard a great deal of forbidden knowledge and advise many Dunefolk leaders from the shadows. Such a thing has never been proven, but the fact remains that some Luminaries seem to hold much more influence than their abilities should afford."{SPECIAL_NOTES}

This comment has been minimized.

Copy link
@jostephd

jostephd Sep 7, 2019

Member

You left {SPECIAL_NOTES} at the end of the description here.

@@ -1531,7 +1532,7 @@ def global_sanity_check(filename, lines):
precomment = nav.text
if '#' in nav.text:
precomment = nav.text[:nav.text.find("#")]
if "{SPECIAL_NOTES}" in precomment:
if "{SPECIAL_NOTE" in precomment:

This comment has been minimized.

Copy link
@jostephd

jostephd Sep 7, 2019

Member

Is this condition supposed to match both the new forms and the deprecated forms? If so, I'd recommend a comment.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

I did a quick test and wmllint reports errors when one but not both of ABILITY_DRAINS / SPECIAL_NOTE_DRAINS is present. I tested both cases.

Looks like that's because it's ABILITY_DRAIN / SPECIAL_NOTE_DRAIN.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Looks like that's because it's ABILITY_DRAIN / SPECIAL_NOTE_DRAIN.

I made the typo in the github comment, but I'm not sure if I also made it in the WML I was editing. (I may have typed the macro names in the github comment, rather than copied them.) Anyway, the errors I reported were expected. I was trying to say that I checked if wmllint still detects the case of "ability present, special note missing", and that it does correctly detect it.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Anyway, the errors I reported were expected. I was trying to say that I checked if wmllint still detects the case of "ability present, special note missing", and that it does correctly detect it.

Ah, I misunderstood then. I guess all is well on that front.

Regarding the naming of the new macros compared to the old, the only alternative I can think of is naming the new ones {NOTE_*}, ie, dropping the SPECIAL_ part. I think I actually considered this (as well as {SPECIALNOTE_*}) when creating this PR but decided to go with the current form.

@CelticMinstrel CelticMinstrel force-pushed the special_notes branch from 9c7ff31 to cd6eb6b Sep 11, 2019

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

...I just realized a possible problem with implementing the new macros in terms of the old - using either will likely give deprecation errors. :/

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

...I just realized a possible problem with implementing the new macros in terms of the old - using either will likely give deprecation errors. :/

So add a {WHATEVER_ARCANE} macro that contains the actual string, define SPECIAL_NOTES_ARCANE as

#define SPECIAL_NOTES_ARCANE
#deprecated 1 Use ... instead
{WHATEVER_ARCANE}#enddef

and define the new macro in terms of {WHATEVER_ARCANE}.

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

@jostephd Does that look about right?

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@CelticMinstrel Yes, perfect. Only two minor issues:

  1. missing #deprecated in SPECIAL_NOTES_ARCANE (just that one)
  2. Can delete the "note the singular" comment now :)

And thanks for changing from SPECIAL_NOTE_* to NOTE_* :-)

@Elvish-Hunter
Copy link
Contributor

left a comment

I found a spelling mistake (IOO instead of FOO) that was also in the original code. Other than that, the wmllint part looks good to me.

@@ -155,7 +155,7 @@
#
# A comment of the form
#
# #wmllint: match {ABILITY_FOO} with {SPECIAL_NOTES_IOO}
# #wmllint: match {ABILITY_FOO} with {NOTE_IOO}

This comment has been minimized.

Copy link
@Elvish-Hunter

Elvish-Hunter Sep 12, 2019

Contributor

This is a spelling mistake: IOO should be FOO

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

Given that it's "special notes" vs "note", I think it can't hurt to leave in the "note the singular" comment.

@CelticMinstrel CelticMinstrel force-pushed the special_notes branch from d3997e4 to b378449 Sep 12, 2019

@CelticMinstrel CelticMinstrel merged commit 4dd7824 into master Sep 12, 2019

0 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@Earth-Cake

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

I guess units in mainline campaigns (non-core) should have changed notes as well? @CelticMinstrel

@CelticMinstrel

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Yes, I plan to do that in the next few days if no-one else gets to it before then. It'll be a little easier since there's no need to allow for backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.