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

Move display-related string colorization functions out of Character class #50972

Merged
merged 7 commits into from
Aug 27, 2021

Conversation

wapcaplet
Copy link
Contributor

@wapcaplet wapcaplet commented Aug 22, 2021

Summary

None

Purpose of change

To de-bloat the Character class and make display-related functions more logically named.

Describe the solution

Move and rename these functions out of the Character class, into a display namespace in panels.h|cpp:

  • Character::get_thirst_description -> display::thirst_text_color
  • Character::get_hunger_description -> display::hunger_text_color
  • Character::get_fatigue_description -> display::fatigue_text_color
  • Character::get_pain_description -> display::pain_text_color
  • Character::get_weight_description -> display::weight_text_color
  • Character::get_weight_string -> display::weight_string
  • Character::get_weight_long_description -> display::weight_long_description
  • static weariness_description -> display::weariness_text_color

The new text_color names were chosen to reflect the fact that these functions return a (string, color) or (translation, color) pair.

Leave the existing activity_level_str function in place in the new namespace (formerly called activity_level).

Describe alternatives you've considered

Going the other way around (migrating display functions from panels into character) seemed like the wrong choice, considering the massive size of the Character class already. These functions are only loosely coupled to the class and don't really belong here.

Considering the sort of general-purpose usage of these functions throughout the codebase (not just for the panels), perhaps panels.h|cpp is not ideal either; however the panels are the biggest consumer of these functions (and the incoming panel widgets in #50945 do too), plus there is a collection of similar functions already in there (dex_string, focus_color, get_moon and so forth), so I think it's a good fit.

Testing

Play the game and ensure the colored descriptive text in sidebar still looks OK.

Additional context

This is mainly in support of #50945

Probably easiest to review commit-by-commit (one or two functions factored out per commit).

@wapcaplet wapcaplet added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Info / User Interface Game - player communication, menus, etc. labels Aug 22, 2021
@actual-nh actual-nh removed the Info / User Interface Game - player communication, menus, etc. label Aug 22, 2021
@actual-nh
Copy link
Contributor

actual-nh commented Aug 22, 2021

I am tempted to call reductions in the size of character.cpp a bugfix... EDIT: Sorry, didn't realize you were editing the labels at the same time as I was.

@wapcaplet
Copy link
Contributor Author

Failing test case in "Basic Build / Basic Build and Test (GCC 9, Curses, LTO)" appears unrelated:

(~[slow] ~[.])=> ../tests/field_test.cpp:215: FAILED:
(~[slow] ~[.])=>   CHECK( fields_test_turns() < time_limit_turns )
(~[slow] ~[.])=> with expansion:
Error: (~[slow] ~[.])=>   1800 (0x708) < 1800 (0x708)
(~[slow] ~[.])=> with message:
(~[slow] ~[.])=>   Fire should've spread to the far point in under 1800 turns

src/panels.h Outdated Show resolved Hide resolved
This namespace is presently only used as a wrapper to make
`activity_level_str` accessible to the crafting GUI, per CleverRaven#49026.

I intend to move several other descriptive functions defined in
`panels.h|cpp` into this namespace, making it a more general-purpose
collection of descriptive text and color translation methods. Hence, the
namespace is here renamed to `display`.
Move to `display` namespace in panels.h, and rename to hunger_text_color
and thirst_text_color, taking a Character instance as an argument.
wapcaplet and others added 4 commits August 26, 2021 06:38
Move these three functions out of `character.h|cpp` and into
`panels.h|cpp` with a `const Character` argument:

- get_weight_string => display::weight_string
- get_weight_description => display::weight_text_color
- get_weight_long_description => display::weight_long_description
Relocate to `panels.h|cpp` and rename to `fatigue_text_color`.
Move these display-related functions into `panels.h|cpp` and rename to
`pain_text_color` to match their brethren.
Apply suggestion from code review.

Co-authored-by: Kevin Granade <[email protected]>
@wapcaplet
Copy link
Contributor Author

wapcaplet commented Aug 26, 2021

Something got screwed up in rebasing after resolving conflicts with master. I will investigate later today - putting back in Draft for the moment.

Never mind, looks like I somehow had some stale .o files. Compiles OK now, and all tests pass locally, except a couple in vehicle_part_test.cpp and vehicle_power_test.cpp that appear to be unrelated, and have been raised in the CDDA Discord development channel recently as well (see #51082).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Info / User Interface Game - player communication, menus, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants