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

gui: replace deprecated printEx with printf #51

Merged
merged 2 commits into from Apr 15, 2020

Conversation

taiyu-len
Copy link
Contributor

Replaces printEx functions with the printf alternatives in the gui.

came across an assertion failure when trying the sample hmtool due to it trying to draw one of the widgets off screen.

also, avoid passing in the label directly as that causes problems if it conains any % formatting characters

@HexDecimal
Copy link
Collaborator

The printEx and printf functions are not equivalent. printEx expects EASCII and printf expects UTF-8. The TCOD_CHAR_* symbols not valid with printf, you'd need to use the Unicode codepoint for those glyphs and remap the TCOD layout at:

// Clones for frame drawing:

So far this does break the hmtool, but I'd rather update the hmtool than keep the deprecated function calls. The landmass status now has %%, and the ↑↓ z button is missing its special glyphs. The glyphs would need to be updated both in the library and in the example, unless the example is changed to use a CP437 tileset.

@HexDecimal HexDecimal self-assigned this Apr 14, 2020
@@ -37,10 +37,11 @@ void ToggleButton::render() {
con->setDefaultBackground(mouseIn ? backFocus : back);
con->setDefaultForeground(mouseIn ? foreFocus : fore);
con->rect(x,y,w,h,true,TCOD_BKGND_SET);
const char check = pressed ? TCOD_CHAR_CHECKBOX_SET : TCOD_CHAR_CHECKBOX_UNSET;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If printf is used then this will need to be Unicode. The correct codes are listed here (U+2610/U+2611). These codes also need to be mapped to the TCOD layout.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The codes no longer need to be remapped on the develop branch.

UTF-8 example, check is now a char pointer:

const char* check = pressed ? "\u2611": "\u2610";
if (label) {
  con->printf(x,y,TCOD_BKGND_NONE,TCOD_LEFT,"%s %s", check, label);
} else {
  con->printf(x,y,TCOD_BKGND_NONE,TCOD_LEFT,"%s", check);
}

@taiyu-len
Copy link
Contributor Author

Ahh, quite a bit more involved then i expected. didnt put much thought behind this, my bad.
I did manage to get it all to work for hmtool, but its probably best to scrap this and go for a more thorough set of changes.
and as it breaks user code probably whenever the next version to make breaking changes is in.

a few things noticed while getting it to work
-- Console::printFrame makes use of similar deprecated functions that expect eascii. that needed to be updated to use printf to show the ↑↓ z consistently
could add a printfframe function instead and deprecate it too i guess.

-- toolbar which uses printframe passes in the name directly to the format argument, which can be unsafe if it uses % characters.

-- TCOD_sys_decode_font_ in sys_sdl_c.c that you mention has 2 branches adding mappings via TCOD_sys_map_clone_,
the true branch properly uses TCOD_chars_t enum values, and has a few more mappings,
while the false branch uses magic numbers and has less mapping.
considering that the magic numbers are exactly whats defined in the enum, i assume it would be preferable to just make both branches share the same set of mappings.

@HexDecimal
Copy link
Collaborator

Ahh, quite a bit more involved then i expected. didnt put much thought behind this, my bad.

Welcome to libtcod development, encodings are tricky to work with everywhere. Hopefully I've made things better since I took over as the primary maintainer, but it's hard to tell.

I did manage to get it all to work for hmtool, but its probably best to scrap this and go for a more thorough set of changes.

I didn't see any problems with your changes so far other than the need to upgrade non-deprecated functions to Unicode. If you push more changes then I'll comment on them, and I can take over if it ever gets too complex.

and as it breaks user code probably whenever the next version to make breaking changes is in.

That would be true for other parts of the API and I'd make a big deal about it if that were the case. Semantic Versioning forbids making breaking changes to the public API without a major version increment, but the GUI headers were never properly documented, and are technically not part of the public API, just some of the samples happen to use them.

could add a printfframe function instead and deprecate it too i guess.

I probably missed it because it was part of the C++ API. I have major plans on refactoring the C++ API to use actual namespaces instead of namespace classes. So either I'll handle that or we'll coordinate on it.

toolbar which uses printframe passes in the name directly to the format argument, which can be unsafe if it uses % characters.

I lot of the old functions do that, fixing it will always break the rare cases where % was used, but the long term plan is to remove all code which passes format strings directly and add the TCOD_FORMAT attribute to all non-deprecated printf style functions. At this point the C API shouldn't have any more issues for console printing, so the C++ API just needs to call into the correct C functions.

the true branch properly uses TCOD_chars_t enum values, and has a few more mappings,
while the false branch uses magic numbers and has less mapping.

You have this backwards, while the TCOD_chars_t enum has names for its constants the numbers they refer to are a magical standard that I don't think is used anywhere other than TCOD, but the magic numbers passed to TCOD_sys_map_clone_ are from the Unicode standard and are not magic. For example:

vs

  "↑↓ z",

With a Unicode mapping and UTF-8 functions you can use the special glyphs directly in the strings of C and C++ code. It's harder to do that with the EASCII functions since those numbers could mean anything, so TCOD_chars_t is used, but because it's non-standard it breaks with anything other than the TCOD layout.

It's only a partial mapping because I've been lazy. The correct thing to do would be to go through the entire TCOD layout and map them all to Unicode. Other layouts like CP437 and any font loaded from a BDF or TTF file are already fully mapped to Unicode instead of TCOD_chars_t.

@taiyu-len
Copy link
Contributor Author

regarding the magic numbers, i wasnt referring to the unicode argument, but the 2nd argument.

for example from one branch

TCOD_sys_map_clone_(0x2500, TCOD_CHAR_HLINE);

and in the other branch
TCOD_sys_map_clone_(0x2500, 0xC4);

they both pass in the same values, but the second passes it in as 0xC4 rather then the equivalent TCOD_CHAR_HLINE which the first does.

anyway, i guess i can go and cleanup the changes ive made and commit the rest of the gui changes.
should i also commit the printframe change, or leave that alone for now

@HexDecimal HexDecimal mentioned this pull request Apr 15, 2020
3 tasks
@HexDecimal
Copy link
Collaborator

Sorry for the mistake. That 2nd branch is for people who end up using a raw layout for a CP437 tileset, I'm forced to guess that the raw layout might be CP437 and remap some of the special characters to Unicode so that any pre-existing code doesn't break. The alternative would be to map any raw tilesets to the raw codepoints and CP437 Unicode codepoints simultaneously.

It could be removed, but only in an a major version release.

You could add the printframe function, but TCODConsole and other namespace classes will one day be removed, and nobody will know that your new function even exists unless it's in the documentation, which I've been terribly neglecting. Hasn't stopped me from adding C++ functions and documenting them through the deprecation warnings of the functions they replace.

@HexDecimal
Copy link
Collaborator

I pushed some changes to the TCOD layout. The develop branch will load TCOD fonts as Unicode. Just pull from your upstream/develop branch and continue.

There is another alternative to the magic mappings in the 2nd branch. I've been considering procedurally generating the box drawing glyphs needed by most of the drawing functions, but it was never a high enough priority for me to start working on.

@taiyu-len
Copy link
Contributor Author

thanks, one thing im stuck on is getting it to draw the arrows that the Z label needs.
before i just added TCOD_sys_map_clone_(/*arrow unicode*/, TCOD_CHAR_ARROW_N); to the TCOD_sys_decode_font_ function, but as youve removed those calls, im not sure what to do now to get arrows outputed.

@HexDecimal
Copy link
Collaborator

You no longer need to mess with TCOD_sys_map_clone_. I took care of that and the changes I made should be backwards compatible. I've loaded and mapped both the Unicode values and the old ones. The arrows should show if they're in Unicode or if they haven't been touched yet.

@taiyu-len
Copy link
Contributor Author

Ok, i got it, had to tweak the libtcod.cfg file to get it showing right, as the mapping from the newly added tcod_codec_ are only done when using fontTcodLayout=true.

anyway, this commit fixes the issues,
-- checkbox is corrected,
-- toolbar uses the console_printfframe directly instead of printframe
-- hmtool ^v z label shows uses unicode arrows.

@HexDecimal
Copy link
Collaborator

Sorry for that. I forgot that the old samples have never been updated in years. They'll need to be switched to use the set custom font function so that they can handle Unicode, and I'll handle that myself.

@HexDecimal HexDecimal merged commit 1e0675a into libtcod:develop Apr 15, 2020
@taiyu-len taiyu-len deleted the gui_replace_printex branch April 15, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants