fix(gui): show the macOS Option glyph in toolbar tooltips, not "Alt+X" - #664
Conversation
got3nks
left a comment
got3nks
left a comment
There was a problem hiding this comment.
Right approach, and I confirmed the msgid delta vs master is zero — the suffix staying outside _() did its job.
One fix before merge: the raw ⌥ is a high-byte literal in a narrow string, converted to wxString via the locale-dependent wxConvLibc. That's the first such literal in src/ (every other non-ASCII in the tree is either in a comment or goes through wxString::FromUTF8), and it lands on the one platform where we already have a documented narrow-conversion quirk — macOS reports GetSystemEncodingName() as Mac OS Roman, which is why #318 had to force UTF-8 under __WXOSX__. Under a non-UTF-8 conversion those bytes render as mojibake (⌥) or collapse to empty instead of the glyph.
Safer to build it from the codepoint, which also can't be mangled by an editor or toolchain re-encoding the source file:
#ifdef __WXMAC__
// U+2325 OPTION KEY -- built from the codepoint so it doesn't depend on
// the narrow->wide conversion (macOS reports Mac OS Roman, see #318).
return " (" + wxString(wxUniChar(0x2325)) + letter + ")";
#else
return " (Alt+" + letter + ")";
#endifSame output as yours — (⌥N), no separator, matching both the Apple convention and what the Navigate menu already renders.
Also worth confirming: did you actually hover a toolbar button on macOS and see the ⌥ render? Tooltips are hover-only, so this wouldn't show up in a build-and-run check either way.
…literal got3nks's review on amule-org#664: a raw ⌥ literal in a narrow string is converted to wxString via the locale-dependent wxConvLibc, and macOS reports GetSystemEncodingName() as Mac OS Roman (the same narrow-conversion quirk amule-org#318 had to work around under __WXOSX__ elsewhere in the tree). Under a non-UTF-8 conversion those bytes would render as mojibake or collapse to empty instead of the glyph. wxUniChar(0x2325) builds it from the codepoint directly, sidestepping the conversion entirely. po/ regenerated: zero msgid changes, confirming the suffix still never touches a translated string. Verified for real this time, not just build-and-run: launched the app and hovered a toolbar button on macOS -- the tooltip reads "Finestra reti (⌥N)", the glyph renders correctly.
|
Fixed in 93d9d52 — switched to `wxUniChar(0x2325)` exactly as suggested, sidestepping the narrow-conversion path entirely. And yes, fair catch — I hadn't actually hovered a toolbar button, only confirmed build+po diff. Did it for real this time: launched the app and hovered "Networks" on macOS, tooltip reads "Finestra reti (⌥N)" — glyph renders correctly, no mojibake. (Screenshot confirmed by a human on this end, not just me — wanted a second pair of eyes given tooltip hover isn't something I can reliably automate/screenshot myself on macOS.) po/ confirmed still zero msgid changes. |
|
Fix verified — Only thing left is mechanical: the |
Follow-up to amule-org#642 per got3nks's post-merge comment: the macOS Navigate menu renders its own "\tAlt+N"-style accelerator using the platform's native glyph automatically (Cocoa substitutes Alt for Option/⌥ on a real NSMenuItem key equivalent), but the toolbar tooltips are plain text, so wx never touches them -- they kept reading "(Alt+N)" even on macOS, inconsistent with the menu right above them. Added TabAccelSuffix(), a small platform-conditional helper building " (⌥N)" on __WXMAC__ and " (Alt+N)" everywhere else, kept outside _() so translated msgids are untouched -- confirmed via the po/ diff after regenerating: zero added/removed/changed msgids, purely line-number churn. Verified: built and ran both amule and amuleGUI, full unit test suite passing (27/27).
…literal got3nks's review on amule-org#664: a raw ⌥ literal in a narrow string is converted to wxString via the locale-dependent wxConvLibc, and macOS reports GetSystemEncodingName() as Mac OS Roman (the same narrow-conversion quirk amule-org#318 had to work around under __WXOSX__ elsewhere in the tree). Under a non-UTF-8 conversion those bytes would render as mojibake or collapse to empty instead of the glyph. wxUniChar(0x2325) builds it from the codepoint directly, sidestepping the conversion entirely. po/ regenerated: zero msgid changes, confirming the suffix still never touches a translated string. Verified for real this time, not just build-and-run: launched the app and hovered a toolbar button on macOS -- the tooltip reads "Finestra reti (⌥N)", the glyph renders correctly.
Mechanical rebase to resolve po/ conflicts against master's amule-org#665 -- no source changes here.
93d9d52 to
a868dc0
Compare
|
Rebased onto current master and regenerated po/ (a868dc0) — clean now. |
got3nks
left a comment
got3nks
left a comment
There was a problem hiding this comment.
Rebase is clean and the codepoint fix is verified — merging. Tier-1 clang-tidy was checked locally on the diff.
2 participants
Follow-up to #642 per got3nks's post-merge comment: the macOS Navigate menu renders its own
"\tAlt+N"-style accelerator using the platform's native glyph automatically (Cocoa substitutes Alt for Option/⌥ on a realNSMenuItemkey equivalent), but the toolbar tooltips are plain text, so wx never touches them — they kept reading"(Alt+N)"even on macOS, inconsistent with the menu right above them.What changed
Added
TabAccelSuffix(), a small platform-conditional helper building" (⌥N)"on__WXMAC__and" (Alt+N)"everywhere else, kept outside_()per the suggested approach so translated msgids are untouched.Testing
Built and ran both
amuleandamuleGUIlocally, full unit test suite passing (27/27). Confirmed via thepo/diff after regenerating: zero added/removed/changed msgids, purely line-number churn.Purely cosmetic, no rush per the original comment — just closing the loop.