Adding cursors does not always trigger update of regions

[rchl]

Describe the bug
When testing #1702 I've noticed that there is minor bug that makes us not trigger some handlers that should trigger on changing selection.

The _update_stored_region_async in:

LSP/plugin/documents.py

Lines 379 to 381 in b117996

	def on_selection_modified_async(self) -> None:
	different, current_region = self._update_stored_region_async()
	if different:

only checks if the first selection has changed so when adding new cursors after the existing one, the different is false and the handlers don't run. It works when adding new cursor before the existing one.

This results in #1702 not working entirely as expected in those circumstances:

Screen.Recording.2022-07-14.at.23.45.40.mov

I haven't tried reproducing this bug with any existing feature but it seems like existing features mostly rely on a single (first) region so it probably works as expected mostly.

[predragnikolic]

I encountered a similar issue when implementing inlay hints,
Potentially related, for more info see:
sublimelsp/LSP-typescript#91 (comment)

(High chance that it is not related, i just saw the self._update_stored_region_async() and I highly think that there is a bug in that function 🙂 as shown in the link)

[jwortmann]

only checks if the first selection has changed so when adding new cursors after the existing one, the different is false and the handlers don't run.

From the docstring of that function, it appears that this behavior is intentional:

LSP/plugin/documents.py

Lines 817 to 826 in 5fc0db5

	def _update_stored_region_async(self) -> Tuple[bool, sublime.Region]:
	"""
	Stores the current first selection in a variable.
	Note that due to this function (supposedly) running in the async worker thread of ST, it can happen that the
	view is already closed. In that case it returns Region(-1, -1). It also returns that value if there's no first
	selection.
	:returns: A tuple with two elements. The second element is the new region, the first element signals whether
	the previous region was different from the newly stored region.
	"""

And as you wrote, for some features like the DocumentHighlights this seems to make sense; it is probably only useful to show the highlights for a single cursor (the first one) at the same time, and it would be wasteful to send another request for DocumentHighlights when another cursor position after the first one is added or changes.

For features which can take multiple cursor position into account (like show code actions or your diagnostics as annotations PR), it would be useful if that function returned two indicators for change of cursor positions, instead of one. For example like

first_different, any_different, current_region = self._update_stored_region_async() 

Then the various features which are triggered on curser change, could be ordered under the relevant condition:

def on_selection_modified_async(self) -> None: 
     first_different, any_different, current_region = self._update_stored_region_async() 
     if first_different:
        # do DocumentHighlights
     if any_different:
        # do Code Actions
        # do diagnostic annotations

[rchl]

Also mentioned this issue in #2182 (comment)

[rchl]

We now have a way to check whether any selection has changed in addition to checking whether first selection has changed so we have a choice to trigger requests on either.

Some features like highlight regions, code action annotations and resolving of code lenses still only checks whether the first region has changed and could in theory result in state not being correct in some cases but it should be so rare that we can address that if there is a need.

For now there is nothing more to do here really so closing.