[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
OK, I just create #1349 for SCN_MARGINCLICK part. Thanks -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany-plugins/pull/1326#issuecomment-2124751106 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
Closed #1326. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany-plugins/pull/1326#event-12885969899 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
Closing this PR as #1338 was merged. Having SCN_MARGINCLICK handled would however be worth adding as a separate PR. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany-plugins/pull/1326#issuecomment-2123515548 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@techee commented on this pull request.
> @@ -306,6 +306,14 @@ gboolean vi_notify_sci(SCNotification *nt)
}
}
+ if (nt->nmhdr.code == SCN_MARGINCLICK) {
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?
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1326#discussion_r1609001082
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@techee commented on this pull request.
> 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, );
+
+ 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);
Alright, yeah, the previous code handled right that, I just forgot why exactly
it was there. I've fixed it in my PR.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1326#discussion_r1605850563
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@techee commented on this pull request.
> /* 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);
OK, so it was me who misunderstood your code :-).
Even though your version works, I'll probably go for
https://github.com/geany/geany-plugins/pull/1338 which is easier for me to
understand - unless you have some problems with it.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1326#discussion_r1605826585
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@scresto09 commented on this pull request.
> 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, );
+
+ 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);
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1326#discussion_r1599776641
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@scresto09 commented on this pull request.
> /* 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);
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1326#discussion_r1598676127
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@techee requested changes on this pull request.
Looks good except for the minor comments below.
I was actually thinking about not doing this at all because it doesn't cover
all fold/unfold situations like e.g. using Geany's keybindings for folding but
the patch is simple and better than nothing so let's do it.
Would you post this as a separate pull request? If
https://github.com/geany/geany-plugins/pull/1338 looks good to you, I'd merge
that PR after which this one could be merged.
> +
+
+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) {
Better `!SSM(...)` instead of the `FALSE` comparison.
Also place `{` on separate line to match the other code style.
> +{
+ 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) {
+ gint prev_parent = SSM(sci, SCI_GETFOLDPARENT, fold_parent, 0);
+
+ if (prev_parent == -1) break;
`break` on separate 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) {
+ gint prev_parent = SSM(sci, SCI_GETFOLDPARENT, fold_parent, 0);
+
+ if (prev_parent == -1) break;
+ 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);
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.
> @@ -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);
Again, remove, it's from another patch.
> @@ -306,6 +306,14 @@ gboolean vi_notify_sci(SCNotification *nt)
}
}
+ if (nt->nmhdr.code == SCN_MARGINCLICK) {
`{` on separate line.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1326#pullrequestreview-2045634798
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@techee commented on this pull request.
> @@ -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;
Should be `line` instead of `p->line` - we don't always pass `p->line` as the
parameter of this function.
> /* 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);
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.
> 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, );
+
+ 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);
Is all this code necessary? Maybe I'm missing something but I did just this:
https://github.com/geany/geany-plugins/pull/1338/commits/fa7025ba9d58fb4680fb47e13bd5c05c6f1f1059#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.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1326#pullrequestreview-2045509286
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
> After read the https://sourceforge.net/p/scintilla/bugs/2438/ report, I think > the best way to automatically set the cursor position on the visible line is > surely to track the SCN_MARGINCLICK event. I did some modification to handle > this with the commit > [5608b74](https://github.com/geany/geany-plugins/pull/1326/commits/5608b74cba25b571428da9a055e696ddb053a247) > . Thanks for noticing the problem with line wrapping. I think your general approach is right - I've just slightly rewritten the `doc_line_from_visible_delta()` function to be easier to grasp by my poor brain, please check https://github.com/geany/geany-plugins/pull/1338/commits/fa7025ba9d58fb4680fb47e13bd5c05c6f1f1059 if it looks good to you. There are a few things in your patch which I don't think are completely correct though - I'll add some inline comments to your code. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany-plugins/pull/1326#issuecomment-2100454899 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@scresto09 pushed 1 commit. 274ff97f51e65b02ae86579768ec8d41f5b5c611 try to fix some issues of the plugin with folded lines -- View it on GitHub: https://github.com/geany/geany-plugins/pull/1326/files/5608b74cba25b571428da9a055e696ddb053a247..274ff97f51e65b02ae86579768ec8d41f5b5c611 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
In response to pull request [#1338](https://github.com/geany/geany-plugins/pull/1338) After read the [https://sourceforge.net/p/scintilla/bugs/2438/](https://sourceforge.net/p/scintilla/bugs/2438/) report, I think the best way to automatically set the cursor position on the visible line is surely to track the SCN_MARGINCLICK event. I did some modification to handle this with the commit [5608b74](https://github.com/geany/geany-plugins/pull/1326/commits/5608b74cba25b571428da9a055e696ddb053a247) . -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany-plugins/pull/1326#issuecomment-2098517956 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
@scresto09 pushed 1 commit. 5608b74cba25b571428da9a055e696ddb053a247 Automatically set the cursor position on the visible line -- View it on GitHub: https://github.com/geany/geany-plugins/pull/1326/files/be15445fa3fd4893f04945af6af94522db2e5e99..5608b74cba25b571428da9a055e696ddb053a247 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany-plugins] Vimode: Fix cursor hang with folded lines (PR #1326)
I'm not sure this is the correct way to address the issue. I think we should take the folds into account and perform the motion commands on top of the visible lines instead of the document lines. I tried to replace this PR with https://github.com/geany/geany-plugins/pull/1338 Please let me know what you think about the changes. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany-plugins/pull/1326#issuecomment-2077621344 You are receiving this because you are subscribed to this thread. Message ID:
