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.

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

use terminal cursor #953

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

gotroyb127
Copy link

@gotroyb127 gotroyb127 commented Jun 11, 2021

Related issues: #927, #462, #952.

With this, the primary cursor is not drawn via styling the cell it is positioned at, but instead it has the terminal draw it. Consequently, the STYLE_CURSOR_PRIMARY is removed, as it is now unused. Also, I added STYLE_CURSOR_MATCHING to specify the style for the matching pair of a (){}[]`"', which is the same for primary and secondary cursors (it is trivial to change it so that one can specify different style for those).

About the implementation:
I have removed all ocurances of curs_set(.) and cursor_visible(.) in ui-terminal-{curses,vt100}.c respectively, which I am unsure if it is the way you would want it.

Also, I just renamed STYLE_CURSOR_PRIMARY to STYLE_CURSOR_MATCHING in all files (lua/themes/*.lua included) to make clear that it is not used anymore and also to have the new option appear somewhere.

Something else to notice is that I tried to prevent drawing of the primary selection when it is only the cell under the cursor so that if one changes the cursor style via terminal-specific escape sequences, such as \e[6 q, the underlying cell is not styled specially.

I expect your comments so I can adjust this to something that might be worth merging.

Cheers.

Edit: also related: #911.

@MaxGyver83
Copy link
Contributor

Awesome! Thank you for this.
I was trying to figure out how I can get different cursor styles for insert mode and replace mode before I found your pull request.

I change cursor styles like that (on top of your pull request):

diff --git a/vis-modes.c b/vis-modes.c
index 2e120aa..8dd2803 100644
--- a/vis-modes.c
+++ b/vis-modes.c
@@ -146,6 +146,7 @@ static void vis_mode_normal_enter(Vis *vis, Mode *old) {
                return;
        if (old != mode_get(vis, VIS_MODE_INSERT) && old != mode_get(vis, VIS_MODE_REPLACE))
                return;
+       printf("\e[2 q\n"); /* block cursor */
        if (vis->autoindent && strcmp(vis->key_prev, "<Enter>") == 0) {
                Text *txt = win->file->text;
                for (Selection *s = view_selections(win->view); s; s = view_selections_next(s)) {
@@ -229,6 +230,10 @@ static void vis_mode_visual_leave(Vis *vis, Mode *new) {
 static void vis_mode_insert_replace_enter(Vis *vis, Mode *old) {
        if (!vis->win || vis->win->parent)
                return;
+       if (vis->mode->id == VIS_MODE_REPLACE)
+               printf("\e[4 q\n"); /* underline cursor */
+       else
+               printf("\e[6 q\n"); /* bar cursor */
        if (!vis->action.op) {
                action_reset(&vis->action_prev);
                vis->action_prev.op = &vis_operators[VIS_OP_MODESWITCH];

Not sure if this is the correct way to do it.

@erf
Copy link
Contributor

erf commented Jul 10, 2022

Could this perhaps be made optional via lua config? Either use terminal cursor or configure via style.

@MaxGyver83
Copy link
Contributor

MaxGyver83 commented Jul 10, 2022

Could this perhaps be made optional via lua config? Either use terminal cursor or configure via style.

This would be cool!

Not sure if this is the correct way to do it.

I think it's not the correct way to do it (just talking about my patch on top of this pull request). I get weird behavior when I enter this after starting vis: Sabc<Esc>.

Peek 2022-07-10 12-11

@MaxGyver83
Copy link
Contributor

MaxGyver83 commented Jul 10, 2022

I think it's not the correct way to do it (just talking about my patch on top of this pull request). I get weird behavior when I enter this after starting vis: Sabc<Esc>.

It seems to be working with this patch:

diff --git a/vis-modes.c b/vis-modes.c
index 2e120aa..d414a2b 100644
--- a/vis-modes.c
+++ b/vis-modes.c
@@ -146,6 +146,8 @@ static void vis_mode_normal_enter(Vis *vis, Mode *old) {
                return;
        if (old != mode_get(vis, VIS_MODE_INSERT) && old != mode_get(vis, VIS_MODE_REPLACE))
                return;
+       printf("\e[2 q"); /* block cursor */
+       fflush(stdout);
        if (vis->autoindent && strcmp(vis->key_prev, "<Enter>") == 0) {
                Text *txt = win->file->text;
                for (Selection *s = view_selections(win->view); s; s = view_selections_next(s)) {
@@ -229,6 +231,11 @@ static void vis_mode_visual_leave(Vis *vis, Mode *new) {
 static void vis_mode_insert_replace_enter(Vis *vis, Mode *old) {
        if (!vis->win || vis->win->parent)
                return;
+       if (vis->mode->id == VIS_MODE_REPLACE)
+               printf("\e[4 q"); /* underline cursor */
+       else
+               printf("\e[6 q"); /* bar cursor */
+       fflush(stdout);
        if (!vis->action.op) {
                action_reset(&vis->action_prev);
                vis->action_prev.op = &vis_operators[VIS_OP_MODESWITCH];

Sorry for spamming this pull request. But maybe it's better than no answer at all :-)

@gotroyb127
Copy link
Author

gotroyb127 commented Jul 12, 2022

Hello.

Maybe comments are more related to issue #927 (edit: #781), but it seems worth mentioning
that it is possible (I'd guess preferable also) to handle cursor shapes from lua
scripts.

I had been using this for a long time (I prefer the block shape nowadays :D):

local lastMode
function updateStatus(win)
	if lastMode ~= vis.mode then
		lastMode = vis.mode
		local n
		if lastMode == vis.modes.NORMAL then
			n = 2
		elseif lastMode == vis.modes.OPERATOR_PENDING then
			n = 4
		elseif lastMode == vis.modes.INSERT then
			n = 4
		elseif lastMode == vis.modes.REPLACE then
			n = 4
		elseif lastMode == vis.modes.VISUAL then
			n = 2
		elseif lastMode == vis.modes.VISUAL_LINE then
			n = 2
		end
		io.stderr:write(strf('\x1b[%d q', n))
	end
	-- status drawing/etc...
end

vis.events.subscribe(vis.events.WIN_STATUS, updateStatus)

I guess having an event which is triggered when mode gets changed would be more
straightforward, but this, checking for changes when drawing the status line,
works nicely nevertheless.

Notice that it writes the escape sequences to stderr, which seems to be where
vis writes its drawing (try vis >/dev/null).

Cheers.

@MaxGyver83
Copy link
Contributor

Cool! That's good to know. I hope your PR will be accepted someday.

@hholst80
Copy link

strf

How do I get access to the 'strf' function?

@gotroyb127
Copy link
Author

strf

How do I get access to the 'strf' function?

strf is being used as an alias for string.format: local strf = string.format (https://github.com/gotroyb127/dotfiles/blob/5d72589137d0218ae8990fe8e9185c02b62f70ce/vis/visUtil.lua#L4)

@MaxGyver83
Copy link
Contributor

@martanne, @ninewise , @rnpnr: Is anything wrong with this pull request? Could you please review it and tell if anything is missing or if this feature is unwelcome?

Copy link
Collaborator

@rnpnr rnpnr left a comment

Choose a reason for hiding this comment

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

Hi Petros,

I've tested it with and without curses support enabled and
everything seems fine to me. Personally I see no point in making this
configurable. Also the STYLE_CURSOR_MATCHING addition is very welcome.

Thanks for the patch! I will merge it soon.

@rnpnr
Copy link
Collaborator

rnpnr commented Jul 15, 2023

@gotroyb127
Actually before I merge it can you give a better description for the
'move cursor to the expected location' commit. Is it for drawing |
at the start of the cell instead of the end?

@rnpnr rnpnr mentioned this pull request Aug 1, 2023
8 tasks
@gotroyb127
Copy link
Author

Sorry for taking so long to reply.

Actually before I merge it can you give a better description for the
'move cursor to the expected location' commit. Is it for drawing |
at the start of the cell instead of the end?

Without 'move cursor to the expected location' commit, vis instructs the
terminal to not draw its cursor and vis makes it appear like there is a cursor
by using appropriate foreground and background colors of the would-be cursor cell.

With the 'move cursor to the expected location' commit, the terminal's cursor is
no longer hidden and is repositioned on the expected location -- and vis's primary
cursor is no longer drawn.

I don't understand what you mean by drawing |.

Cheers.

Copy link
Collaborator

@rnpnr rnpnr left a comment

Choose a reason for hiding this comment

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

Hi, so after some more testing I noticed that this seems to break the highlighting of a matching pair when vis is used in a tty. I pushed this branch with the rebased and squashed changes to work off of. I can potentially look into it more if I find some time.

I don't understand what you mean by drawing |.

Sorry I said that because of an added comment in the code. The confusion about the cursor was because it wasn't clear if the commit message was referring to the terminal's cursor or vis' cursor. I understand now, thanks.

@mcepl
Copy link
Contributor

mcepl commented Mar 27, 2024

Also, this doesn’t apply cleanly (or rather not at all) on the current master branch.

@gotroyb127 Could we ask for the rebase, please?

Previously, vis used to hide the terminal's cursor, but now terminal's
cursor is positioned to the active cursor's position.

The STYLE_CURSOR_MATCHING style is added so that it can be adjusted
independently.
@gotroyb127
Copy link
Author

I've reimplemented it (with some improvements) based on current master, and overwritten the previous
branch (I just force-pushed, I don't have any experience with rebase-ing but I'm guessing that'll be enough).

What's different with this new patch is that STYLE_CURSOR_MATCHING is added but does not replace STYLE_CURSOR_PRIMARY (and thus fixes #952).

STYLE_CURSOR_PRIMARY can be set to an empty string (from lua configuration) to not having it styled
specially. This is necessary in cases like #927.

Cheers.

@mcepl
Copy link
Contributor

mcepl commented May 13, 2024

@rnpnr Could you consider and either merge or reject this PR before #1191? This seems to conflict.

@rnpnr
Copy link
Collaborator

rnpnr commented May 13, 2024

@rnpnr Could you consider and either merge or reject this PR before #1191? This seems to conflict.

I like this and want to merge it but I think there is a remaining issue. When you do some actions such as <C-n> to complete word the cursor remains in place instead of moving to the bottom of the screen where its supposed to. I find that this is a little hard to read because there is no separation between vis-menu and the rest of the editor.

@gotroyb127
Copy link
Author

Hi.

I like this and want to merge it but I think there is a remaining issue. When you do some actions such as <C-n> to complete word the cursor remains in place instead of moving to the bottom of the screen where its supposed to.

This is, indeed, not the intended behavior, and I am unsure why this happens. In vis-complete, vis-menu is called with the -b option so it should use the last line of the terminal, independently of where the cursor is currently positioned at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants