Fix T88801: Positioning of Menu Mnemonic Underlines
ClosedPublic
Actions

Authored by Harley Acheson (harley) on Jun 7 2021, 12:00 AM.

Details

Reviewers

Campbell Barton (campbellbarton)

Group Reviewers

User Interface

Maniphest Tasks

T88801: Mnemonics underline in menus is off center and needs some extra padding with custom fonts

Commits

rB0fcc063fd99c: Fix T88801: Positioning of Menu Underlines

Summary

This patch corrects small errors in both horizontal and vertical positioning of the little underlines shown under some hotkey letters in menus.

The improved horizontal alignment is a subtle change, just from better measuring.

The vertical alignment is drastically changed. Currently we are just printing an underscore glyph in that character's natural position. But some fonts design that quite high so that camel_case looks good. And using a consistent height is a problem for underlining characters with descenders. A letter like "j" or "Q" needs an underline that is slightly lower.

This also replaces the printing of the underscore character with a pixel-aligned line of the same width. And with calculated height that changes with font size but is allowed to be no smaller than user line width. It is much easy to see.

The following image shows our default font before (left) and after (right). You wil notice that everything has improved horizontal centering, especially noticeable in "Save" and "Link". You should see improved vertical positioning for "Append" and "Quit":

A more extreme example is using monospaced font ttf-iosevka-fixed-slab. Every single underline is vastly improved on the right.

Diff Detail

Repository: rB Blender

Event Timeline

Harley Acheson (harley) requested review of this revision.Jun 7 2021, 12:00 AM

Harley Acheson (harley) created this revision.

Harley Acheson (harley) added a task: T88801: Mnemonics underline in menus is off center and needs some extra padding with custom fonts.Jun 7 2021, 12:02 AM

Thanks Harley, it's a major improvement for custom fonts, it looks pretty good!

1 change requested otherwise looks good to me.

source/blender/editors/interface/interface_widgets.c
2206–2211	Since this is getting more involved than a pair of numbers, more readable to make this into a struct. eg: struct UnderlineData { /** The offset of the character to test. / int str_test_offset; / The underline with in pixels. / int char_width_px; /* Write the X,Y offset here. */ int r_offset_px[2]; };

This revision now requires changes to proceed.Jun 7 2021, 3:16 AM

@Campbell Barton (campbellbarton): ...more readable to make this into a struct.

Yes, Thanks! Much better like that.

Campbell Barton (campbellbarton) accepted this revision.Jun 7 2021, 5:00 AM

This revision is now accepted and ready to land.Jun 7 2021, 5:00 AM

Note, this patch could use an extra cleanup pass (no extra review iteration is needed).

source/blender/editors/interface/interface_widgets.c
1934	This can be a `size_t`, avoids the cast in the callback function.
1934–1935	The `ul_` prefix is redundant as destruct is already named to make it clear this deals with underline data.
1950	use `const`.
2209	`round_fl_to_int`.
2210	`MAX2` -> `max_ii`.
2211	Avoid relying on order of struct members. struct UnderlineData ul_data = { .ul_str_offset = ul_str_offset, .ul_width_px = ul_width, };