Fixes #2333. TextField is selecting badly a word on double click.

[BDisp]

Fixes #2333 - We shouldn't ever use indexing with the Text property to compare chars, because they are indexed as byte instead of the representation of the displayed character.

Pull Request checklist:

• I've named my PR in the form of "Fixes #issue. Terse description."
• My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• My code follows the Terminal.Gui library design guidelines
• I ran dotnet test before commit
• I have made corresponding changes to the API documentation (using /// style comments)
• My changes generate no new warnings
• I have checked my code and corrected any poor grammar or misspellings
• I conducted basic QA to assure all features are working

[tznind]

Nice find, good catch!

[tig]

We shouldn't ever use indexing with the Text property to compare chars, because they are indexed as byte instead of the representation of the displayed character.

I'm confused by the phrasing here. Text is a ustring and thus is NOT byte based, right?

Regardless, I think it would help make the TextField code more clear if we renamed the TextField text member to runes, e.g.:

Old:

	public class TextField : View {
		List<Rune> text;

New:

	public class TextField : View {
		List<Rune> runes;

Also, note I found this by searching for Text [:

		List<Rune> DeleteSelectedText ()
		{
			ustring actualText = Text;
			SetSelectedStartSelectedLength ();
			int selStart = SelectedStart > -1 ? start : point;
			(var _, var len) = TextModel.DisplaySize (text, 0, selStart, false);
			(var _, var len2) = TextModel.DisplaySize (text, selStart, selStart + length, false);
			(var _, var len3) = TextModel.DisplaySize (text, selStart + length, actualText.RuneCount, false);
			var newText = actualText [0, len] +
				actualText [len + len2, len + len2 + len3];
			ClearAllSelection ();
			point = selStart >= newText.RuneCount ? newText.RuneCount : selStart;
			return newText.ToRuneList ();
		}

Is this code incorrect too? I don't think so, since it's counting Runes, but figured i'd ask.

FWIW, I searched the entire project for (char)Text [ and found only the bad code you fixed:

[BDisp]

We shouldn't ever use indexing with the Text property to compare chars, because they are indexed as byte instead of the representation of the displayed character.

I'm confused by the phrasing here. Text is a ustring and thus is NOT byte based, right?

I said they are indexed as byte and it's true, see bellow. ustring is based on UTF8 encoding which handles byte arrays. With regular chars normally the are the same but if there are any combining unicode the byte array changes.
https://github.com/gui-cs/NStack/blob/92b5d7f56bad11a9ce637797d0c8c4a14d6f50c5/NStack/strings/ustring.cs#L892
https://github.com/gui-cs/NStack/blob/92b5d7f56bad11a9ce637797d0c8c4a14d6f50c5/NStack/strings/ustring.cs#L168-L174
https://github.com/gui-cs/NStack/blob/92b5d7f56bad11a9ce637797d0c8c4a14d6f50c5/NStack/strings/ustring.cs#L286-L290
https://github.com/gui-cs/NStack/blob/92b5d7f56bad11a9ce637797d0c8c4a14d6f50c5/NStack/strings/ustring.cs#L349-L355

Regardless, I think it would help make the TextField code more clear if we renamed the TextField text member to runes, e.g.:

Old:

	public class TextField : View {
		List<Rune> text;

New:

	public class TextField : View {
		List<Rune> runes;

I fully agree.

Also, note I found this by searching for Text [:

		List<Rune> DeleteSelectedText ()
		{
			ustring actualText = Text;
			SetSelectedStartSelectedLength ();
			int selStart = SelectedStart > -1 ? start : point;
			(var _, var len) = TextModel.DisplaySize (text, 0, selStart, false);
			(var _, var len2) = TextModel.DisplaySize (text, selStart, selStart + length, false);
			(var _, var len3) = TextModel.DisplaySize (text, selStart + length, actualText.RuneCount, false);
			var newText = actualText [0, len] +
				actualText [len + len2, len + len2 + len3];
			ClearAllSelection ();
			point = selStart >= newText.RuneCount ? newText.RuneCount : selStart;
			return newText.ToRuneList ();
		}

Is this code incorrect too? I don't think so, since it's counting Runes, but figured i'd ask.

Yes it's correct because the TextModel.DisplaySize use the Rune.RuneLen method which returns the length of bytes occupied by a rune.

FWIW, I searched the entire project for (char)Text [ and found only the bad code you fixed:

Before I submitted this PR I already also had searched and found only the one I fixed, but it's not accurate that may haven't this bug on another place.
I think we only need the NStack because of the Rune.ColumnWidth and I maybe we can use the System.Text.Rune and make the own Rune.ColumnWidth method on the Core folder and change ustring to use string. The ColumnWidth may not be equal for all fonts type, some of them may have a 1 length and other 2. Thus, we can get rid of the NStack, but it's a lot of work. Is it worth for the v2?

[tig]

I think we only need the NStack because of the Rune.ColumnWidth and I maybe we can use the System.Text.Rune and make the own Rune.ColumnWidth method on the Core folder and change ustring to use string. The ColumnWidth may not be equal for all fonts type, some of them may have a 1 length and other 2. Thus, we can get rid of the NStack, but it's a lot of work. Is it worth for the v2?

Nice work @BDisp.

See #92 ;-)