Context
The Visual Basic Editor (VBE) does a good job at completing member scopes, for example when you type this and hit ENTER...
Private Sub DoSomething
You get this for free:
Private Sub DoSomething()
End Sub
The problem is that VBA is a very, uhm, blocky language, and the VBE doesn't automatically complete any of the blocks. For example when you type this and hit ENTER...
With Something
All you get is this:
With Something
And what you'd like is this:
With Something
End With
The next release of Rubberduck will fix this, and more. Here's how.
Interface & Wiring
I started with an IAutoComplete interface that looks like this:
namespace Rubberduck.AutoComplete
{
public interface IAutoComplete
{
string InputToken { get; }
string OutputToken { get; }
bool Execute(AutoCompleteEventArgs e, AutoCompleteSettings settings);
bool IsInlineCharCompletion { get; }
bool IsEnabled { get; set; }
}
}
This interface is flexible enough to allow toggling each individual implementation on/off using Rubberduck settings, and the InputToken / OutputToken properties are flexible enough to support both inline and block autocompletion.
The AutoCompleteEventArgs class is defined in the Rubberduck.VBEditor project, which is responsible for the lower-level VBE messenging hooks and COM wrappers:
namespace Rubberduck.VBEditor.Events
{
public class AutoCompleteEventArgs : EventArgs
{
public AutoCompleteEventArgs(ICodeModule module, WindowsApi.KeyPressEventArgs e)
{
if (e.Key == Keys.Delete ||
e.Key == Keys.Back ||
e.Key == Keys.Enter ||
e.Key == Keys.Tab)
{
Keys = e.Key;
}
else
{
Character = e.Character;
}
CodeModule = module;
CurrentSelection = module.GetQualifiedSelection().Value.Selection;
CurrentLine = module.GetLines(CurrentSelection);
}
/// <summary>
/// <c>true</c> if the character has been handled, i.e. written to the code pane.
/// Set to <c>true</c> to swallow the character and prevent the WM message from reaching the code pane.
/// </summary>
public bool Handled { get; set; }
/// <summary>
/// The CodeModule wrapper for the module being edited.
/// </summary>
public ICodeModule CodeModule { get; }
/// <summary>
/// <c>true</c> if the event is originating from a <c>WM_CHAR</c> message.
/// <c>false</c> if the event is originating from a <c>WM_KEYDOWN</c> message.
/// </summary>
/// <remarks>
/// Inline completion is handled on WM_CHAR; deletions and block completion on WM_KEYDOWN.
/// </remarks>
public bool IsCharacter => Keys == default;
/// <summary>
/// The character whose key was pressed. Undefined value if <see cref="Keys"/> isn't `<see cref="Keys.None"/>.
/// </summary>
public char Character { get; }
/// <summary>
/// The actionnable key that was pressed. Value is <see cref="Keys.None"/> when <see cref="IsCharacter"/> is <c>true</c>.
/// </summary>
public Keys Keys { get; }
/// <summary>
/// The current location of the caret.
/// </summary>
public Selection CurrentSelection { get; }
/// <summary>
/// The contents of the current line of code.
/// </summary>
public string CurrentLine { get; }
}
}
The EventArgs object is created by VBENativeServices in a static handler (which needs to be static for various reasons) that is invoked when a key is pressed in the editor's code pane:
public static event EventHandler<AutoCompleteEventArgs> KeyDown;
private static void OnKeyDown(KeyPressEventArgs e)
{
using (var pane = GetCodePaneFromHwnd(e.Hwnd))
{
using (var module = pane.CodeModule)
{
var args = new AutoCompleteEventArgs(module, e);
KeyDown?.Invoke(_vbe, args);
e.Handled = args.Handled;
}
}
}
The value of the Handled property is carried to the invoking native handler, which then uses this value to determine whether to "swallow" the keypress or let it through to the code pane - this all happens in the rather low-level CodePaneSubClass class:
namespace Rubberduck.VBEditor.WindowsApi
{
public class KeyPressEventArgs
{
public KeyPressEventArgs(IntPtr hwnd, IntPtr wParam, IntPtr lParam, char character = default)
{
Hwnd = hwnd;
WParam = wParam;
LParam = lParam;
Character = character;
if (character == default(char))
{
Key = (Keys)wParam;
}
else
{
IsCharacter = true;
}
}
public bool IsCharacter { get; }
public IntPtr Hwnd { get; }
public IntPtr WParam { get; }
public IntPtr LParam { get; }
public bool Handled { get; set; }
public char Character { get; }
public Keys Key { get; }
}
//Stub for code pane replacement. :-)
internal class CodePaneSubclass : FocusSource
{
public ICodePane CodePane { get; }
internal CodePaneSubclass(IntPtr hwnd, ICodePane pane) : base(hwnd)
{
CodePane = pane;
}
public override int SubClassProc(IntPtr hWnd, IntPtr msg, IntPtr wParam, IntPtr lParam, IntPtr uIdSubclass, IntPtr dwRefData)
{
KeyPressEventArgs args;
switch ((WM)msg)
{
case WM.CHAR:
args = new KeyPressEventArgs(hWnd, wParam, lParam, (char)wParam);
OnKeyDown(args);
if (args.Handled) { return 0; }
break;
case WM.KEYDOWN:
args = new KeyPressEventArgs(hWnd, wParam, lParam);
OnKeyDown(args);
if (args.Handled) { return 0; }
break;
}
return base.SubClassProc(hWnd, msg, wParam, lParam, uIdSubclass, dwRefData);
}
protected override void DispatchFocusEvent(FocusType type)
{
var window = VBENativeServices.GetWindowInfoFromHwnd(Hwnd);
if (!window.HasValue)
{
return;
}
OnFocusChange(new WindowChangedEventArgs(window.Value.Hwnd, window.Value.Window, CodePane, type));
}
}
}
One concern is that a decision was taken to handle both WM_CHAR and WM_KEYDOWN, because CHAR conveniently gives us the exact char to use, but only KEYDOWN can tell us whether the DELETE or <kbd>BACKSPACE</kbd> key was pressed - so the KeyPressEventArgs contain information that tells us whether we need to look at the Character or the Keys enum value when we handle the keypress... and this makes the name KeyPress rather all-encompassing.
AutoCompleteService
The component responsible for handling the low-level VBENativeServices.KeyDown event gets the settings service and the IAutoComplete provider constructor-injected - the class is needs to handle changes in configuration settings, which could conceivably be toggling one or more autocompletes' IsEnabled state at any point in time.. but we don't want to re-load the settings file at every keypress either:
namespace Rubberduck.AutoComplete
{
public class AutoCompleteService : SubclassingWindow, IDisposable
{
private readonly IGeneralConfigService _configService;
private readonly List<IAutoComplete> _autoCompletes;
private AutoCompleteSettings _settings;
public AutoCompleteService(IGeneralConfigService configService, IAutoCompleteProvider provider)
{
_configService = configService;
_autoCompletes = provider.AutoCompletes.ToList();
ApplyAutoCompleteSettings(configService.LoadConfiguration());
_configService.SettingsChanged += ConfigServiceSettingsChanged;
VBENativeServices.KeyDown += HandleKeyDown;
}
private void ConfigServiceSettingsChanged(object sender, ConfigurationChangedEventArgs e)
{
var config = _configService.LoadConfiguration();
ApplyAutoCompleteSettings(config);
}
private void ApplyAutoCompleteSettings(Configuration config)
{
_settings = config.UserSettings.AutoCompleteSettings;
foreach (var autoComplete in _autoCompletes)
{
var setting = config.UserSettings.AutoCompleteSettings.AutoCompletes.FirstOrDefault(s => s.Key == autoComplete.GetType().Name);
if (setting != null && autoComplete.IsEnabled != setting.IsEnabled)
{
autoComplete.IsEnabled = setting.IsEnabled;
continue;
}
}
}
private void HandleKeyDown(object sender, AutoCompleteEventArgs e)
{
var module = e.CodeModule;
var qualifiedSelection = module.GetQualifiedSelection();
var selection = qualifiedSelection.Value.Selection;
if (e.Keys != Keys.None && selection.LineCount > 1 || selection.StartColumn != selection.EndColumn)
{
return;
}
var currentContent = module.GetLines(selection);
var handleDelete = e.Keys == Keys.Delete && selection.EndColumn <= currentContent.Length;
var handleBackspace = e.Keys == Keys.Back && selection.StartColumn > 1;
foreach (var autoComplete in _autoCompletes.Where(auto => auto.IsEnabled))
{
if (handleDelete || handleBackspace)
{
if (DeleteAroundCaret(e, autoComplete))
{
break;
}
}
else
{
if (autoComplete.Execute(e, _settings))
{
break;
}
}
}
}
private bool DeleteAroundCaret(AutoCompleteEventArgs e, IAutoComplete autoComplete)
{
if (autoComplete.IsInlineCharCompletion)
{
var code = e.CurrentLine;
// If caret LHS is the AC input token and RHS is the AC output token, we can remove both.
// Substring index is 0-based. Selection from code pane is 1-based.
// LHS should be at StartColumn - 2, RHS at StartColumn - 1.
var caretLHS = code.Substring(Math.Max(0, e.CurrentSelection.StartColumn - 2), 1);
var caretRHS = code.Length >= e.CurrentSelection.StartColumn
? code.Substring(e.CurrentSelection.StartColumn - 1, 1)
: string.Empty;
if (caretLHS == autoComplete.InputToken && caretRHS == autoComplete.OutputToken)
{
var left = code.Substring(0, e.CurrentSelection.StartColumn - 2);
var right = code.Substring(e.CurrentSelection.StartColumn);
using (var pane = e.CodeModule.CodePane)
{
e.CodeModule.ReplaceLine(e.CurrentSelection.StartLine, left + right);
pane.Selection = new Selection(e.CurrentSelection.StartLine, e.CurrentSelection.StartColumn - 1);
e.Handled = true;
}
return true;
}
}
return false;
}
public void Dispose()
{
VBENativeServices.KeyDown -= HandleKeyDown;
if (_configService != null)
{
_configService.SettingsChanged -= ConfigServiceSettingsChanged;
}
_autoCompletes.Clear();
}
}
}
So the service handles deleting & backspacing. Two classes handle inline and block completion, respectively.
Inline Autocompletion
The simplest form of autocompletion is the "inline" kind. Since all inline autocompletes work the same way, the execution logic is implemented in a base class:
namespace Rubberduck.AutoComplete
{
public abstract class AutoCompleteBase : IAutoComplete
{
protected AutoCompleteBase(string inputToken, string outputToken)
{
InputToken = inputToken;
OutputToken = outputToken;
}
public bool IsInlineCharCompletion => InputToken.Length == 1 && OutputToken.Length == 1;
public bool IsEnabled { get; set; }
public string InputToken { get; }
public string OutputToken { get; }
public virtual bool Execute(AutoCompleteEventArgs e, AutoCompleteSettings settings)
{
if (!e.IsCharacter || !IsInlineCharCompletion)
{
return false;
}
var module = e.CodeModule;
using (var pane = module.CodePane)
{
var selection = pane.Selection;
if (e.Character.ToString() == InputToken)
{
var code = module.GetLines(selection).Insert(Math.Max(0, selection.StartColumn - 1), InputToken + OutputToken);
module.ReplaceLine(selection.StartLine, code);
pane.Selection = new Selection(selection.StartLine, selection.StartColumn + 1);
e.Handled = true;
return true;
}
return false;
}
}
}
}
Classes derived from the above base class include:
public class AutoCompleteClosingBrace : AutoCompleteBase
{
public AutoCompleteClosingBrace()
: base("{", "}") { }
}
And:
public class AutoCompleteClosingParenthese : AutoCompleteBase
{
public AutoCompleteClosingParenthese()
:base("(", ")") { }
}
And also:
public class AutoCompleteClosingString : AutoCompleteBase
{
public AutoCompleteClosingString()
: base("\"", "\"") { }
}
IMO it does smell that each class has exactly nothing but constructor arguments. OTOH it does make it very easy to use reflection to automagically have the AutoCompleteProvider know about all supported autocompletions. Any better ideas?
Block Autocompletion
Auto-completing code blocks is a tad more complicated: since Rubberduck has a Smart Indenter with a bunch of settings, not honoring the indenter settings wouldn't feel right, so we want the derived types to be able to override some predicate that determines whether the block body should be indented. But we also want to avoid completing a block that's actually a comment, and then If and #If tokens need to be treated differently (i.e. precompiler directives). The result is a little bit hairy, but the derived classes are just as neat (/smelly?) as the inline autocompletes:
namespace Rubberduck.AutoComplete
{
public abstract class AutoCompleteBlockBase : AutoCompleteBase
{
/// <param name="indenterSettings">Used for auto-indenting blocks as per indenter settings.</param>
/// <param name="inputToken">The token that starts the block, i.e. what to detect.</param>
/// <param name="outputToken">The token that closes the block, i.e. what to insert.</param>
protected AutoCompleteBlockBase(IConfigProvider<IndenterSettings> indenterSettings, string inputToken, string outputToken)
:base(inputToken, outputToken)
{
IndenterSettings = indenterSettings;
}
protected virtual bool FindInputTokenAtBeginningOfCurrentLine => false;
protected virtual bool SkipPreCompilerDirective => true;
protected readonly IConfigProvider<IndenterSettings> IndenterSettings;
protected virtual bool ExecuteOnCommittedInputOnly => true;
protected virtual bool MatchInputTokenAtEndOfLineOnly => false;
protected virtual bool IndentBody => true;
public override bool Execute(AutoCompleteEventArgs e, AutoCompleteSettings settings)
{
var ignoreTab = e.Keys == Keys.Tab && !settings.CompleteBlockOnTab;
if (IsInlineCharCompletion || e.Keys == Keys.None || ignoreTab)
{
return false;
}
var module = e.CodeModule;
using (var pane = module.CodePane)
{
var selection = pane.Selection;
var code = module.GetLines(selection);
if (SkipPreCompilerDirective && code.Trim().StartsWith("#"))
{
return false;
}
var pattern = SkipPreCompilerDirective
? $"\\b{InputToken}\\b"
: $"{InputToken}\\b"; // word boundary marker (\b) would prevent matching the # character
var isMatch = MatchInputTokenAtEndOfLineOnly
? code.EndsWith(InputToken, System.StringComparison.OrdinalIgnoreCase)
: Regex.IsMatch(code.Trim(), pattern, RegexOptions.IgnoreCase);
if (isMatch && !code.HasComment(out _) && !IsBlockCompleted(module, selection))
{
var indent = code.TakeWhile(c => char.IsWhiteSpace(c)).Count();
var newCode = OutputToken.PadLeft(OutputToken.Length + indent, ' ');
var stdIndent = IndentBody ? IndenterSettings.Create().IndentSpaces : 0;
module.InsertLines(selection.NextLine.StartLine, "\n" + newCode);
module.ReplaceLine(selection.NextLine.StartLine, new string(' ', indent + stdIndent));
pane.Selection = new Selection(selection.NextLine.StartLine, indent + stdIndent + 1);
e.Handled = true;
return true;
}
return false;
}
}
private bool IsBlockCompleted(ICodeModule module, Selection selection)
{
string content;
var proc = module.GetProcOfLine(selection.StartLine);
if (proc == null)
{
content = module.GetLines(1, module.CountOfDeclarationLines);
}
else
{
var procKind = module.GetProcKindOfLine(selection.StartLine);
var startLine = module.GetProcStartLine(proc, procKind);
var lineCount = module.GetProcCountLines(proc, procKind);
content = module.GetLines(startLine, lineCount);
}
var options = RegexOptions.IgnoreCase;
var inputPattern = $"(?<!{OutputToken.Replace(InputToken, string.Empty)})\\b{InputToken}\\b";
var inputMatches = Regex.Matches(content, inputPattern, options).Count;
var outputMatches = Regex.Matches(content, $"\\b{OutputToken}\\b", options).Count;
return inputMatches > 0 && inputMatches == outputMatches;
}
}
}
The matching logic allows this single AutoCompleteDoBlock class to work with Do...Loop, Do Until...Loop, and Do While...Loop constructs:
public class AutoCompleteDoBlock : AutoCompleteBlockBase
{
public AutoCompleteDoBlock(IIndenterSettings indenterSettings)
: base(indenterSettings, $"{Tokens.Do}", Tokens.Loop) { }
}
And Enum blocks may or may not be indented, depending on indenter settings:
public class AutoCompleteEnumBlock : AutoCompleteBlockBase
{
public AutoCompleteEnumBlock(IIndenterSettings indenterSettings)
: base(indenterSettings, $"{Tokens.Enum}", $"{Tokens.End} {Tokens.Enum}") { }
protected override bool IndentBody => IndenterSettings.IndentEnumTypeAsProcedure;
}
Everything works pretty well (see it in action on YouTube), with one known edge case I'm not sure I want to address: rule of thumb, in order for it to break your VBA code, you pretty much need to be actively trying to break the feature.
Does anything look off?
These two implementations in particular, kind of smell funny:
public class AutoCompleteIfBlock : AutoCompleteBlockBase
{
public AutoCompleteIfBlock(IIndenterSettings indenterSettings)
: base(indenterSettings, $"{Tokens.Then}", $"{Tokens.End} {Tokens.If}") { }
protected override bool MatchInputTokenAtEndOfLineOnly => true;
}
public class AutoCompleteOnErrorResumeNextBlock : AutoCompleteBlockBase
{
public AutoCompleteOnErrorResumeNextBlock(IIndenterSettings indenterSettings)
: base(indenterSettings, $"{Tokens.On} {Tokens.Error} {Tokens.Resume} {Tokens.Next}", $"{Tokens.On} {Tokens.Error} {Tokens.GoTo} 0") { }
protected override bool ExecuteOnCommittedInputOnly => false;
}
Feels like the base class is going to be growing with members as per some specific derived type's needs - like the If...End If autocomplete that needs to account for VBA's totally legal single-line If {condition} Then {statement} syntax, which Rubberduck shouldn't try to complete... On the other hand most implementations do have extremely similar mechanisms, so the base implementation makes a rather sensible default.
2 Answers 2
Like t3chb0t, I am not familiar with VBA, but I will mention a few things that caught my eye:
1) Lack of documentation. To me it is surprising, that you went through the trouble of documenting a class that is an event argument, but completely left out IAutoComplete interface, which plays a pivotal role in this entire thing. I can make an educated guess on what most of the members do but by no means it is obvious.
2) config.UserSettings.AutoCompleteSettings.AutoCompletes.FirstOrDefault - here you can use dictionary instead of doing a linear lookup.
3) autoComplete.IsEnabled = setting.IsEnabled; - you might want to consider replacing it with autoComplete.ApplySettings(setting). Even though it "leaks" settings class to the interface, it makes a job of extending and/or customizing settings much easier.
4) Personally, I prefer to split bool Execute into void Execute and bool CanExecute (what Microsoft does in ICommand interface). I think it separates concerns better and allows to override those methods individually. It also makes it clear, that bool is not the result of operation (success/failure) but an indication of whether or not operation can be executed in the first place (can execute/cannot execute).
5) What pissed me off about Resharper is how hard it was to just input a single {. Without the }. And how it then removed both braces when you tried to remove the second one... AFAIR they did something about it in later versions, but I am not sure what (maybe I just got used to it). You seem to mimic this behavior but I would love to see it improved in some way. :) Maybe add a setting, that can toggle this feature (the delete part) on or off.
6) Another thought about DeleteAroundCaret: maybe DELETE should be handled internally by autocomplete classes? I mean if autocomplete is responsible for adding "output", it should also know how to remove it. At least it makes sense to me.
7) VBENativeServices.KeyDown += HandleKeyDown; - I think you need another abstraction layer on top of this event, that would track which keys are used where. From my experience handling key presses in hotkey-heavy apps quickly becomes a nightmare, if you use a single event for that. You should come up with a way to manage hotkeys and detect conflicts. When multiple services want to handle, say, DELETE key in different way, there should be a clear policy in place, that should decide which service should handle this key in the current context, or throw an exception if the hotkey is ambiguous.
-
\$\begingroup\$ Thanks! About the R#-esque deletion: it is only triggered when the caret is between the opening and the closing character, so to have a single
(you would to((and get(|)), then arrow-right (and get()|) and hit delete =) ...feels like the best of both worlds really! \$\endgroup\$Mathieu Guindon– Mathieu Guindon2018年06月19日 11:39:29 +00:00Commented Jun 19, 2018 at 11:39 -
\$\begingroup\$ @MathieuGuindon I double-checked that on my current R# version (which is a bit out of date), and R# removes both braces if you hit backspace when cursor is in the middle. But it only removes last brace, if you hit delete. Which is probably what I do nowadays, when I want a single opening brace. I type subconsciously, so its hard to tell for sure. :) \$\endgroup\$Nikita B– Nikita B2018年06月19日 11:51:49 +00:00Commented Jun 19, 2018 at 11:51
-
\$\begingroup\$ Hmm, I'll probably adjust it to match the R# behavior then. Not sure what Vanilla-VS does though. \$\endgroup\$Mathieu Guindon– Mathieu Guindon2018年06月19日 11:56:11 +00:00Commented Jun 19, 2018 at 11:56
This review isn't definitely worth 200 extra points because I think in order to write a decent one, you have to pretty well understand how this code together with VBA works or even try to run it. I don't so I cannot tell whether the way you do things is the preferred way. I can only scratch the surface and say a few things about the style of the code etc. and this is what I will do...
You use a lot of helper variables which I really like because they explain a lot of things but some if statemens could use them too as they seem to be a little bit magical like
if (e.Key == Keys.Delete || e.Key == Keys.Back || e.Key == Keys.Enter || e.Key == Keys.Tab) {
that could use some HashSet<Key> autoCompleteKeys or
if (e.Keys != Keys.None && selection.LineCount > 1 || selection.StartColumn != selection.EndColumn)
that is even more mysterious.
Of all of them this is my favourite as this is the cleanest one:
var handleDelete = e.Keys == Keys.Delete && selection.EndColumn <= currentContent.Length; var handleBackspace = e.Keys == Keys.Back && selection.StartColumn > 1; foreach (var autoComplete in _autoCompletes.Where(auto => auto.IsEnabled)) { if (handleDelete || handleBackspace)
using (var pane = GetCodePaneFromHwnd(e.Hwnd)) { using (var module = pane.CodeModule) { var args = new AutoCompleteEventArgs(module, e); KeyDown?.Invoke(_vbe, args); e.Handled = args.Handled; } }
You could squeeze together some of the usings by removing the first level of indentation
using (var pane = GetCodePaneFromHwnd(e.Hwnd))
using (var module = pane.CodeModule)
public override int SubClassProc(IntPtr hWnd, IntPtr msg, IntPtr wParam, IntPtr lParam, IntPtr uIdSubclass, IntPtr dwRefData) { KeyPressEventArgs args; switch ((WM)msg) { case WM.CHAR: args = new KeyPressEventArgs(hWnd, wParam, lParam, (char)wParam); OnKeyDown(args); if (args.Handled) { return 0; } break; case WM.KEYDOWN: args = new KeyPressEventArgs(hWnd, wParam, lParam); OnKeyDown(args); if (args.Handled) { return 0; } break; } return base.SubClassProc(hWnd, msg, wParam, lParam, uIdSubclass, dwRefData); }
The only difference between these two cases is the creation of the args. I find you could move this switch into its own method and use returns.
public override int SubClassProc(IntPtr hWnd, IntPtr msg, IntPtr wParam, IntPtr lParam, IntPtr uIdSubclass, IntPtr dwRefData)
{
var args = CreateKeyPressArgs(...);
OnKeyDown(args);
return args.Handled ? 0 : base.SubClassProc(hWnd, msg, wParam, lParam, uIdSubclass, dwRefData);
}
public override int CreateKeyPressArgs(IntPtr hWnd, IntPtr msg, IntPtr wParam, IntPtr lParam, IntPtr uIdSubclass, IntPtr dwRefData)
{
switch ((WM)msg)
{
case WM.CHAR:
return new KeyPressEventArgs(hWnd, wParam, lParam, (char)wParam);
case WM.KEYDOWN:
return new KeyPressEventArgs(hWnd, wParam, lParam);
default:
throw or return null... // to make the compiler happy
}
}
_configService.SettingsChanged += ConfigServiceSettingsChanged; VBENativeServices.KeyDown += HandleKeyDown;
This isn't the standard way of naming event handlers in C#. From what I have learned it should be object_event:
private void AutoCompleteService_SettingsChanged(...)
and
private void VBENativeServices_KeyDown(...)
if (handleDelete || handleBackspace) { if (DeleteAroundCaret(e, autoComplete)) { break; } } else { if (autoComplete.Execute(e, _settings)) { break; } }
This is absolutely my favourite way of writing conditions because I think this is much cleaner and easier to understand than a combination of weird && and || or if/else if/else etc.
if (autoComplete.IsInlineCharCompletion) { var code = e.CurrentLine; // If caret LHS is the AC input token and RHS is the AC output token, we can remove both. // Substring index is 0-based. Selection from code pane is 1-based. // LHS should be at StartColumn - 2, RHS at StartColumn - 1. var caretLHS = code.Substring(Math.Max(0, e.CurrentSelection.StartColumn - 2), 1); var caretRHS = code.Length >= e.CurrentSelection.StartColumn ? code.Substring(e.CurrentSelection.StartColumn - 1, 1) : string.Empty; if (caretLHS == autoComplete.InputToken && caretRHS == autoComplete.OutputToken) { var left = code.Substring(0, e.CurrentSelection.StartColumn - 2); var right = code.Substring(e.CurrentSelection.StartColumn); using (var pane = e.CodeModule.CodePane) { e.CodeModule.ReplaceLine(e.CurrentSelection.StartLine, left + right); pane.Selection = new Selection(e.CurrentSelection.StartLine, e.CurrentSelection.StartColumn - 1); e.Handled = true; } return true; } } return false;
I also like that you use positive conditions rather then early returns that are also much easier to follow rather than negative ones.
But not always. E.g. this part isn't consitent with the previous example:
if (!e.IsCharacter || !IsInlineCharCompletion) { return false; } var module = e.CodeModule; using (var pane = module.CodePane) { var selection = pane.Selection; if (e.Character.ToString() == InputToken) { var code = module.GetLines(selection).Insert(Math.Max(0, selection.StartColumn - 1), InputToken + OutputToken); module.ReplaceLine(selection.StartLine, code); pane.Selection = new Selection(selection.StartLine, selection.StartColumn + 1); e.Handled = true; return true; } return false; }
I think it would be cleaner to flip the conditions to positive ones and build this method similar to the other one, this is:
if (e.IsCharacter && IsInlineCharCompletion)
{
...
}
return false;
var code = module.GetLines(selection).Insert(Math.Max(0, selection.StartColumn - 1), InputToken + OutputToken);
This line inside the Execute methods seems to be doing something very important but it's very well hidden by the calculation inside the Insert. This could be deciphered by using helper variables.
e.Handled = true; return true;
Let's replace it with :)
return (e.Handled = true);
Overall, I think this is a very good quality code that is a pleasure to read. I'm also quite astonished by how you can extend the VBA editor.
-
\$\begingroup\$ About the part that says it's your absolute favorite way of writing conditions: the potentially side-effecting conditions
if(DeleteAroundCaret())andif(autoComplete.Execute())had to be on their own =) \$\endgroup\$Mathieu Guindon– Mathieu Guindon2018年06月19日 11:44:36 +00:00Commented Jun 19, 2018 at 11:44
TwoDim = Array( _ Array(1,2,3), _ Array(4,5,6) _ )might have a hope of aligning nicely? \$\endgroup\$var = iif(test, true result, false result)\$\endgroup\$