[vim/vim] avoid heavy redrawing cursor (#1873)
mattn
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/1873
Commit Summary
- avoid heavy redrawing cursor
File Changes
- M src/terminal.c (19)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Codecov
Codecov Report
Merging #1873 into master will increase coverage by
0.02%.
The diff coverage isn/a.
@@ Coverage Diff @@ ## master #1873 +/- ## ========================================== + Coverage 75.04% 75.07% +0.02% ========================================== Files 76 76 Lines 125234 125234 ========================================== + Hits 93984 94016 +32 + Misses 31250 31218 -32
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/version.c | 79.29% <0%> (-1.02%) |
⬇️ |
| src/os_unix.c | 57.15% <0%> (-0.34%) |
⬇️ |
| src/message.c | 67.9% <0%> (-0.21%) |
⬇️ |
| src/getchar.c | 73.36% <0%> (-0.05%) |
⬇️ |
| src/channel.c | 83.55% <0%> (-0.05%) |
⬇️ |
| src/gui.c | 45.58% <0%> (+0.05%) |
⬆️ |
| src/gui_gtk_x11.c | 47.5% <0%> (+0.15%) |
⬆️ |
| src/libvterm/src/screen.c | 74.62% <0%> (+0.15%) |
⬆️ |
| src/term.c | 52.99% <0%> (+0.23%) |
⬆️ |
| src/window.c | 81.32% <0%> (+0.53%) |
⬆️ |
| ... and 1 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 1c84493...1ad38b3. Read the comment docs.
mattn
@brammool I found another solution. Which do you like?
diff --git a/src/terminal.c b/src/terminal.c index 5be51de9f..4ceb57cd9 100644 --- a/src/terminal.c +++ b/src/terminal.c @@ -636,7 +636,7 @@ handle_movecursor( } } - if (is_current) + if (is_current && visible) update_cursor(); return 1;
Kazunobu Kuriyama
I found an issue with this PR's (original) patch.
For instance, when I do cat userfunc.c on the built-in terminal of a patched Athena, Motif, or GTK+ 2 GUIs, I often see the cursor placed in a wrong position when scrolling is done.
As for the diff that has just posted, I haven't seen the same issue so far.
But, @mattn , I have one thing to make sure about the diff before giving my conclusion: Which HEAD should I apply it to? As no instructions are given, I applied it to the current master (8.0.0701) and tested it. But the line number 636 shown in the diff doesn't look like that the diff was made against the current HEAD, which makes me wonder.
Or, does "another solution" mean that we should apply the diff on top of this PR's own patch?
mattn
Did you try my first patch? or second one? The first patch is based on previous revision. Second one is based on master/HEAD.
Kazunobu Kuriyama
Did you try my first patch? or second one?
I did try the first patch and found the issue as I wrote.
The first patch is based on previous revision.
Can I have its hash? I have no idea on what you are referring to "previous revision."
Second one is based on master/HEAD.
That's the point of my question. Actually, when I applied the posted patch to my clone repo points to master/HEAD, I got
patching file terminal.c
Hunk #1 succeeded at 625 (offset -11 lines).
It looks to me that the offset indicates that the diff was not made against master/HEAD.
mattn
Sorry. I make you confusing. Okay, I'll rebase from master.
Kazunobu Kuriyama
OK, 'cause I have to be AFK for hours, let me leave my conclusion now:
- While Patch 8.0.0771 has improved the cursor drawing of the GTK+ 2 GUI much and made its built-in terminal as usable as that of the GTK+ 3 GUI, it has made scrolling of the latter GUI noticeably clumsy.
- If we merge @mattn 's second patch into the current master/HEAD (1c84493) with leaving his first patch out, then it not only makes scrolling of the former GUI much smoother but also makes that of the latter GUI as smooth as pre-8.0771 versions.
- Even with the second patch included, the Athena and Motif GUIs still needs further improvement on their cursor drawing and movement.
Therefore, though I'm still unsure whether or not the second patch was made against master/HEAD and whether or not the executables resulting from the codebase of his repo which was used to make it are the same as those I have and were tested properly, I vote against the first patch and for the second patch, based on my own test result.
mattn
@nuko8 Thanks for your review. As you mentioned about patch 8.0.0771, the cursor had appear in right location immediately. However, when lines are scrolled-up, cursor flicker many times. This patch is the avoid redrawing. To update cursor smooth, it should not call gui_update_cursor in many part. And it call gui_update_cursor at handle_movecursor while visible is true.
Kazunobu Kuriyama
Thank you for the clarification. Now I see the issue better and confirm that your second patch works just like you described. I highly appreciate your work on it as well as your recent intensive work on other part of the built-in terminal, helping Bram. Keep up the good work!
Bram Moolenaar
Yasuhiro Matsumoto wrote:
> -- Commit Summary --
>
> * avoid heavy redrawing cursor
>
> -- File Changes --
>
> M src/terminal.c (19)
Thanks. I found one place a flush is still needed, otherwise the screen
is not updated after ":term ls" ends.
--
hundred-and-one symptoms of being an internet addict:
247. You use www.switchboard.com instead of dialing 411 and 555-12-12
for directory assistance.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Bram Moolenaar
Yasuhiro Matsumoto wrote:
> @brammool I found another solution. Which do you like?
>
> ```diff
> diff --git a/src/terminal.c b/src/terminal.c
> index 5be51de9f..4ceb57cd9 100644
> --- a/src/terminal.c
> +++ b/src/terminal.c
> @@ -636,7 +636,7 @@ handle_movecursor(
> }
> }
>
> - if (is_current)
> + if (is_current && visible)
> update_cursor();
>
> return 1;
We should store the property for the cursor visibility.
I'll merge it in.
--
hundred-and-one symptoms of being an internet addict:
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—