Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vimode: Fix cursor hang with folded lines #1326

Closed
wants to merge 4 commits into from
Closed

Vimode: Fix cursor hang with folded lines #1326

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5709047
Fix cursor hang when we want to move cursor on top line and this line…
scresto09 Apr 15, 2024
be15445
Merge branch 'geany:master' into vimode-fix-fold-up
scresto09 Apr 15, 2024
5608b74
Automatically set the cursor position on the visible line
scresto09 May 7, 2024
274ff97
try to fix some issues of the plugin with folded lines
scresto09 May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 64 additions & 41 deletions vimode/src/cmds/motion.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,42 +40,53 @@ void cmd_goto_right(CmdContext *c, CmdParams *p)
SET_POS(p->sci, pos, TRUE);
}

static gint doc_line_from_visible_delta(CmdParams *p, gint line, gint delta, gint *previous)
{
gint step = delta < 0 ? -1 : 1;
gint new_line = p->line;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be line instead of p->line - we don't always pass p->line as the parameter of this function.

gint left = abs(delta);
gint prev_line = -1, tmp;

if ((step > 0 && new_line < p->line_num - 1) || (step < 0 && new_line > 0)) {
while (left > 0) {
tmp = new_line;
if (SSM(p->sci, SCI_GETLINEVISIBLE, new_line, 0)) {
left--;
prev_line = tmp;
}
new_line += step;
if (new_line >= p->line_num || new_line <= 0) break;
}
}

if (previous) *previous = prev_line;

return new_line;
}

