how does wait handling work in the new system?

1. IWaitIndicator/IWaitContext are platformazed and public (need better names though - we end up with IWaitableUIOperationContext kind of name). I'll use old names to make simpler to explain.
2. All command handlers are executed under a shared wait indicator/context set up by the editor and can obtain wait context via CommandExecutionContext argument.
3. With the wait context command handlers get shared cancellation token.
4. Because the wait context is shared between command handlers, I added a concept of "wait context scope". When a command handler needs to affect the shared wait context (allow or disallow cancellation, add its own message or progress) it pushes a new wait context scope and disposes it when it's done. Shared wait context deals with aggregating these scopes into the wait dialog. Cancellability is aggregated in a sticky way though - if one handler pushed a scope to disallow cancellation and didn't just handle the command, it stays disabled even after the handler is done.

@CyrusNajmabadi I linked the spec in description and did a bunch of improvements. Please take another look when you have time. Thanks!

@CyrusNajmabadi
Regarding additional 3000 lines: most of them are coming from new localized command handler names in xlf files (25 handlers x 10 lines per new string x number of languages).

Regarding chaining - I'm going to disappoint you but we gave up on pure non-chained approach and ended up with dual model (simple and if needed - chained). See spec for design consideration.

Regarding chaining - I'm going to disappoint you but we gave up on pure non-chained approach and ended up with dual model (simple and if needed - chained)

That doesn't disappoint me :) I'm not actually surprised given the complexity of some of our chains.

which did this change from IsAvailable to IsUndetermined?

This test used to verify that in some condition this handler delegates GetComandState() to the next handler and result is CommandState.Unavailable. After the migration, this handler is now a simple non-chained command handler that has no next handler reference. Instead of delegating to the next, it returns CommandState.Undetermined, which indicates that it should be ignored and next handler should be called. So the test was updated to validate this instead.

Is there a link to the docs on these types?

