Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 29, 2021

Conversation

kbuffington
Copy link
Contributor

@kbuffington kbuffington commented May 25, 2021

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
Copy link
Contributor Author

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

@kbuffington kbuffington changed the title Improve accuracy of get_text_width if selected font has kerning pairs Improve accuracy of CalcTextWidth if selected font has kerning pairs May 25, 2021
@marc2k3
Copy link
Contributor

marc2k3 commented May 26, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

marc2k3 commented May 27, 2021

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

@kbuffington
Copy link
Contributor Author

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
Copy link
Owner

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

  • This is a breaking change, hence it must be moved under a separate argument (e. g. use_exact or smth) and should not be enabled by default.
  • This new argument must be added to docs.
  • DT_SINGLELINE should be added to DrawText call.

@@ -111,11 +111,22 @@ size_t get_text_height( HDC hdc, std::wstring_view text )
return static_cast<size_t>( size.cy );
}

size_t get_text_width( HDC hdc, std::wstring_view text )
size_t get_text_width( HDC hdc, std::wstring_view text, bool accurate )
{
SIZE size;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Size is only used in the second branch of if, so it should be moved there

{
// TODO: add error checks
GetTextExtentPoint32( hdc, text.data(), static_cast<int>( text.size() ), &size );
}
return static_cast<size_t>( size.cx );
Copy link
Owner

@TheQwertiest TheQwertiest Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return should be moved into if branch

@TheQwertiest
Copy link
Owner

TheQwertiest commented Jun 28, 2021

@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
Copy link
Contributor Author

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
Copy link
Owner

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

@kbuffington
Copy link
Contributor Author

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

@kbuffington
Copy link
Contributor Author

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

@TheQwertiest
Copy link
Owner

Awesome, thanks!

@TheQwertiest TheQwertiest merged commit 2412e64 into TheQwertiest:master Jun 29, 2021
@TheQwertiest TheQwertiest added the enhancement New feature or request label Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants