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.

Sign up for GitHub

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

Map editor overhaul #33619

Merged
merged 3 commits into from
Sep 5, 2019
Merged

Map editor overhaul #33619

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
50fa905
Refactor map editor code
Qrox Aug 26, 2019
8e5a722
Apply suggestions from reviews
Qrox Aug 29, 2019
a479cde
Use remove_color_tags in utf8_width to remove color tags
Qrox Aug 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions data/raw/keybindings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,18 @@
"name": "Page down",
"bindings": [ { "input_method": "keyboard", "key": "NPAGE" } ]
},
{
"type": "keybinding",
"id": "SCROLL_UP",
"name": "Scroll up",
"bindings": [ { "input_method": "mouse", "key": "SCROLL_UP" } ]
},
{
"type": "keybinding",
"id": "SCROLL_DOWN",
"name": "Scroll down",
"bindings": [ { "input_method": "mouse", "key": "SCROLL_DOWN" } ]
},
{
"type": "keybinding",
"id": "SELECT_ALL",
Expand Down Expand Up @@ -1221,16 +1233,13 @@
{
"type": "keybinding",
"id": "CONFIRM_QUIT",
"category": "EDITMAP_TERRAIN",
"category": "EDITMAP_FEATURE",
"name": "Confirm & quit",
"bindings": [ { "input_method": "keyboard", "key": "g" } ]
},
{
"type": "keybinding",
"id": "CONFIRM_QUIT",
"category": "EDITMAP_TRAPS",
"name": "Confirm & quit",
"bindings": [ { "input_method": "keyboard", "key": "t" } ]
"bindings": [
{ "input_method": "keyboard", "key": "e" },
{ "input_method": "keyboard", "key": "u" },
{ "input_method": "keyboard", "key": "t" }
]
},
{
"type": "keybinding",
Expand All @@ -1243,7 +1252,7 @@
"type": "keybinding",
"id": "START",
"category": "EDITMAP_SHAPE",
"name": "Select shape",
"name": "To start",
"bindings": [ { "input_method": "keyboard", "key": "z" } ]
},
{
Expand Down Expand Up @@ -1283,8 +1292,15 @@
"type": "keybinding",
"id": "EDIT_TERRAIN",
"category": "EDITMAP",
"name": "Edit terrain / furniture",
"bindings": [ { "input_method": "keyboard", "key": "g" } ]
"name": "Edit terrain",
"bindings": [ { "input_method": "keyboard", "key": "e" } ]
},
{
"type": "keybinding",
"id": "EDIT_FURNITURE",
"category": "EDITMAP",
"name": "Edit furniture",
"bindings": [ { "input_method": "keyboard", "key": "u" } ]
},
{
"type": "keybinding",
Expand Down
8 changes: 6 additions & 2 deletions src/catacharset.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,13 @@ int utf8_width( const char *s, const bool ignore_tags )
continue;
}
if( ignore_tags ) {
if( ch == '<' ) {
const char *const tag1 = "color_";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cataclysm-DDA/src/catacharset.cpp

Lines 153 to 159 in 50fa905

const char *const tag1 = "color_";
const char *const tag2 = "/color>";
if( !inside_tag && ch == '<' &&
( strncmp( ptr, tag1, strlen( tag1 ) ) == 0 || strncmp( ptr, tag2, strlen( tag2 ) ) == 0 ) ) {
inside_tag = true;
} else if( inside_tag && ch == '>' ) {

Can you clarify the purpose of this logic? I understand the addition of inside_tag to the bottom conditional, but I'm unclear on the intent of the string comparisons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be less performant, but it could make more sense to remove all the tags from the string.
Isn't remove_color_tags doing this actually?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the intent of the string comparisons.

For example, if we consider a string "< <color_cyan>Map editor</color> >", and set inside_tag to true on every occurrence of < (the previous logic), the characters that will be counted for text width will only be "Map editor >", without the starting bracket and space. If we check if < is followed by "color_" or "/color>", then it will properly count the starting characters. As I said in the OP this is only a temporary solution, but considering the state of how tags are handled currently in the many output functions, a proper fix would be complicated and should probably go in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think remove_color_tags would more clear here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would, but utf8_width needs fixing regardless since it's also called in other functions such as right_print. Maybe change utf8_width to also use remove_color_tags if ignore_tags is true?

const char *const tag2 = "/color>";
if( !inside_tag && ch == '<' &&
( strncmp( ptr, tag1, strlen( tag1 ) ) == 0 || strncmp( ptr, tag2, strlen( tag2 ) ) == 0 ) ) {

inside_tag = true;
} else if( ch == '>' ) {
} else if( inside_tag && ch == '>' ) {
inside_tag = false;
continue;
}
Expand Down
Loading