Add GuiCursor Support

[jgehrig]

Based on #520

Thanks @b-r-o-c-k, your Pull Request was instrumental in putting this together and understanding the various guicursor options. 😄

I have moved some of the cursor code from Shell into ShellWidget, so we can have a clean abstraction. Moving the cursor there allows us to keep Cell immutable, and avoid const_cast. I have also integrated guicursor styling with the new HighlightAttribute map from work on ext_linegrid.

I think this still needs a minimum neovim API version, I am not yet sure which one we should use due to the highlight map and hl_id vs attr_id. I will investigate...

[jgehrig]

@equalsraf I am curious if you have any opinions on how guicursor should enabled/disabled for the GUI.

I had planned on adding an options-check in shell.cpp here:

	if (true) // :GuiCursor? Setting to enable/disable?
	{
		ShellWidget::paintEvent(ev);
		return;
	}

We could do a few things:

1. Always on for clients that support guicursor
2. Add an option, :GuiCursor to this shim; default to on.
3. Add an option, :GuiCursor to this shim; default to off.
4. Add a temporary undocumented setting to QSettings as a fallback in case we break something.

I was thinking we could add an on-by-default setting :GuiCursor to the shim, and check ginit.vim. Any thoughts?

[jgehrig]

Changes should fix Issue #599.

We should also investigate Issue #619, to see if it can be corrected while making these changes.

[justinmk]

any opinions on how guicursor should enabled/disabled for the GUI.

Why is GuiCursor command/function needed? Why not let it be controlled by 'guicursor' option? That's the purpose of Nvim emitting option_set--so that the options can work like gvim.

[jgehrig]

@justinmk Good point. I suppose I didn't fully understand cursor_style_enabled from mode_info_set.

It is nice to have a fallback for disabling new features, in case something is broken. It looks like set guicursor= would be adequate in this case.

[jgehrig]

The option guicursor will be enabled by default. Users could still fall-back to the old cursor via neovim's :set guicursor=.

I still need to check how well this behaves with older versions of nvim. I suspect very old versions will be fine, but versions in between may have issues with hl_id vs attr_id. Tests pending...

[equalsraf]

@equalsraf I am curious if you have any opinions on how guicursor should enabled/disabled for the GUI.

Enable by default since the existing implementation is a bit of an hack in any case. I dont think we need any specific commands for this. We can fallback to current behaviour if something weird happens (like a missing option). I dont see any harm in adding an escape hatch just for disabling this.

[jgehrig]

@equalsraf

Interesting... It looks like nvim-qt does not currently work with nvim <= 0.3.1.

The lowest working version is nvim 0.3.2, which still uses attr_id. So, this PR works fine with every working combination of nvim + nvim-qt.

Enable by default since the existing implementation is a bit of an hack in any case. I dont think we need any specific commands for this. We can fallback to current behaviour if something weird happens (like a missing option).

This sounds good to me. Let's get guicursor implemented, and keep the existing code as a fallback for the short term. Eventually, we can replace the old cursor implementation with guicurosr and some default styling options.

Once this is merged, I will create an Issue to track removal of the old cursor code.

[jgehrig]

The current iteration works down to v0.2.0, however cursor styling is only applied when ext_linegrid is supported and enabled.

The fallback paths from attr_id to hl_id get convoluted. I will try to write some code than cleanly switches between the two. Alternatively, it may be easier to just require ext_linegrid for cursor styling due to the simpler code. The support would still be better than what exists today...

[equalsraf]

Alternatively, it may be easier to just require ext_linegrid for cursor styling due to the simpler code. The support would still be better than what exists today...

I'm ok with this 👍

[jgehrig]

Let's make ext_linegrid a requirement for cursor styling. Keep it Simple! 👍

A user that gets this nvim-qt patch is also likely to have a recent version of nvim too.

Supporting the old highlight API requires more complicated async code, and won't benefit many users. Attached is a (rough) patch to support Neovim API 3 cursor styling.

patch_legacy_grid_cursor.txt

[jgehrig]

@equalsraf

Done. This should be ready for merge.

I have tested the basic features (blink, styling, shape/size), and everything looks good for new and old versions of nvim. We're going with the no cursor coloring without ext_linegrid approach.

A quick test and proof read of the code on someone else's machine wouldn't hurt... There is a decent amount of new and refactored code in this change :)

Thanks!

[jgehrig]

@equalsraf

The merge with #643 has been handled. I did some quick re-testing, and everything still looks good.