Changed default Windows clipboard handler to expand LF to CRLF to fix clipboard corruption on some systems

[PathogenDavid]

✨ CRLF: Because Teletype machines are still relevant in 2021 ✨

Background

This PR fixes the clipboard corruption bug described in #4029

In short: For whatever reason, that user's clipboard gets corrupted when copying text containing non-CRLF newlines. Unfortunately, even after configuring my system to use Russian I was unable to reproduce the issue locally so we're not 100% sure what causes this.

Regardless though, the Windows documentation does state that the CF_UNICODETEXT clipboard format is expected to use CRLF newlines, so using CRLF newlines is the technically correct thing to do:

Unicode text format. Each line ends with a carriage return/linefeed (CR-LF) combination. A null character signals the end of the data.

As a random anecdotal sample: SDL's WIN_SetClipboardText function expands LF to CRLF as well.

Changes

The changes to the existing implementation are essentially:

1. Scan the input string and count LF newlines that are not a part of a CRLF newline.
2. Over-allocate the clipboard text buffer by the number of bare LF newlines to make room for the CRs to be added later.
3. After the UTF8 -> UTF16 widening, convert the bare LF newlines to CRLF.

The widening is implemented by iterating over the string backwards and copying it to the end of the buffer to avoid the need for allocating an extra buffer. It's also skipped if the copied text did not contain any newlines. (If that chubby for loop grosses you out too much, I can rewrite it to be simpler at the expense of double-allocating the clipboard buffer.)

Performance

Since this bug seems to be extremely niche I took some basic performance metrics to see how the changes affected things. Unsurprisingly, the performance impact is negligible for a function called so rarely (and usually with very little data.) The results on my machine are summarized below:

Sample Data	Current Implementation	This PR's Implementation
"Hello, world!"	0.073100 ms	0.491900 ms
imgui.cpp with CRLF line endings (566 KB)	1.428400 ms	8.495800 ms
imgui.cpp with LF line endings (555 KB)	1.161600 ms	11.585000 ms
The complete works of William Shakespeare (10.8 MB)	28.956500 ms	90.826100 ms

Since this isn't a proper microbenchmark, those results tend to be pretty noisy. Either way I don't think people are regularly copying the entirety of the complete works of William Shakespeare from a Dear ImGui app, so I don't think the performance regression will hurt anyone 😅

You can find these source of these quick-and-dirty benchmarks in my GH-4029 branch in the example_glfw_opengl3 project.

GLFW

GLFW's _glfwPlatformSetClipboardString implementation also has this issue.

As such, I've modified imgui_impl_glfw.cpp to use Dear ImGui's built-in clipboard handling when building for Windows. I did this mainly because our GLFW backend appears to prefer keeping backwards compatibility with older versions of GLFW and the GLFW clipboard implementation offers little value over our own.

(If I get around to submitting a PR to GLFW we could put it behind a version gate on Windows once it's merged and released.)

Allegro

Allegro's win_set_clipboard_text implementation also has this issue. The linked comment suggests they might be resistant to changing this.

As such, I disabled the use of Allegro's clipboard in imgui_impl_allegro5.cpp on Windows as well.

[ocornut]

Thank you David for the detailed investigation and PR.

I haven't dug in details, but some thoughts:

• Feel like the conversion might be better done in the higher-level function ImGui::SetClipboardText() ? which would ensure all backends on Win32 can benefit from it (even without a backend update). As a bonus it would also likely be a little simpler.

• As for lack of repro (can't repro either)... it's extremely puzzling that we can't repro. Note that while @LPVOIDDev states they have "No other apps that mess with clipboard" it is difficult to prove this and they are very often unwanted apps/hooks in Windows ecosystem. Either way, even if it is the fault of a third-party app, we should strive to work well in their ecosystem.

Note that if you dig in "classic" Windows settings dialog (generally stashed in options linked from the right-side of config panels) there are variety of less obvious settings, e.g.:

[PathogenDavid]

Feel like the conversion might be better done in the higher-level function ImGui::SetClipboardText() ? which would ensure all backends on Win32 can benefit from it (even without a backend update). As a bonus it would also likely be a little simpler.

Not a bad idea, I definitely zeroed in on fixing the lower level function. (I also probably got a little carried away on wanting to avoid extra heap allocations out of habit.)

The only real issue I see with doing it this way is that a "well behaved" backend would be trying to redundantly do the LF -> CRLF expansion, but I doubt it'd ever matter in the end.

I'll try to find some time to write up an alternative PR that works this way instead.

it is difficult to prove this and they are very often unwanted apps/hooks in Windows ecosystem. Either way, even if it is the fault of a third-party app, we should strive to work well in their ecosystem.

Yup, that was my line of thinking as well.

Note that if you dig in "classic" Windows settings dialog (generally stashed in options linked from the right-side of config panels) there are variety of less obvious settings, e.g.:

I believe that setting only affects apps using the -A APIs. (Or in the case of the clipboard API, apps populating CF_TEXT without CF_LOCALE.)

[PathogenDavid]

@ocornut Safe to assume that given #4029 (comment) we'll put this one to bed? Or do you want to consider it anyway to obey the specifics of the CF_UNICODETEXT spec?

[ocornut]

Safe to assume that given #4029 (comment) we'll put this one to bed? Or do you want to consider it anyway to obey the specifics of the CF_UNICODETEXT spec?

I think right now it is simpler and safer to not apply the change until we have evidence that it would actually be solving a problem (rather than working around a bug inside a malware!), else we'd be left with code whose purpose would be unclear. I'm confident this thread can be useful for future reference too. Nowadays even Notepad supports Linux-style carriage returns.

Sorry this took quite an investigation on your side!

[PathogenDavid]

Agreed. I was hesitant to even submit the PR without knowing exactly what caused it in the first place.

And no worries, it'll make a fun story in the future! I've been enjoying seeing all the attention it's gotten on Twitter.