gui/tiletypes: remove COLOR_RESET use with Pen - #1604
Open
ChrisJohnsen wants to merge 1 commit into
Open
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.