TheQwertiest / foo_spider_monkey_panel

foobar2000 component that allows to use JavaScript to create CUI/DUI panels
https://theqwertiest.github.io/foo_spider_monkey_panel/
MIT License
274 stars 21 forks source link

Improve accuracy of CalcTextWidth if selected font has kerning pairs #140

Closed kbuffington closed 3 years ago

kbuffington commented 3 years ago

I discovered that if a font has kerning pairs and the text you wish to get the width of contains one (or more) of those pairs the value of CalcTextWidth will be off for each pair in the text. I couple You's or To's and things get really bad really quickly (the cursor and highlighted text should be directly adjacent to the previous Y):

bad calc

See discussion here where the suggestion was made to switch to using DrawText with DT_CALCRECT.

I made the change and now things look good again:

image

In the process of investigating I also discovered that despite the documentation, GdiDrawText does not return:

     * @return {Array<number>}
     *     index | meaning <br>
     *     [0] left   (DT_CALCRECT) <br>
     *     [1] top    (DT_CALCRECT) <br>
     *     [2] right  (DT_CALCRECT) <br>
     *     [3] bottom (DT_CALCRECT) <br>
     *     [4] characters drawn

even if DT_CALCRECT flag is set. I had no idea how you wanted to handle that, so I left it as is.

kbuffington commented 3 years ago

@marc2k3 CalcTextWidth will be affected in JSP as well, although over there at least GdiDrawText does not claim to return any values :)

marc2k3 commented 3 years ago

This new code doesn't like large strings/being called repeatedly as is done by the EstimateLineWrap function. It freezes fb2k indefinitely until you kill it with task manager. Try the last.fm bio sample.

kbuffington commented 3 years ago

Yeah, I'm seeing the same thing. For some reason I've never been able to attach to FSM's dll's to debug exactly what's going wrong. I can understand it being slow, but doesn't make sense why it would lock up FB.

Simple solution would be to have EstimateLineWraps's recursive functions continue to always call GetTextExtentPoint32 I suppose.

kbuffington commented 3 years ago

Should be safe now as it only calls DrawText from CalcTextWidth, and in all other cases uses the less accurate version which should be perfectly fine.

marc2k3 commented 3 years ago

This is buggy if the text contains ampersands. You need to combine DT_NOPREFIX with DT_CALCRECT when using DrawText.

kbuffington commented 3 years ago

D'oh. Should have caught that. Also added a check to skip this block if text.length is 1 or less. Kerning pairs don't matter if you can't have a kerning pair.

TheQwertiest commented 3 years ago

@kbuffington thanks for your contribution! There are a few things that need to be changed before it can be merged though:

TheQwertiest commented 3 years ago

@kbuffington can you fix (and test) the PR, plz? So that I could include it in the release (which I'm planning to ship soon).

kbuffington commented 3 years ago

Yeah, I'll try to make some time for this. I'm not completely sure what you meant by: "DT_SINGLELINE should be added to DrawText call." but I haven't looked at the code in a month so maybe it'll make more sense when I do.

TheQwertiest commented 3 years ago

DT_SINGLELINE single line causes method to ignore new lines (i.e. \n), this way it would work the same as GetTextExtentPoint32

kbuffington commented 3 years ago

After looking at the code I understand what you were referencing and will add that.

kbuffington commented 3 years ago

Believe I've addressed everything, retested with new optional parameters and it all seems good.

TheQwertiest commented 3 years ago

Awesome, thanks!