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

Re-enable the editor help, and use it to document the keyboard modifiers #4414

Merged
merged 1 commit into from Oct 8, 2019

Conversation

@stevecotton
Copy link
Contributor

stevecotton commented Sep 30, 2019

... with the idea that these pages will either be improved or hidden again before 1.16.0. I think the terrain-editor part is good enough to stay in 1.16. The scenario editor part would have to be updated by someone familiar with that part.

This puts the two tips that are currently in https://wiki.wesnoth.org/MapEditor in to the in-game text.

@stevecotton stevecotton force-pushed the stevecotton:editor_help branch from 28e88ae to 56b0fc1 Oct 4, 2019
@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 5, 2019

Re the "(filename in curly brackets)", the curly brackets could be escaped with << >>, but then they either have to be

Or this:

patch adding {LEFT_CURLY} and {RIGHT_CURLY} escape sequences
diff --git a/data/core/help.cfg b/data/core/help.cfg
index 2e4c02c8f0f..948ae4044e4 100644
--- a/data/core/help.cfg
+++ b/data/core/help.cfg
@@ -99,6 +99,8 @@
         title= _ "Introduction"
         text="<img>src=misc/logo-bg.png~BLIT(misc/logo.png) align=middle box=no</img>" + _ "
 
+foo{LEFT_CURLY}bar
+
 <italic>text='Battle for Wesnoth'</italic> is a turn-based fantasy strategy game somewhat unusual among modern strategy games. While other games strive for complexity, <italic>text='Battle for Wesnoth'</italic> strives for simplicity of both rules and gameplay. This does not make the game simple, however — from these simple rules arise a wealth of strategy, making the game easy to learn but a challenge to master.
 
 The following pages outline all you need to know to play Wesnoth. As you play, new information is added to the various categories as you come across new aspects of the game. For more detailed information on special situations and exceptions, follow the included links."
diff --git a/src/serialization/preprocessor.cpp b/src/serialization/preprocessor.cpp
index f1d76a67853..06a1f99fbfb 100644
--- a/src/serialization/preprocessor.cpp
+++ b/src/serialization/preprocessor.cpp
@@ -42,6 +42,8 @@ static lg::log_domain log_preprocessor("preprocessor");
 
 static std::string current_file_str = "CURRENT_FILE";
 static std::string current_dir_str = "CURRENT_DIRECTORY";
+static std::string left_curly_str = "LEFT_CURLY";
+static std::string right_curly_str = "LEFT_CURLY";
 
 // map associating each filename encountered to a number
 static std::map<std::string, int> file_number_map;
