Cursor Fix

[kirawi]

Fixes #58, #61, #73, & #74

[kirawi]

I'm going to look at some easy issues to fix, marking as a draft for now.

[kirawi]

Found a bug in my code, fixing.

[archseer]

Looks like you also need to run cargo fmt :)

[archseer]

I think one of the bounds also needs to be adjusted, if you open a larger file there will be a line number in the status line.

[kirawi]

I found various regressions to the shortcuts as a result of the cursor change, I'll be working on it tomorrow.

[kirawi]

One issue I'm having is r + r on an empty buffer, I'm going to take a look at that tomorrow. Sadly, I'm a monkey with terminal editors so I'm smashing random keys and hoping it doesn't panic... It's likely related to the change to the cursor allowing it to go past the final character in a line.

[archseer]

allowing it to go past the final character in a line.

So this does work already on master, you should be able to go past the last char (technically onto the invisible \n) but you shouldn't be able to go past the final line.

[kirawi]

allowing it to go past the final character in a line.

So this does work already on master, you should be able to go past the last char (technically onto the invisible \n) but you shouldn't be able to go past the final line.

That's interesting... I'll undo those changes then.

[archseer]

The rest is quite an improvement though :) The off by one line number rendering fix explains why a single line without a trailing \n would have no line number

[kirawi]

Ah, I see. Okay, so what's happening is that on the current master branch, it considers \n a separate character which allows it to seem like that, while mine doesn't do that. However, I am still running into that annoying bug even after removing all the cursor changes, so I'll look at it tomorrow. I hope you don't mind if I put in my previous changes to the cursor, as it would properly allow it to go beyond the final character of the line? Currently, if you remove that \n with the d or r hotkeys for example, you can't do that.

[kirawi]

Amusingly, the fix for the bug was the same as the fix for _delete(), somehow I missed that. Basically, it needs this to be correct:

let transaction =
                Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| {
                    let max_to = doc.text().len_chars(); // This
                    let to = std::cmp::min(max_to, range.to() + 1); // And this
                    (range.from(), to, Some(text.clone()))
                });

[kirawi]

I think it works, but I'm not sure if there are remaining bugs.

edit:

• It doesn't correctly copy + paste
• It can't deal with crlf

These are bugs that are also in the master branch, but it might be worth getting this pushed to fix the current panics and then let someone else fix that in a subsequent PR.

[archseer]

You also want to test that len_after is correct

[archseer]

It's looking good, I'll do some testing today and then merge.

[archseer]

A few issues:

• iasd<Esc>b panics
• w is unable to go past end of line anymore (positioning either on the last char or past last char)
• pressing X repeatedly crashes (even in an empty doc)
• A doesn't append at end of line, but one character before
• iasd asd<Esc>S (so "asd asd", split on space) no longer selects the last character on the line

Are you testing this on windows? I think that might explain some of the differences. I don't think we can rely on text.line_to_char(line + 1) - 1 to get the end of line anymore since it depends on the line ending (CR = 1 vs CRLF = 2)

[kirawi]

Agreed. Additionally, I'm wondering if there's a better solution to the cursor thing considering how many bugs are popping up as a result of it, and I can only imagine the nightmare that would happen if someone else had to change something and everything breaks. Since everything seems to be because of 1 off & bounding errors, I'll be looking into a better way to do that. Maybe a field or inline function that determines the "real" current character.

[archseer]

I agree, there should be some sort of safe wrapper to clamp the range. We already achieve that on one end with saturating_sub, but we need something for the upper bound.

Might be worth splitting some of the fixes out and getting them merged early, specifically the division by zero bug and possibly the ..= fix for line numbers, and the len_after fix.

Also, feel free to join us at https://matrix.to/#/#helix-community:matrix.org to discuss (make sure to join #helix-editor:matrix.org if you're on a client that doesn't support Matrix Spaces yet)

[kirawi]

Unnecessary after 14830e7, though be aware that let height = self.area.height.saturating_sub(2); // - 2 for statusline was not undone in that commit which is leading to the missing line at the bottom of the viewport.