Add 'Reason' to SelectionChangeEvent

[johnfn]

In the process of working on a Vim plugin for VSCode I've run into a number of areas where I need a little bit of API help. Apologies if any of my suggestions already exist!

One thing that I would really like is an onClick event. I know that we have a way to see if selections are changed, but the difficulty is that the Vim plugin changes the selections a lot, which makes it difficult to know who changed the selections - me or you. onClick would solve this problem.

[jrieken]

Not sure about onClick but we could enrich the TextEditorSelectionChangeEvent with some sort of hint

[johnfn]

Yeah, that would be great.

[alexdima]

fyi @jrieken - there is a ICursorSelectionChangedEvent.source - 'mouse' will be set there if the mouse handler or touch handler caused it. https://github.com/Microsoft/vscode/blob/master/src/vs/editor/browser/view/viewController.ts#L71

[jrieken]

@alexandrudima I also see ICursorSelectionChangedEvent.reason. How are both reason and source related? Will source be set when reason is Paste?

[jrieken]

I need some naming inspiration for this one... I wonder if source is a good name because thats commonly used for the emitter/sender of event objects (which would be the editor itself following the model). Maybe something like modifiedBy and an enum TextEditorSelectionModifier. Idea:

    export enum TextEditorSelectionModifier {
       Keyboard = 1,
       Mouse = 2
    }

    export interface TextEditorSelectionChangeEvent {

        modifiedBy?: TextEditorSelectionModifier;

        textEditor: TextEditor;
        selections: Selection[];
    }

Then looking at the editor the source is a string which can be arbitrary, like emmet, so I wonder what modifiers we should enumerate for the API. @johnfn Do you only require mouse/onClick or also things keyboard, api etc?

[siegebell]

@jrieken what if multiple extensions want to tweak the selection in response to a keyboard or mouse selection change? The extensions could check for modifiedBy == api (or source != 'editor'?) to prevent infinite looping, but the end result would be nondeterministic.

(I think you mean for api to indicate that an extension changed the selection?)

[jrieken]

I don't understand that how that relates to modifiedBy? Already today listeners can trigger events while being active. Since event delivery happens in the order of subscriptions I don't see the nondeterminism. Having said that, it's usually a bad idea to trigger events while processing them..

[siegebell]

@jrieken I'm talking about the current situation, not the proposed change. Sorry it was tangential, but modifiedBy made me think of preventing instability in handling selection-changed events when the event is retriggered during processing, which made me think about what happens when extensions concurrently attempt to change the selection.

Unless you wait for one handler to complete before calling another, I'm pretty sure the outcome is nondeterministic.

But maybe the right answer is as you suggeted: "don't retrigger an event while processing it." In which case I need to make a feature-request for modifying selections before they are applied...

[jrieken]

In which case I need to make a feature-request for modifying selections before they are applied.

Yeah, that's a different thing because the extension host is only being informed about selection changes and isn't part of the select-operation.

[alexdima]

@jrieken I have complete confidence I can leave this issue entirely to you

[jrieken]

@johnfn Do you only require 'mouse/onClick' or also things 'keyboard', 'api' etc?

@johnfn Can you please answer?

[johnfn]

@jrieken ah, super sorry! I have difficulty sorting through all my Github notifications. 😞

mouse and/or onClick are by far the highest priority. The others are welcome if possible.

[jrieken]

Thanks. I understand mouse as in 'user selected something' explicitly (not because of undo/redo, setting new content etc). Will make the changes early August such that we have enough time to fine tune

[johnfn]

Exactly. Any click or release event, and explicitly not anything else.

[jrieken]

@johnfn I have just pushed the change to master. It would be great if you can give it try with tomorrows insider build and give feedback

[johnfn]

@jrieken ooh, this is a big one. I have been waiting on this for a while. Thanks!

[jrieken]

Thanks and happy coding

[rebornix]

@jrieken I've played with this new API change and most cases work perfectly. The only problem I've run into right now is sometimes TextEditorSelectionChangeKind is undefined, for example, when you insert a code snippet and all placeholders are selected, we receive a selection change with TextEditorSelectionChangeKind set as undefined. Any ideas?

[jrieken]

That's the defined behaviour - there is no guarantee that the source is always known