insert_str should accept newlines

[pm100]

       debug_assert!(
            !line.contains('\n'),
            "string given to insert_str must not contain newline: {:?}",
            line,
        );

it should be looking at the input string not the line that is being inserted into

[rhysd]

Yes, it is actually just a limitation due to current implementation. Allowing newlines would be better. If fixing this, I think current insert_str should be renamed insert_piece as private method and kept since inserting a piece of string to text buffer is one of primitive operation of text editor and used everywhere internally. Then we can add insert_str as a new method.

[pm100]

With the selection pr I support multi line insert. But I thought I would point this out because what happens if you pass a multi line string to insert_str today it accepts it, removes the new lines and the inserts the rest into the specified line. Maybe it should be an assert not a debug assert

[rhysd]

I see. I understood your intention. I think EditKind::Insert should support multiple lines of strings, right? I think I can do it separately.

[rhysd]

I think making EditKind::Remove support multiple lines would also be useful for your selection support patch. I'll try this either.

[pm100]

I will add it.

…

________________________________ From: Linda_pp ***@***.***> Sent: Tuesday, October 31, 2023 6:41:31 PM To: rhysd/tui-textarea ***@***.***> Cc: pm100 ***@***.***>; Author ***@***.***> Subject: Re: [rhysd/tui-textarea] the assert on insert_str is wrong (Issue #42) I think making EditKind::Remove support multiple lines would also be useful for your selection support patch. I'll try this either. — Reply to this email directly, view it on GitHub<#42 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AADXOD7ENUAVTAT7LWJA6LLYCGSEXAVCNFSM6AAAAAA6W23VP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBYGI3DOOJVGA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[pm100]

My point was that the assert is a good idea in the current code but it tests the wrong field. Try it, do an insert_str of "a\nb"

[rhysd]

I want to minimize diff of each change as much as possible. So I prefer to supporting multi-line insert/remove as separate work.

[pm100]

That's kind of impossible. If I select multiple rows and press delete it gets deleted, if I press undo it gets reinserted, so I need (and have) all the plumbing to allow multiline insert and delete already written. I mean I can keep the user visible API insert_str to only one line, but that means simply crippling it by adding an assert From: Linda_pp ***@***.***> Sent: Tuesday, October 31, 2023 7:46 PM To: rhysd/tui-textarea ***@***.***> Cc: pm100 ***@***.***>; Author ***@***.***> Subject: Re: [rhysd/tui-textarea] the assert on insert_str is wrong (Issue #42) I want to minimize diff of each change as much as possible. So I prefer to supporting multi-line insert/remove as separate work. - Reply to this email directly, view it on GitHub<#42 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AADXOD7K2WE6PEQG5FIVE4DYCGZW7AVCNFSM6AAAAAA6W23VP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBYGMYTMNZWGM>. You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>>

[rhysd]

I press undo it gets reinserted, so I need (and have) all the plumbing to allow multiline insert and delete already written.

So you're happy if EditKind has some new variant which supports multiple lines like EditKind::ReplaceRange { start: (usize, usize), end: (usize, usize), content: Vec<String> }, am I correct? Then we can use the new variant to support inserting multiple lines by insert_str and you will be able to use it for selection API.

Actually I have thought to add this variant previously so I'm happy to implement it by myself.

[rhysd]

NOTE: I know that the issue you reported is just a small mistake in assertion condition, but I'd like to expand the discussion whether textarea should support editing multiple lines of chunks at once.

I know you're working on bigger feature (I really appreciate it). If my idea (adding the new variant) can help your work, and can make your implementation smaller, I'd like to do it first.

[rhysd]

I'm implementing EditKind::InsertChunk and EditKind::DeleteChunk in this branch: https://github.com/rhysd/tui-textarea/compare/multiline?expand=1

• Cutting selected text can be implemented with DeleteChunk
• Pasting multi-line text can be implemented with InsertChunk

[pm100]

I wish I has never said anything. I already have multiline insert delete, yank, paste, undo, redo implemented as part of the select PR.
It 80% of the code change. I wasnt asking for insert_str to support multi line merely pointing out that the assert was wrong

I will redo the select PR once you have done your changes - sigh :-(

BTW I just made EditKind::Insert and Remove support blocks rather that spans on one line. One line is just a special case of the general insert or remove a chunk of text. Done by allowed \ns to be embedded in the yank and history buffers

like here for each deleted line

        if allremoved.is_empty() {
            allremoved = removed;
        } else {
            allremoved = format!("{}\n{}", allremoved, removed);
        }

at the end allremoved to placed into history and yank. This make ^y easy as input_str supports multi-line paste now. Same for redo

[rhysd]

Hmm, such large PR is hard to merge honestly. I'd like to do things more incrementally. I'm sorry for that.

Generally, it is a good idea to talk with a maintainer (me in this case) before adding such large changes. Then we can agree with how the changes will happen.

[pm100]

I can submit the multi line bit first. Then the select as a separate pr. I promise to take out the trace and move the input handling back. Most of the code change is in util.rs. I moved the line array management code there because it has to be called from textarea and history. So the change is very isolated

[pm100]

Generally, it is a good idea to talk with a maintainer (me in this case) before adding such large changes. Then we can agree with how the changes will happen.

Basically thats what the first PR was meant to be. I didnt expect you to accept it, I was after feedback. the feeddback was

• dont leave trace
• dont move 'input'
• break into smaller changes

[pm100]

@rhysd - can I post a modified version of select using this new code

[rhysd]

@pm100 Sure. I definitely review your code if PR is submitted. And I apologize for your rework. This change (multi-lines support internally) was what I was thinking for a long time. So I'd like to implement by myself.

[rhysd]

Cut

TextKind::DeleteChunk(c, r, i) is available. r is the row of start position. i is the byte offset of start position. The following code can apply the edit.

let lines: Vec<String> = ...; // Clone selected text
let (row, col) = ...; // Get left top of selection
let i = lines[row].chars().nth(col).map(|(i, _)| i).unwrap_or(lines[row].len());
let edit = TextKind::DeleteChunk(lines, row, i);
edit.apply(row, &mut self.lines);

Paste

This is already implemented. (TextArea::paste).

Highlighting selection

All your code should be able to be reused.

[pm100]

I am on it. I already worked out what to do.

[pm100]

@rhysd In your mind does self.cursor represent the screen location (ignoring viewport position) or is it offset in lines array. In hard tab mode these two things are different. The doc comments do not say. I know what happens at the moment , but sometimes the intent and the code are different (aka 'a bug'). It makes a difference for me when calculating what to copy, etc. It would be nice not to implement things one way and then have to change it.

(BTW - this tripped me up in my first go at the select PR)

[rhysd]

self.cursor represent the screen location (ignoring viewport position) or is it offset in lines array

It's always number of characters. It's not byte offset. For example, when the text is "🐶🐱" and the cursor is at next to the dog emoji, then the cursor position is (0, 1) and the byte offset is 4 (since emoji text occupies 4 bytes). I strongly recommend to check your implementation with some multi-byte characters inputs before submitting a PR.

[pm100]

@rhysd I know all that about byte vs char. That not the question.. switch on hard tabs, enter a\tb in minimal. Is self.cursor now 0,3 or 0,6? (Tab_len=4)

[rhysd]

@pm100 3

[rhysd]

Hard tab character is always counted as 1. Display width is not related.

[pm100]

@rhysd so it's not screen position (0,6) then, it's offset in line array (0,3) . That needs to be explained in the docs of things like self.cursor() self.delete_str.... this is an important distinction. Could also provide a helper function to convert to screen position

[rhysd]

Yeah, I don't think current document is perfect. It needs to be polished.

[pm100]

It means that things with the same column value on different rows don't necessarily line up.

[rhysd]

Yes. For example あ occupies 2 characters width but it is counted as 1 in column. So thinking about "ab\nあc", 'b' is not aligned with 'c'.

[pm100]

Well TIL :-)

Is there a way to find out the width of chars on the screen, does crossterm know? I think it will be useful to have a screen cooridinate to logical cursor location conversion fn (Imagine wanting full mouse support, need to convert screen coordinates to char location)