New View.SetCursor() - problem/question

[dankox]

I found this problem when I was trying to update my application to use gocui version v1.0.0-beta-2.

I had there code like this (where v is referencing gocui.View):

v.Clear()
v.SetCursor(0, 0)

The View which is there, is 1 line in height. It's basically a command prompt and after hitting enter, the prompt will be cleared and cursor set to the beginning.

I know that I don't have error checks :) but the point is, that after the upgrade the function SetCursor was changed like this: https://github.com/awesome-gocui/gocui/blob/a0f5addf3ea3aac5a8444a76e2b7c87e4f229ce2/view.go#L239

Now after View.Clear (which sets v.lines = nil) I can't really run View.SetCursors(0, 0). Easy fix for me in this situation is just to move SetCursor above Clear, but I'm just wondering if this shouldn't be fixed in gocui too.

@mjarkk I think you did these changes regarding the line buffer. So I would like to hear your opinion. Before the check was like this:

if x < 0 || x >= maxX || y < 0 || y >= maxY {

Which makes it work, because the maxY is 1 in my case. And it kinda makes sense, because you could point anywhere into the view this way.

So I'm curious about the reason to limit the SetCursor this way, as it appears that before it was possible to set it in the range of the View dimension while after the change the limitation is to the lines in the line buffer.

[cmetz]

I ran into the same issue, i'm currently fixing /optimizing other stuff as well, also it potentially ran into a len(v.lines[y]) failing on a nil slice.

I think they changed it to implement a logic where it is only possible to set the cursor to a position where a line is existing and allowing to set x outside of maxX when there is content in the line , or outside of the line content, when its below maxX. a little wired logic..

But in my opinion a .Clear() should also reset the cursor position to 0, so there shouldn't be the need for setting it extra.

I'm new to Go, so i'm not sure if chaning it from:

 if x < 0 || y < 0 || y >= len(v.lines) || (len(v.lines[y]) >= x && x >= maxX) { 

to somehting like this would fix it (always let 0 values pass)

 if (x != 0 && y != 0) && (x < 0 || y < 0 || y >= len(v.lines) || (len(v.lines[y]) >= x && x >= maxX)) { 

[mjarkk]

Seems like setting the v.lines = nil is wrong as we no where in the code check for != nil but that doesn't fix the full issue.

@mjarkk I think you did these changes regarding the line buffer. So I would like to hear your opinion. Before the check was like this:

I've probably made those change :sweat_smile:, I think we cannot go back to the old code here as it works fundamentally different. In the past the cursor position was based on the visible lines and now it's based on the buffer lines.
If we would check for the view size the cursor wouldn't be able to go lower than the view size.

But in my opinion a .Clear() should also reset the cursor position to 0, so there shouldn't be the need for setting it extra.

We could do i'm ok with either tough as we might introduce some unexpected behaviour into code bases i would suggest keeping it like it as is currently.

to somehting like this would fix it (always let 0 values pass)

I'm not sure if we should exactly do that but your point is right i think we indeed should allow setting the cursor position to 0,0 on empty buffers as it's a common action after .Clear()

[dankox]

I was looking thru it to understand the problem a bit more and here is what I learned (feel free to correct me).
Cursor position in view is used mostly for editing purpose. Basically all the edit.go functions uses it for write, delete, newline, moving cursor up and down, etc.
This makes it important only when editing is turned on, which means the view.lines should be the way to check where it can be done.

For this I would suggest solution in a way, to basically check the cursor, and if it's not in the v.lines range, then check if it is in the view range and if it is, put it into the "closest" v.lines position. In short, if you try to set it to (6,3) but there is only 4 lines, then it would create a new line and put it in the beginning (5,1) (maybe not even create, just put it there, writeRune would create that line afterwards).
This way it would resolve the problem with SetCursor(0,0) and also other sets which would go out of range but still be in the view range.

However, there is still one problem as expected :) and that is mouse position. Mouse set cursor position to view with relative position in the window. Not taking into account lines or anything.
So I'm not really sure how to proceed here. Will check it later and try to create some edge cases, to see what problems might arise.

[mjarkk]

then check if it is in the view range and if it is, put it into the "closest" v.lines position

Why check if it's inside the view range?
I think having these 2 rules for setting the cursor position is very confusing and not clear, maybe a better solution here is to allow settings the cursor everywhere without an error but when it's outside of the buffer lines change it to the last buffered char? that's very easy to understand.

[dankox]

Hmmm... so you mean anywhere in limit of View? Or even outside of the dimension and it will be in the buffer?
Maybe just clarifying what (x, y) in SetCursor means (is it position in the View or buffer)? As for view.cx and view.cy those are position in the view buffer if I get it correctly?

[mjarkk]

Maybe just clarifying what (x, y) in SetCursor means (is it position in the View or buffer)

It's relative to the buffer not to the view.
This also means you could place the cursor outside of the view if the buffered text is larger than the view (now that i'm thinking about it we should maybe also scroll to the cursor position).

I made this change because i don't know of any situation where you would have scrollable text and want to place the cursor relative to the view and not relative to the buffer.
Also internally it's easier to base everything on the buffer and not to the viewport of the user, i presume jroimartin originally made it relative to the view so the library is less memory heavy but at the cost of more bugs and more complicated code.

[cmetz]

I think setting it on the buffer makes more sense, but it should work when the buffer is nil.

I also discovered an offset bug: When clearing a buffer the view offsets are not getting reset to 0 which leads to a missing first character in an single line input prompt which got scrolled recently. I also set v.ox and v.oy to 0 on a v.Clear() in my current patched version.

[mjarkk]

Hmm you're right maybe we should reset everything in the clear function.
I've created #78 that should fix the problem

[dankox]

I've checked it. I think that should "clear" most of the problem with SetCursor up :) Thank you.
I still think there might be a bit of problem with mouse and cursor highlights... however, I would have to come up with a "test case" where we could check it.
So fo now I guess this is fine and can be closed when the PR is merged.

[mjarkk]

I've merged the PR to fix the issue and created a new release v1.0.0-beta-3