Repository navigation
gui/tiletypes: remove COLOR_RESET use with Pen - #1604
ChrisJohnsen wants to merge 1 commit into
Conversation
COLOR_RESET should not be used with Pen. The original COLOR_RESET `fg` value here ended up being treated as COLOR_WHITE: for a Pen specified with just a single number, both COLORS_RESET (-1) and COLOR_WHITE (15) are decoded as "bold" grey-on-black (7-on-0) due to the masking done when decoding the Pen. The EMPTY_TOKEN is used to draw the "blank space" (non-text/non-checkbox parts) of the "more options" entries. Use of the `hpen` field overrides label text's normal "swap foreground and background" hover effect. Use UI_COLORS.HIGHLIGHTED to match the hover styling of the left side (text label) of the option entries (`OptionsPopup:init` `addOption` `left_specs` `pens_hover`). Since only a foreground color is specified for both the "left text" and the EMPTY_TOKEN, they both use Pen's default background color: black. Nicely, UI_COLORS.HIGHLIGHTED *is* COLOR_WHITE, so there isn't a visual change even if the active font has foreground pixels in its space characters (EMPTY_TOKEN is always drawn as space characters).
|
Needs changelog |
|
To be clear, there is no user-visible change in behavior or appearance after the change in this PR. The code change here is just to get rid of a "bad example" of using COLOR_RESET as a Pen color. Do you still want a changelog? I just re-tested this by patching a copy of 53.16-r2rc1 downloaded from Steam (which includes DFHack/dfhack#5910). I also used a "visible space" font (modified from the default font to include foreground pixels in the space character) to make sure there was no behavior change even in that edge case. Both before and after the change in this PR, the relevant UI looks like the top half of the image. BTW, the the original COLOR_RESET usage in this code is not actually affected by the changes in 5910 since that only touched |
2 participants
Ref DFHack/dfhack#5864
This Pen use of COLOR_RESET as a foreground color never actually caused any OOB reads (before DFHack/dfhack#5864) since the foreground value already had masking applied to it (related to bold handling).
The EMPTY_LABEL is used to fill (with space characters) the blank portions of the
gui/tiletypes"more options" entries that are not text label nor checkbox. Since spaces usually don't have any foreground pixels, the foreground used here mostly doesn't matter.To prevent a visual change for fonts that have foreground pixels in the space character, the foreground should be COLOR_WHITE since this is equivalent to what happens when a COLOR_RESET foreground is specified. Luckily,
UI_COLORS.HIGHLIGHTED(used for the text portion of the entry while in the hover state) is also COLOR_WHITE.Suggestion: Switch to specifying
hpen=UI_COLORS.HIGHLIGHTED. This matches the text portion of the options and produces an effective Pen that is unchanged from when COLOR_RESET was used. Thus, there are no visible changes.Pictured here:
The (top of the) "more options" window of
gui/tiletypeswith mouse hovering over the "Light" entry. This shows the COLOR_WHITE text under the mouse and the COLOR_GREY text for the non-hover entries. The bottom row shows what happens if the background of EMPTY_TOKEN is COLOR_WHITE instead of COLOR_BLACK.