@@ -1476,6 +1478,12 @@ bool preprocessor_data::get_chunk()
 			} else if(symbol == current_dir_str && strings_.size() - token.stack_pos == 1) {
 				pop_token();
 				put(filesystem::directory_name(parent_.get_current_file()));
+			} else if(symbol == left_curly_str && strings_.size() - token.stack_pos == 1) {
+				pop_token();
+				put("{");
+			} else if(symbol == right_curly_str && strings_.size() - token.stack_pos == 1) {
+				pop_token();
+				put("}");
 			} else if(local_defines_ && (arg = local_defines_->find(symbol)) != local_defines_->end()) {
 				if(strings_.size() - token.stack_pos != 1) {
 					std::ostringstream error;

@stevecotton stevecotton force-pushed the stevecotton:editor_help branch from 56b0fc1 to 19ccca5 Oct 5, 2019
@stevecotton

This comment has been minimized.

Copy link
Contributor Author

stevecotton commented Oct 5, 2019

Thanks for the patch, that does look nicer.

However, I feel that it's better to merge the current PR without it, and then make a new PR for adding that patch to the preprocessor - I expect other devs to want to review adding new built-in macros, and to not expect such a change in a documentation PR.

(The recent force-push resolved the TODO in the clipboard section)

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 5, 2019

However, I feel that it's better to merge the current PR without it, and then make a new PR for adding that patch to the preprocessor - I expect other devs to want to review adding new built-in macros, and to not expect such a change in a documentation PR.

It goes without saying that I wouldn't have tried to sneak the change in under the radar. #4432

@sevu

This comment has been minimized.

Copy link
Member

sevu commented Oct 5, 2019

po: this is followed by an untranslatable "map_data=maps/01_First_Map.map",
with curly brackets which normally make the WML preprocessor try to include
that file.

Better recommend map_file=maps/01_First_Map.map, which does also not need curly brackets. (#4397)

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

stevecotton commented Oct 6, 2019

Better recommend map_file=maps/01_First_Map.map, which does also not need curly brackets. (#4397)

I mention that too, and I'd be happy to add that a recommendation to use map_file, but almost every scenario for 1.14 or earlier uses the "preprocessor include" style.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

CelticMinstrel commented Oct 6, 2019

I don't really understand your curly brace problem here, but in case it helps, I want to note that _ << >> works perfectly well as a translatable string.

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 6, 2019
@stevecotton

This comment has been minimized.

Copy link
Contributor Author

stevecotton commented Oct 6, 2019

I don't really understand your curly brace problem here, but in case it helps, I want to note that _ << >> works perfectly well as a translatable string.

Thanks, that solves this in a way that should be compatible with 1.14. I've checked and wmlxgettext correctly picks up the translatable string too.

@stevecotton stevecotton force-pushed the stevecotton:editor_help branch from 19ccca5 to 21f3402 Oct 6, 2019
@CelticMinstrel

This comment has been minimized.

Copy link
Member

CelticMinstrel commented Oct 6, 2019

It would have to, since we're already using that syntax for the name generators. :)

[/topic]
# wmllint: markcheck on

# wmllint: markcheck off
# This section uses << >> quotes so that curly brackets can be used without being interpreted by the preprocessor
# Note: wmllint will attempt to improve lines that include, without the space character, "map_data ="

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 8, 2019

Member

What's the purpose of this comment line? Does wmllint make changes to this block that should be reverted, after running wmllint and before committing its changes?

This comment has been minimized.

Copy link
@stevecotton

stevecotton Oct 8, 2019

Author Contributor

The version in the PR is okay, wmllint won't make changes to the file as it is now. The purpose of the comment is to stop someone thinking "there's usually no space in map_data space =", and removing that space.

If the map_data = on this comment line is changed to map_data=, then the file will be mangled by

wesnoth/data/tools/wmllint

Lines 2649 to 2654 in 704aadd

if map_only or (("map_data=" in line or "mask=" in line)
and line.count('"') in (1, 2)
and '""' not in line
and "{" not in line
and "}" not in line
and not within('time')):

Similarly, in the translatable string:

  • map_data=(filename in curly brackets) caused wmllint to mangle the file
  • map_data =(filename in curly brackets) doesn't match the regular expression, so wmllint doesn't mangle it
  • wmllint: ignore and similar aren't useful here - it still does the mangling

When I last pushed an update I hadn't realised that the workaround was no longer needed, because the line in the translatable string itself has literal curly brackets, it's okay both with and without that extra whitespace, because wmllint's line 2652 doesn't mangle lines with curly brackets.

What would be the clearest way to say this?

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 8, 2019

Member

Honestly, it sounds like we should fix wmllint to not look for map_data in comment lines, and maybe also to do a regexp test, re.search("map_data *=", line), instead of a substring test. But that's beyond the scope of the issue.

In the meantime, how about this:

# Note: If you change the following description string or this comment line, run wmllint on this file afterwards and make sure wmllint doesn't "improve" the map_data={...} lines.

which uses curlies in the comment to fool wmllint. But that's not cunning, that's just brittle.

We might also remove the spaces around the = sign in the translatable string. It's nice to write example code in a way that makes wmllint handle it correctly if someone copies it into a real addon.

By the way, I forgot to say, thanks for writing all the new prose here! I don't want to appear to be nitpicking, but that wmllint comment is the only thing I had a question about when I read the diff.

This comment has been minimized.

Copy link
@stevecotton

stevecotton Oct 8, 2019

Author Contributor

Thanks.

…ers (#4414)

... with the idea that these pages will either be improved or hidden again
before 1.16.0. I think the terrain-editor part will be good enough to stay in
1.16, but the scenario editor part needs to be updated by someone familiar with
it.

This puts the two tips that are currently in https://wiki.wesnoth.org/MapEditor
in to the in-game text.
@stevecotton stevecotton force-pushed the stevecotton:editor_help branch from 21f3402 to ed611f9 Oct 8, 2019
@stevecotton stevecotton merged commit ed611f9 into wesnoth:master Oct 8, 2019
0 of 3 checks passed
0 of 3 checks passed
label
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@stevecotton stevecotton deleted the stevecotton:editor_help branch Oct 8, 2019
@CelticMinstrel

This comment has been minimized.

Copy link
Member

CelticMinstrel commented Oct 9, 2019

I just realized something... you need quotes around the macro inclusion, don't you? Instead of map_data={stuff} it needs to be map_data="{stuff}". If you don't do that you'll probably get errors.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

stevecotton commented Oct 9, 2019

I just realized something... you need quotes around the macro inclusion, don't you? Instead of map_data={stuff} it needs to be map_data="{stuff}". If you don't do that you'll probably get errors.

Already fixed in the final committed version. 😁

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