Add word wrap support for more unicode whitespace characters.#488
Add word wrap support for more unicode whitespace characters.#488jacquesh merged 7 commits intojacquesh:mainfrom
Conversation
342a85a to
689a466
Compare
|
I have looked into the issue regarding the above crossed out text. For windows machines, it should not be necessary to set the locale because windows uses a characters unicode properties to determine whether the character is whitespace, rather than using the locale. Non-windows systems do utilize the locale for this purpose though. I am not too familiar with the f2k ecosystem, but if I'm not mistaken, this plugin is windows-only, but I would like confirmation. If it is windows-only, it is safe to remove the |
jacquesh
left a comment
There was a problem hiding this comment.
Seems like a well thought-out PR. Thanks!
I'm not mistaken, this plugin is windows-only
Correct. Not really for any particular ideological reasons. It's just the only environment I run fb2k on and was the easiest way to setup the UI. Support for Mac would be neat but would require rewriting all of the UI code :( So this is unlikely to change.
A comment at ui_lyrics_panel.cpp:482 says that you don't bother force wrapping if a single word doesn't fit on a line. Unless you have a reason to avoid it, I think you allow splitting words for better international language support. For example, Japanese does not typically contain any space characters, and individual lines can become long enough to overflow past the edges of the panel.
I'm 99% sure that my reason was "I didn't think about it very hard and the lyrics I have on my PC break just fine on only space" >_<
Proper unicode support would be great, but is also likely to be non-trivial to do. If you'd like to take a stab at it (in a separate PR), feel free. Really we'd need to replace our internal wrapping logic with some actual unicode text shaping. Off the top of my head I do not know what support (if any) the Win32 API has for unicode text shaping. I'd rather not pull in another large dependency like HarfBuzz if I can avoid it. If I needed a library for text shaping I'd probably use kb_text_shape. I've not used it myself but I've heard good things.
|
Apologies, I might have phrased my comment on "international language support" poorly. I simply meant that you should force wrap a line if it doesn't fit on the screen, even if there are no spaces in it. In the comment, you explained that the reason you don't already do this is because it's rare that a single word doesn't fit on the line, and I was trying to point out that there are international languages where it is common. I believe having a word break in an awkward place is better than not being able to see it the word at all. |
Co-authored-by: Jacques Heunis <github@jacquesheunis.com>
I replaced space detection logic for lyrics word wrap calculation, as well as a few other places it made sense such as blank line detection.
The new logic uses the
std::isspace/std::iswspacefunctions to determine if a character is whitespace based on the current c++ locale. This approach allows for flexible whitespace classification for any Unicode character.I introduced a new initialization class
LocaleInitthat sets the c++ locale to whatever the users current locale is (""is the users preferred locale).The plugin might behave the same way without this initialization, depending on what implementation of the standard library you are using. For example, even though it is not in the c++ specification, the MSVC implementation considers U+3000 to be a whitespace character by defaultmy tests for this may have been flawed, as i am struggling to find people corroborating this online. I will look into it more later. Regardless, the locale should be set for consitency. For consistencies sake, I decided to set the locale during initialization so that it doesn't rely on the implementations defaults.I wasn't sure whether to put the
find_first_spaceandfind_last_spacefunctions inwin32_util.cpporui_util.cpp, but I went withwin32_util.cppbecause the functions could theoretically be used outside of a UI context.Side note:
A comment at
ui_lyrics_panel.cpp:482says that you don't bother force wrapping if a single word doesn't fit on a line. Unless you have a reason to avoid it, I think you allow splitting words for better international language support. For example, Japanese does not typically contain any space characters, and individual lines can become long enough to overflow past the edges of the panel.