void cmd_goto_up(CmdContext *c, CmdParams *p)
{
gint one_above, pos;
gint previous = -1;
gint new_line;
gint wrap_count;

if (p->line == 0)
return;

new_line = doc_line_from_visible_delta(p, p->line, -p->num, &previous);

/* Calling SCI_LINEUP/SCI_LINEDOWN in a loop for num lines leads to visible
* slow scrolling. On the other hand, SCI_LINEUP preserves the value of
* SCI_CHOOSECARETX which we cannot read directly from Scintilla and which
* we want to keep - perform jump to previous/following line and add
* one final SCI_LINEUP/SCI_LINEDOWN which recovers SCI_CHOOSECARETX for us. */
one_above = p->line - p->num - 1;
if (one_above >= 0 && SSM(p->sci, SCI_GETLINEVISIBLE, one_above, 0))
{
/* Every case except for the first line - go one line above and perform
* SCI_LINEDOWN. This ensures that even with wrapping on, we get the
* caret on the first line of the wrapped line */
pos = SSM(p->sci, SCI_GETLINEENDPOSITION, one_above, 0);

if (previous > -1) {
guint pos = SSM(p->sci, SCI_POSITIONFROMLINE, previous, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you didn't understand the purpose of this strange "one above" and then SCI_LINEDOWN dance. While you get to the correct line alright, you lose the cursor's x position on the line this way. Notice how, when moving cursor up and down, it remembers the maximum x coordinate on the line and even after bing on lines where the maximum x is lower *like e.g. an empty line where x is 0), it returns back to the right position on longer lines.

Unfortunately this "maximum x" is not possible to obtain from Scintilla but Scintilla automatically recovers it when doing SCI_LINEDOWN or SCI_LINEUP. So the trick here is to first go to the line above or below, and then perform SCI_LINEDOWN or SCI_LINEUP to get us to the right line and get the x position of the cursor. When you remove this code, you'll always end with the x position at the beginning of the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think I understood the problem you explained correctly.

But from what I understood, for cmd_goto_up this method is only really necessary when the line you want to go to is not the previous visible line.

With this fix my idea was to have "doc_line_from_visible_delta" fill the "previous" variable with the line number just below the visible line to access it with a SET_POS_NOX, then in any case do a SCI_LINEUP to go to the desired line.

Same for goto_down and SCI_LINEDOWN.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so it was me who misunderstood your code :-).

Even though your version works, I'll probably go for #1338 which is easier for me to understand - unless you have some problems with it.

SET_POS_NOX(p->sci, pos, FALSE);
SSM(p->sci, SCI_LINEDOWN, 0, 0);
}
else
{
/* This is the first line and there is no line above - we need to go to
* the following line and do SCI_LINEUP. In addition, when wrapping is
* on, we need to repeat SCI_LINEUP to get to the first line of wrapping.
* This may lead to visible slow scrolling which is why there's the
* fast case above for anything else but the first line. */
gint one_below = p->line - p->num + 1;
gint wrap_count;

one_below = one_below > 0 ? one_below : 1;
pos = SSM(p->sci, SCI_POSITIONFROMLINE, one_below, 0);
SET_POS_NOX(p->sci, pos, FALSE);

if (new_line < p->line) {
SSM(p->sci, SCI_LINEUP, 0, 0);

wrap_count = SSM(p->sci, SCI_WRAPCOUNT, GET_CUR_LINE(p->sci), 0);
Expand All @@ -97,18 +108,20 @@ void cmd_goto_up_nonempty(CmdContext *c, CmdParams *p)

static void goto_down(CmdParams *p, gint num)
{
gint one_above, pos;
gint last_line = p->line_num - 1;
gint previous = -1;
gint new_line;

if (p->line == last_line)
if (p->line >= p->line_num - 1)
return;

/* see cmd_goto_up() for explanation */
one_above = p->line + num - 1;
one_above = one_above < last_line ? one_above : last_line - 1;
pos = SSM(p->sci, SCI_GETLINEENDPOSITION, one_above, 0);
SET_POS_NOX(p->sci, pos, FALSE);
SSM(p->sci, SCI_LINEDOWN, 0, 0);
new_line = doc_line_from_visible_delta(p, p->line, num, &previous);

if (previous > -1) {
guint pos = SSM(p->sci, SCI_GETLINEENDPOSITION, previous, 0);
SET_POS_NOX(p->sci, pos, FALSE);
}

if (new_line > p->line) SSM(p->sci, SCI_LINEDOWN, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all this code necessary? Maybe I'm missing something but I did just this:

fa7025b#diff-15a3a15a958bc6f85e0f1fae419e23c60bbee47bf617ef133f7539fc1f29b277R128

doc_line_from_visible_delta() only returns a line from the valid line range and when you are on the first line, this is already the "line above" for which you then perform SCI_LINEDOWN so it should be OK on this side. On the other end of the document when you are on the last line, SCI_LINEDOWN just won't do anything so there's no need for some special handling there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a detail but this code is useful to have the same behavior as VIM.
With VIM, when the cursor is on the last line but not the last character and you press the down arrow, the cursor does not move.
Without this code, the cursor will move to the end of the line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, yeah, the previous code handled right that, I just forgot why exactly it was there. I've fixed it in my PR.

}


Expand Down Expand Up @@ -136,39 +149,40 @@ void cmd_goto_down_one_less_nonempty(CmdContext *c, CmdParams *p)
void cmd_goto_page_up(CmdContext *c, CmdParams *p)
{
gint shift = p->line_visible_num * p->num;
gint new_line = get_line_number_rel(p->sci, -shift);
gint new_line = doc_line_from_visible_delta(p, p->line, -shift, NULL);
goto_nonempty(p->sci, new_line, TRUE);
}


void cmd_goto_page_down(CmdContext *c, CmdParams *p)
{
gint shift = p->line_visible_num * p->num;
gint new_line = get_line_number_rel(p->sci, shift);
gint new_line = doc_line_from_visible_delta(p, p->line, shift, NULL);
goto_nonempty(p->sci, new_line, TRUE);
}


void cmd_goto_halfpage_up(CmdContext *c, CmdParams *p)
{
gint shift = p->num_present ? p->num : p->line_visible_num / 2;
gint new_line = get_line_number_rel(p->sci, -shift);
gint new_line = doc_line_from_visible_delta(p, p->line, -shift, NULL);
goto_nonempty(p->sci, new_line, TRUE);
}


void cmd_goto_halfpage_down(CmdContext *c, CmdParams *p)
{
gint shift = p->num_present ? p->num : p->line_visible_num / 2;
gint new_line = get_line_number_rel(p->sci, shift);
gint new_line = doc_line_from_visible_delta(p, p->line, shift, NULL);
goto_nonempty(p->sci, new_line, TRUE);
}


void cmd_goto_line(CmdContext *c, CmdParams *p)
{
gint num = p->num > p->line_num ? p->line_num : p->num;
goto_nonempty(p->sci, num - 1, TRUE);
num = doc_line_from_visible_delta(p, num, -1, NULL);
goto_nonempty(p->sci, num, TRUE);
}


Expand All @@ -177,30 +191,39 @@ void cmd_goto_line_last(CmdContext *c, CmdParams *p)
gint num = p->num > p->line_num ? p->line_num : p->num;
if (!p->num_present)
num = p->line_num;
goto_nonempty(p->sci, num - 1, TRUE);
num = doc_line_from_visible_delta(p, num, -1, NULL);
goto_nonempty(p->sci, num, TRUE);
}


void cmd_goto_screen_top(CmdContext *c, CmdParams *p)
{
gint line;
gint top = p->line_visible_first;
gint count = p->line_visible_num;
gint line = top + p->num;
goto_nonempty(p->sci, line > top + count ? top + count : line, FALSE);
gint max = doc_line_from_visible_delta(p, top, count, NULL);
gint num = p->num;

if (!p->num_present)
num = 0;

line = doc_line_from_visible_delta(p, top, num, NULL);
goto_nonempty(p->sci, line > max ? max : line, FALSE);
}


void cmd_goto_screen_middle(CmdContext *c, CmdParams *p)
{
goto_nonempty(p->sci, p->line_visible_first + p->line_visible_num/2, FALSE);
gint num = doc_line_from_visible_delta(p, p->line_visible_first, p->line_visible_num / 2, NULL);
goto_nonempty(p->sci, num, FALSE);
}


void cmd_goto_screen_bottom(CmdContext *c, CmdParams *p)
{
gint top = p->line_visible_first;
gint count = p->line_visible_num;
gint line = top + count - p->num;
gint line = doc_line_from_visible_delta(p, top, count - p->num, NULL);
goto_nonempty(p->sci, line < top ? top : line, FALSE);
}

Expand Down