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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
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)
{
/* 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
32 changes: 32 additions & 0 deletions vimode/src/utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ gint get_line_number_rel(ScintillaObject *sci, gint shift)
return new_line;
}


void goto_nonempty(ScintillaObject *sci, gint line, gboolean scroll)
{
gint line_end_pos = SSM(sci, SCI_GETLINEENDPOSITION, line, 0);
Expand All @@ -219,3 +220,34 @@ void goto_nonempty(ScintillaObject *sci, gint line, gboolean scroll)
pos = NEXT(sci, pos);
SET_POS(sci, pos, scroll);
}


void ensure_current_line_expanded(ScintillaObject *sci)
{
gint line = GET_CUR_LINE(sci);
if (!SSM(sci, SCI_GETLINEVISIBLE, line, 0))
SSM(sci, SCI_ENSUREVISIBLE, line, 0);
}


gint jump_to_expended_parent(ScintillaObject *sci, gint line)
{
gint fold_parent = line;

/* go through the parents as long as they are not visible */
while (SSM(sci, SCI_GETLINEVISIBLE, fold_parent, 0) == FALSE) {
Copy link
Member

Choose a reason for hiding this comment

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

Better !SSM(...) instead of the FALSE comparison.

Also place { on separate line to match the other code style.

gint prev_parent = SSM(sci, SCI_GETFOLDPARENT, fold_parent, 0);

if (prev_parent == -1) break;
Copy link
Member

Choose a reason for hiding this comment

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

break on separate line.

fold_parent = prev_parent;
}

if (fold_parent != line)
{
/* move the cursor on the visible line before the fold */
gint pos = SSM(sci, SCI_POSITIONFROMLINE, fold_parent, 0);
SET_POS(sci, pos, TRUE);
Copy link
Member

Choose a reason for hiding this comment

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

Use SET_POS_NOX() instead. This one will preserve the "maximum x" cursor value so when further moving up or down, this one will be recovered.

}

return fold_parent;
}
3 changes: 3 additions & 0 deletions vimode/src/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,8 @@ void perform_substitute(ScintillaObject *sci, const gchar *cmd, gint from, gint
const gchar *flag_override);

gint get_line_number_rel(ScintillaObject *sci, gint shift);
void ensure_current_line_expanded(ScintillaObject *sci);
Copy link
Member

Choose a reason for hiding this comment

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

Again, remove, it's from another patch.


gint jump_to_expended_parent(ScintillaObject *sci, gint cur_line);

#endif
8 changes: 8 additions & 0 deletions vimode/src/vi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,14 @@ gboolean vi_notify_sci(SCNotification *nt)
}
}

if (nt->nmhdr.code == SCN_MARGINCLICK) {
Copy link
Member

Choose a reason for hiding this comment

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

{ on separate line.

Copy link
Member

Choose a reason for hiding this comment

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

I've just merged #1338 which functionality-wise covers most of this PR but doesn't contain the SCN_MARGINCLICK part. Would you create a separate pull request containing it?

if (nt->margin == 2)
{
gint line = GET_CUR_LINE(sci);
jump_to_expended_parent(sci, line);
}
}

/* This makes sure that when we click behind the end of line in command mode,
* the cursor is not placed BEHIND the last character but ON the last character.
* We want to ignore this when doing selection with mouse as it breaks things. */
Expand Down