[CRITICAL] Fix clipboard text operations and UTF-8 cursor handling

[PavelSharp]

Edit: Improve structure
This pull request effectively repairs the entire clipboard handling system in Nuklear. To achieve this, it addresses three separate bugs:

• [CRITICAL BUG] nk_str_insert_text_char and nk_str_insert_str_char has wrong implementation #840
• The cursor position issue
• The clipboard copy bug

Note: see #840 for more details and a minimal reproducible example.

[PavelSharp]

Hi, @sleeptightAnsiC!
Thank you for your comment - I really appreciate the feedback. I carefully reviewed your notes and started digging into the issue. While doing so, I actually discovered two fantastic bugs in Nuklear that (hopefully!) explain all the clipboard and UTF-8 handling problems. Let me show you what I found:

1 Cursor position issue
The problem is caused by an incorrect cursor offset in line 27194 (nk_textedit_paste).
Obviously, we want to move the cursor by the number of codepoints, not bytes.
So instead of:

state->cursor += len;

it should be:

state->cursor += glyphs;

2 Clipboard copy bug - this one is mind-blowing
Nuklear calls clip->copy only once (line 28172 in nk_do_edit), like this:

edit->clip.copy(edit->clip.userdata, text, end - begin);

The backend must provide its own implementation of this function.
But guess what? It expects the length in bytes, not in codepoints (I double-checked nk_glfw3_clipboard_copy and nk_sdl_clipboard_copy). So that’s the root cause! Luckily, it’s easy to fix.
Here’s the corrected version:

text = nk_str_at_const(&edit->string, begin, &unicode, &glyph_len);
text2 = nk_str_at_const(&edit->string, end, &unicode, &glyph_len);

if (edit->clip.copy)
    edit->clip.copy(edit->clip.userdata, text, text2 - text);

After these changes, everything starts working properly, though this is still just a draft fix.
Would you like to give it a try? 😇
As an optional step towards full unicode support, it will be useful to take another look at #829.

[sleeptightAnsiC]

The issues that I mentioned in #841 (comment) does not occur after 3342ffc

But a possibility that you used LLM to fix those worries me a lot...

[sleeptightAnsiC]

Something is really odd here (and I'm not talking about LLM concerns anymore, although I think my concerns are valid)

Per your comment under #852 (comment), if utf8/clipboard handling is broken everywhere (in every demo), this really feels like we have a regression here. What we should do first, is not trying to fix it, but instead we should find out why the regression has happened, what caused it and so on... Without this we may break something else, cause another regression, or run into risk that this thing will break again.

@PavelSharp did you try to git-bisect this issue before submitting PR/Issue ?

EDIT: Okey, fine... I think that the next Pavel's comment explains everything. Although, I still think this whole situation is a bit strange.

[PavelSharp]

@sleeptightAnsiC
So, let’s talk specifically about the clipboard copy bug. The root cause lies in these lines:

Nuklear/src/nuklear_edit.c

Lines 284 to 291 in fcd64f8

	int b = edit->select_start;
	int e = edit->select_end;
	int begin = NK_MIN(b, e);
	int end = NK_MAX(b, e);
	text = nk_str_at_const(&edit->string, begin, &unicode, &glyph_len);
	if (edit->clip.copy)
	edit->clip.copy(edit->clip.userdata, text, end - begin);

Here we clearly see that end - begin is passed as the len argument. Since b and e represent text selection positions in the edit buffer, end - begin gives us the number of Unicode codepoints selected.

However, as I said earlier, all backends treat len as the number of bytes, not the number of codepoints. And that mismatch is the core of this bug.

As for how old this issue is. It’s actually nearly 10 years old. I traced it back to the commit where version 1.0 was released. Before that point, Nuklear was still named zahnrad.

[PavelSharp]

@RobLoach I think this PR is finally ready to be merged!
All the requested changes have been addressed, and all merge conflicts have been fully resolved.
Also, as mentioned in our Reviewers Guide (“Have at least one other person review the changes before merging”), an additional review and testing has already been completed by @sleeptightAnsiC in his comment. Thanks.
With this, the clipboard behavior is now properly fixed across all existing backends

[RobLoach]

What in the ChatGPT 💀 ... I'll be happy to merge once we get the 👍

Any nit-picks can be fixed as follow ups. Thanks again, all, for all the hard work here.

[sleeptightAnsiC]

Given that this PR basically fixes something that was broken since forever, and I'm afraid a lot of people had to make some workarounds related to the bug, it might be safe to mark the fix as a "breaking change" and/or increase the major version (?)

I'm definitely NOT someone who should be reviewing this. A long-time contributor should take an action here and decide.

@rswinkle Would you mind taking a look? or maybe tag someone who you think is appropriate?

[PavelSharp]

@sleeptightAnsiC

I didn't test the latest changes yet. The old comment doesn't really apply here, it's outdated.

I'm sorry, but the only real code change since that comment is just this small tweak:
return nk_str_insert_at_rune(str, pos, text, len); to return nk_str_insert_at_rune(str, pos, text, len) ? len : 0;

Everything else was just rebasing and updating the branch.
Honestly, it felt like a tiny detail 🙂

[sleeptightAnsiC]

I'll try to test this soon.

the only real code change since that #841 (comment) is just this small tweak

Retested and everything seems fine.

Any nit-picks can be fixed as follow ups.

My only nitpick here is that it might be nice to have this change as one single commit (?) Those two commits don't really work correctly without each other, and the third commit is rebuilding the amalgamation, so it may confuse people. This way, if said change ever causes an issue for someone, it will be easier to find it with git-log or git-bisect. But this is just a minor inconvenience, and @RobLoach can easily solve it with "squash+merge", so no worries.

And my previous comment still applies #841 (comment)

[PavelSharp]

Just to note, there have been a couple of previous attempts to address this:

• nk_do_edit cut and copy handler passes len as grapheme cluster count to nk_plugin_copy not as count bytes in const char* #764, as @sleeptightAnsiC pointed in his comment
• Fix copied string corrupt when contain CJK glyphs #543, where the author describes the same problem but only fixes it for the GLFW backend.

This PR resolves the root cause. Personally, I believe it's obviously that the copy callback should take the number of bytes instead of glyphs.