I wish I could just publish the spec on GitHub :( Hopefully next features we will spec that way.

Essentially, the goal was to make simple handling simple and still allow advanced chaining scenarios. So we ended up with 2 ways to implement a command handler:

public interface ICommandHandler<T> : IDiagnosableCommandHandler where T : CommandArgs
{
    CommandState GetCommandState(T args);
    bool ExecuteCommand(T args, CommandExecutionContext executionContext);
}

and

public interface IChainedCommandHandler<T> : IDiagnosableCommandHandler where T : CommandArgs
{        
    CommandState GetCommandState(T args, Func<CommandState> nextCommandHandler);
    void ExecuteCommand(T args, Action nextCommandHandler, CommandExecutionContext executionContext);
}

IDiagnosableCommandHandler simply adds a DisplayName for diagnostics:

public interface IDiagnosableCommandHandler : ICommandHandler
{
        /// <summary>
        /// Gets display name of the command handler used to represent it to the user, for
        /// example when blaming it for delays or for commanding diagnostics.
        /// </summary>
        string DisplayName { get; }
}

IChainedCommandHandler works just as before, but ICommandHandler has no access to the next handler so the commanding service takes care of calling its next handler if needed (specifically when GetCommandState() returns CommandState.Undetermined or ExecuteCommand() returns false).

@olegtk I admit I would have guessed IDiagnosableCommandHandler had ICommandHandler as a base type, not the other way around. It seems like IDiagnosableCommandHandler is just IHasDisplayName or something? Can we make a better name? Because I suspect you'll want that exact same thing for completion providers being slow, etc.

good point, I'm going to go with MS.VS.Utilities.IHasDisplayName and
public interface ICommandHandler<T> : ICommandHandler, IHasDisplayName where T : CommandArgs

public interface IChainedCommandHandler<T> : ICommandHandler, IHasDisplayName where T : CommandArgs

what happened here?

undone

i'm tring to understand the relation between allowCancellation and .CancellationToken. Is it a stack? What happens if some other code sets allowCancellation to false? Will that affect our cancelation token?

Maybe it would be clearer if the scope itself had the CancellationToken?

It's a two way cancellability supported by the threaded wait dialog underneath this abstraction. UserCancellationToken allows user to (attempt to) cancel a command and AllowCancellation allows command handler to disable Cancel button.

I renamed it to UserCancellation token, does it look more readable now?

using (context.WaitContext.AddScope(allowCancellation: true, EditorFeaturesResources.Finding_token_to_rename))
{
    ExecuteRenameWorker(args, context.WaitContext.UserCancellationToken);
}

is there docs on this context obejct?

It's just a holder of useful stuff commanding service wants to provide to command handlers. Currently it only has wait context:

namespace Microsoft.VisualStudio.Commanding
{
    /// <summary>
    /// Represents a command execution context, which is set up by a command handler service
    /// and provided to each command handler.
    /// </summary>
    public sealed class CommandExecutionContext
    {
        /// <summary>
        /// Creates new instance of the <see cref="CommandExecutionContext"/>.
        /// </summary>
        public CommandExecutionContext(IUIThreadOperationContext waitContext)
        {
            this.WaitContext = waitContext ?? throw new ArgumentNullException(nameof(waitContext));
        }

        /// <summary>
        /// Provides a context of executing a command handler on the UI thread, which
        /// enables two way shared cancellability and wait indication.
        /// </summary>
        public IUIThreadOperationContext WaitContext { get; }
    }
}

namespace Microsoft.VisualStudio.Utilities
{
    /// <summary>
    /// Represents a context of executing potentially long running operation on the UI thread, which
    /// enables shared two way cancellability and wait indication.
    /// </summary>
    /// <remarks>
    /// Instances implementing this interface are produced by <see cref="IUIThreadOperationExecutor"/>
    /// MEF component.
    /// </remarks>
    public interface IUIThreadOperationContext : IPropertyOwner, IDisposable
    {
        /// <summary>
        /// Cancellation token that allows user to cancel the operation unless the operation
        /// is not cancellable.
        /// </summary>
        CancellationToken UserCancellationToken { get; }

        /// <summary>
        /// Gets whether the operation can be cancelled.
        /// </summary>
        /// <remarks>This value is composed of initial AllowCancellation value and
        /// <see cref="IUIThreadOperationScope.AllowCancellation"/> values of all currently added scopes.
        /// The value composition logic takes into acount disposed scopes too - if any of added scopes
        /// were disposed while its <see cref="IUIThreadOperationScope.AllowCancellation"/> was false,
        /// this property will stay false regardless of all other scopes' <see cref="IUIThreadOperationScope.AllowCancellation"/>
        /// values.
        /// </remarks>
        bool AllowCancellation { get; }

        /// <summary>
        /// Gets user readable operation description, composed of initial context description and
        /// descriptions of all currently added scopes.
        /// </summary>
        string Description { get; }

        /// <summary>
        /// Gets current list of <see cref="IUIThreadOperationScope"/>s in this context.
        /// </summary>
        IEnumerable<IUIThreadOperationScope> Scopes { get; }

        /// <summary>
        /// Adds a UI thread operation scope with its own two way cancellability, description and progress tracker.
        /// The scope is removed from the context on dispose.
        /// </summary>
        IUIThreadOperationScope AddScope(bool allowCancellation, string description);

        /// <summary>
        /// Allows a component to take full ownership over this UI thread operation, for example
        /// when it shows its own modal UI dialog and handles cancellability through that dialog instead.
        /// </summary>
        void TakeOwnership();
    }
}

system should sort before MS.

will do in all modified files

fixed

What does Undetermined mean?

It's a declarative way of expressing "command is enabled if anybody else says so, but I personally have no opinion whatsoever", a simpler alternative to implementing IChainedComamndHandler and delegating GetCommandState to the next handler.
About 25% of Roslyn command handlers (typically those "customizing" editor commands) are like that so I assume this is pretty important case for any lang service and I'd like to make it simple.
Better naming suggestions are welcome.

Gotcha. Thanks!

CommandState.NoOpinion :D .EnabledIfSomeoneElseWantsIt :D

Yeah, "Undetermined" still isn't what I'd call it. NoOpinion seems great.

Or perhaps even "Unspecified". Undetermined is the result of a computation, not an input.

@jasonmalinowski Unspecified is better than Undetermined indeed. What about Unknown? #Resolved

ok, went with Unspecified

In reply to: 159562654 [](ancestors = 159562654)

the next line (that checks of operations is null) seems to eb gone.

I can see it here below

can you explain the old/new logic here. (alternatively, you can try to keep indentation the same so that hte diff is clearer).

there is no change in logic here, just 4 less spaces. Instead of

waitIndicator.Wait(
    title: EditorFeaturesResources.Automatic_Line_Ender,
    message: EditorFeaturesResources.Automatically_completing,
    allowCancel: false, action: w =>
    {
        ... code ...
    }

it's now

using (context.WaitContext.AddScope(allowCancellation: false, EditorFeaturesResources.Automatically_completing))
{
    ... code ...
}

Hmm, I don't see how can I increase indentation in the modified code :(

Btw, this looks sooo much cleaner in CodeFlow :)

In reply to: 159564396 [](ancestors = 159564396)

Not certain if i can use codeflow :-/

where did this code go?

AbstractBlockCommentEditingCommandHandler is used by TypeScript so I cannot change it. So I refactored it into BaseAbstractBlockCommentEditingCommandHandler and PlatformAbstractBlockCommentEditingCommandHandler (not happy about that name though). Once TypeScript migrates to the new commanding, I will simplify it back.

Never mind, it's better to have this class implement both ICommandHandler interfaces now and once TypeScript migrates over I can just delete legacy part.

In reply to: 159565154 [](ancestors = 159565154)

extra newline.

fixed

hmmm, this indicates some important difference. Seems like the editor cannot find proper buffer to execute "ToggleSuggestionMode" command before "#" is typed, while Roslyn could.

fixed the issue in the editor bits so now reverting this change back

Is the intent that these strings can be shown to the user? If so, do we need to make them semi-understandable by users?

nameof?

done

Convention is we are using the real value between _s not the semantic name. In this case, not sure why we can't just use the previous value.

will update all new strings to follow the convention

using Microsoft.VisualStudio.Commanding?

@olegtk I admit I would have guessed IDiagnosableCommandHandler had ICommandHandler as a base type, not the other way around. It seems like IDiagnosableCommandHandler is just IHasDisplayName or something? Can we make a better name? Because I suspect you'll want that exact same thing for completion providers being slow, etc.

crefs?

done

In reply to: 158573958 [](ancestors = 158573958)

static

private readonly

Is this the editor-defined base type? Not sure "Abstract" should actually be in the type name...?

I forget the layering here -- should this be somewhere lower in EditorFeatures? Or CommandBindingDefinition is the VS-only bit?

Yes, binding is host specific so it's VS only thing, VS for Mac would need another way to bind.

title: EditorFeaturesResources.Find_References, [](start = 16, length = 47)

note that we are losing the title now

Not much of value was lost.

cancellationToken [](start = 60, length = 17)

here is another case, the cancellation is disabled but still using token.
Also I don't really understand why we disable cancellability here, but not in AbstractChangeSignatureCommandHandler for example.