Asynchronous Code Inspections

Rubberduck code inspections can take quite a while to complete, so I decided to run them on a background thread instead of locking up the IDE. The only drawback is… the IDE isn’t locked-up while inspections are running, so the user can end up with inspection results that go against stale parse results – this could mean stale inspection results (say you add Option Explicit to a module that didn’t have it), or stale token positioning (say you add or remove lines of code; every inspection result refering to a token located below that will be misplaced when navigating).

But the UX is worth it.. I think.

The status label indicates when parsing is happening…

…and when inspections are running:

All results are populated at once in the grid:

And there’s a distinct status when no issues are found:

Here’s the presenter code:

namespace Rubberduck.UI.CodeInspections
{
    public class CodeInspectionsDockablePresenter : DockablePresenterBase
    {
        private CodeInspectionsWindow Control { get { return UserControl as CodeInspectionsWindow; } }

        private IEnumerable<VBProjectParseResult> _parseResults;
        private IList<ICodeInspectionResult> _results;
        private readonly IInspector _inspector;

        public CodeInspectionsDockablePresenter(IInspector inspector, VBE vbe, AddIn addin, CodeInspectionsWindow window)
            :base(vbe, addin, window)
        {
            _inspector = inspector;
            _inspector.IssuesFound += OnIssuesFound;
            _inspector.Reset += OnReset;
            _inspector.Parsing += OnParsing;
            _inspector.ParseCompleted += OnParseCompleted;

            Control.RefreshCodeInspections += OnRefreshCodeInspections;
            Control.NavigateCodeIssue += OnNavigateCodeIssue;
            Control.QuickFix += OnQuickFix;
            Control.CopyResults += OnCopyResultsToClipboard;
        }

        // indicates that the _parseResults are no longer in sync with the UI
        private bool _needsResync;

        private void OnParseCompleted(object sender, ParseCompletedEventArgs e)
        {
            ToggleParsingStatus(false);
            if (sender == this)
            {
                _needsResync = false;
                _parseResults = e.ParseResults;
                Task.Run(() => RefreshAsync());
            }
            else
            {
                _parseResults = e.ParseResults;
                _needsResync = true;
            }
        }

        private void OnParsing(object sender, EventArgs e)
        {
            ToggleParsingStatus();
            Control.Invoke((MethodInvoker) delegate
            {
                Control.EnableRefresh(false);
            });
        }

        private void ToggleParsingStatus(bool isParsing = true)
        {
            Control.Invoke((MethodInvoker) delegate
            {
                Control.ToggleParsingStatus(isParsing);
            });
        }

        private void OnCopyResultsToClipboard(object sender, EventArgs e)
        {
            var results = string.Join("n", _results.Select(FormatResultForClipboard));
            var text = string.Format("Rubberduck Code Inspections - {0}n{1} issue" + (_results.Count != 1 ? "s" : string.Empty) + " found.n",
                            DateTime.Now, _results.Count) + results;

            Clipboard.SetText(text);
        }

        private string FormatResultForClipboard(ICodeInspectionResult result)
        {
            var module = result.QualifiedSelection.QualifiedName;
            return string.Format(
                "{0}: {1} - {2}.{3}, line {4}",
                result.Severity,
                result.Name,
                module.Project.Name,
                module.Component.Name,
                result.QualifiedSelection.Selection.StartLine);
        }

        private int _issues;
        private void OnIssuesFound(object sender, InspectorIssuesFoundEventArg e)
        {
            Interlocked.Add(ref _issues, e.Issues.Count);
            Control.Invoke((MethodInvoker) delegate
            {
                var newCount = _issues;
                Control.SetIssuesStatus(newCount);
            });
        }

        private void OnQuickFix(object sender, QuickFixEventArgs e)
        {
            e.QuickFix(VBE);
            _needsResync = true;
            OnRefreshCodeInspections(null, EventArgs.Empty);
        }

        public override void Show()
        {
            base.Show();
            Task.Run(() => RefreshAsync());
        }

        private void OnNavigateCodeIssue(object sender, NavigateCodeEventArgs e)
        {
            try
            {
                e.QualifiedName.Component.CodeModule.CodePane.SetSelection(e.Selection);
            }
            catch (COMException)
            {
                // gulp
            }
        }

        private void OnRefreshCodeInspections(object sender, EventArgs e)
        {
            Task.Run(() => RefreshAsync()).ContinueWith(t =>
            {
                Control.SetIssuesStatus(_results.Count, true);
            });
        }

        private async Task RefreshAsync()
        {
            Control.Invoke((MethodInvoker) delegate
            {
                Control.EnableRefresh(false);
                Control.Cursor = Cursors.WaitCursor;
            });

            try
            {
                if (VBE != null)
                {
                    if (_parseResults == null || _needsResync)
                    {
                        _inspector.Parse(VBE, this);
                        return;
                    }

                    var parseResults = _parseResults.SingleOrDefault(p => p.Project == VBE.ActiveVBProject);
                    if (parseResults == null || _needsResync)
                    {
                        _inspector.Parse(VBE, this);
                        return;
                    }

                    _results = await _inspector.FindIssuesAsync(parseResults);

                    Control.Invoke((MethodInvoker) delegate
                    {
                        Control.SetContent(_results.Select(item => new CodeInspectionResultGridViewItem(item))
                            .OrderBy(item => item.Component)
                            .ThenBy(item => item.Line));
                    });
                }
            }
            catch (COMException exception)
            {
                // swallow
            }
            finally
            {
                Control.Invoke((MethodInvoker) delegate
                {
                    Control.Cursor = Cursors.Default;
                    Control.SetIssuesStatus(_issues, true);
                    Control.EnableRefresh();
                });
            }
        }

        private void OnReset(object sender, EventArgs e)
        {
            _issues = 0;
            Control.Invoke((MethodInvoker) delegate
            {
                Control.SetIssuesStatus(_issues);
                Control.InspectionResults.Clear();
            });
        }
    }
}

Here’s the code-behind for the window:

namespace Rubberduck.UI.CodeInspections
{
    public partial class CodeInspectionsWindow : UserControl, IDockableUserControl
    {
        private const string ClassId = "D3B2A683-9856-4246-BDC8-6B0795DC875B";
        string IDockableUserControl.ClassId { get { return ClassId; } }
        string IDockableUserControl.Caption { get { return "Code Inspections"; } }

        public BindingList<CodeInspectionResultGridViewItem> InspectionResults 
        {
            get { return CodeIssuesGridView.DataSource as BindingList<CodeInspectionResultGridViewItem>; }
            set { CodeIssuesGridView.DataSource = value; }
        }

        public CodeInspectionsWindow()
        {
            InitializeComponent();
            RefreshButton.Click += RefreshButtonClicked;
            QuickFixButton.ButtonClick += QuickFixButton_Click;
            GoButton.Click += GoButton_Click;
            PreviousButton.Click += PreviousButton_Click;
            NextButton.Click += NextButton_Click;
            CopyButton.Click += CopyButton_Click;

            var items = new List<CodeInspectionResultGridViewItem>();
            CodeIssuesGridView.SelectionMode = DataGridViewSelectionMode.FullRowSelect;
            CodeIssuesGridView.DataSource = new BindingList<CodeInspectionResultGridViewItem>(items);

            CodeIssuesGridView.AutoResizeColumns();
            CodeIssuesGridView.Columns["Issue"].AutoSizeMode = DataGridViewAutoSizeColumnMode.Fill;

            CodeIssuesGridView.SelectionChanged += CodeIssuesGridView_SelectionChanged;
            CodeIssuesGridView.CellDoubleClick += CodeIssuesGridView_CellDoubleClick;
        }

        public void ToggleParsingStatus(bool enabled = true)
        {
            StatusLabel.Image = enabled
                ? Resources.hourglass
                : Resources.exclamation_diamond;
            StatusLabel.Text = enabled
                ? RubberduckUI.Parsing
                : RubberduckUI.CodeInspections_Inspecting;
        }

        public void SetIssuesStatus(int issueCount, bool completed = false)
        {
            _issueCount = issueCount;
            if (issueCount == 0)
            {
                if (completed)
                {
                    StatusLabel.Image = Resources.tick_circle;
                    StatusLabel.Text = RubberduckUI.CodeInspections_NoIssues;
                }
                else
                {
                    StatusLabel.Image = Resources.hourglass;
                    StatusLabel.Text = RubberduckUI.CodeInspections_Inspecting;
                }
            }
            else
            {
                if (completed)
                {
                    StatusLabel.Image = Resources.exclamation_diamond;
                    StatusLabel.Text = string.Format("{0} issue" + (issueCount != 1 ? "s" : string.Empty), issueCount);
                }
                else
                {
                    StatusLabel.Image = Resources.hourglass;
                    StatusLabel.Text = string.Format("{0} ({1} issue" + (issueCount != 1 ? "s" : string.Empty) + ")", RubberduckUI.CodeInspections_Inspecting, issueCount);
                }
            }
        }

        private int _issueCount;
        public void EnableRefresh(bool enabled = true)
        {
            RefreshButton.Enabled = enabled;
            QuickFixButton.Enabled = enabled && _issueCount > 0;
        }

        public event EventHandler CopyResults;
        private void CopyButton_Click(object sender, EventArgs e)
        {
            var handler = CopyResults;
            if (handler != null)
            {
                handler(this, EventArgs.Empty);
            }
        }

        private void QuickFixButton_Click(object sender, EventArgs e)
        {
            QuickFixItemClick(QuickFixButton.DropDownItems.Cast<ToolStripMenuItem>().First(item => item.Checked), EventArgs.Empty);
        }

        private void PreviousButton_Click(object sender, EventArgs e)
        {
            var previousIssueIndex = (CodeIssuesGridView.SelectedRows[0].Index == 0)
                ? CodeIssuesGridView.Rows.Count - 1
                : CodeIssuesGridView.SelectedRows[0].Index - 1;

            CodeIssuesGridView.Rows[previousIssueIndex].Selected = true;
            var item = CodeIssuesGridView.Rows[previousIssueIndex].DataBoundItem as CodeInspectionResultGridViewItem;
            OnNavigateCodeIssue(item);
        }

        private void NextButton_Click(object sender, EventArgs e)
        {
            if (CodeIssuesGridView.Rows.Count == 0)
            {
                return;
            }

            var nextIssueIndex = (CodeIssuesGridView.SelectedRows[0].Index == CodeIssuesGridView.Rows.Count - 1)
                ? 0
                : CodeIssuesGridView.SelectedRows[0].Index + 1;

            CodeIssuesGridView.Rows[nextIssueIndex].Selected = true;
            var item = CodeIssuesGridView.Rows[nextIssueIndex].DataBoundItem as CodeInspectionResultGridViewItem;
            OnNavigateCodeIssue(item);
        }

        private IDictionary<string, Action<VBE>> _availableQuickFixes;
        private void CodeIssuesGridView_SelectionChanged(object sender, EventArgs e)
        {
            var enableNavigation = (CodeIssuesGridView.SelectedRows.Count != 0);
            NextButton.Enabled = enableNavigation;
            PreviousButton.Enabled = enableNavigation;
            GoButton.Enabled = enableNavigation;
            CopyButton.Enabled = enableNavigation;

            var quickFixMenu = QuickFixButton.DropDownItems;
            if (quickFixMenu.Count > 0)
            {
                foreach (var quickFixButton in quickFixMenu.Cast<ToolStripMenuItem>())
                {
                    quickFixButton.Click -= QuickFixItemClick;
                }
            }

            if (CodeIssuesGridView.SelectedRows.Count > 0)
            {
                var issue = (CodeInspectionResultGridViewItem) CodeIssuesGridView.SelectedRows[0].DataBoundItem;
                _availableQuickFixes = issue.GetInspectionResultItem()
                    .GetQuickFixes();
                var descriptions = _availableQuickFixes.Keys.ToList();

                quickFixMenu.Clear();
                foreach (var caption in descriptions)
                {
                    var item = (ToolStripMenuItem) quickFixMenu.Add(caption);
                    if (quickFixMenu.Count > 0)
                    {
                        item.CheckOnClick = false;
                        item.Checked = quickFixMenu.Count == 1;
                        item.Click += QuickFixItemClick;
                    }
                }
            }

            QuickFixButton.Enabled = QuickFixButton.HasDropDownItems;
        }

        public event EventHandler<QuickFixEventArgs> QuickFix;
        private void QuickFixItemClick(object sender, EventArgs e)
        {
            var quickFixButton = (ToolStripMenuItem)sender;
            if (QuickFix == null)
            {
                return;
            }

            var args = new QuickFixEventArgs(_availableQuickFixes[quickFixButton.Text]);
            QuickFix(this, args);
        }

        public void SetContent(IEnumerable<CodeInspectionResultGridViewItem> inspectionResults)
        {
            var results = inspectionResults.ToList();

            CodeIssuesGridView.DataSource = new BindingList<CodeInspectionResultGridViewItem>(results);
            CodeIssuesGridView.Refresh();
        }

        private void GoButton_Click(object sender, EventArgs e)
        {
            var issue = CodeIssuesGridView.SelectedRows[0].DataBoundItem as CodeInspectionResultGridViewItem;
            OnNavigateCodeIssue(issue);
        }

        public event EventHandler<NavigateCodeEventArgs> NavigateCodeIssue;
        private void CodeIssuesGridView_CellDoubleClick(object sender, DataGridViewCellEventArgs e)
        {
            if (e.RowIndex < 0)
            {
                return;
            }
            var issue = CodeIssuesGridView.Rows[e.RowIndex].DataBoundItem as CodeInspectionResultGridViewItem;
            OnNavigateCodeIssue(issue);
        }

        private void OnNavigateCodeIssue(CodeInspectionResultGridViewItem item)
        {
            var handler = NavigateCodeIssue;
            if (handler == null)
            {
                return;
            }

            var result = item.GetInspectionResultItem();
            handler(this, new NavigateCodeEventArgs(result.QualifiedSelection));
        }

        public event EventHandler RefreshCodeInspections;
        private void RefreshButtonClicked(object sender, EventArgs e)
        {
            var handler = RefreshCodeInspections;
            if (handler == null)
            {
                return;
            }

            toolStrip1.Refresh();

            handler(this, EventArgs.Empty);
        }
    }
}

The thing works… but something’s definitely off – RefreshAsync is running more often than it should.

Your event handler naming confused me at first. The standard naming convention is that OnEventName raises the event. And the standard handler looks like ObjectName_EventName.

public class ClassWithEvent : IDisposable
{
    public event EventHandler SomeEvent;
    protected virtual void OnSomeEvent()
    {
        var e = SomeEvent;
        if (e != null)
            e(this, EventArgs.Empty);
    }

    public void Dispose()
    {
        SomeEvent = null;
    }
}

public class ClassListensToEvent
{
    private ClassWithEvent myClass;

    public ClassListensToEvent()
    {
        myClass.SomeEvent += myClass_SomeEvent;
    }

    private void myClass_SomeEvent(object sender, EventArgs eventArgs)
    {
    }
}

I would do some heavy refactoring to RefreshAsync and how it’s used. I don’t completely understand your workflow but I would expect that RefreshAsync could be cancelled. No point in refreshing with out of date results. I would use a CancellationTokenSource and pass a CancellationToken to both RefreshAsync and Task.Run like this:

Task.Run(() => RefreshAsync(token), token);

If you get updated results the first thing you would do is call CancellationTokenSource.Cancel() to stop any existing refresh. Inside RefreshAsync you can catch and swallow TaskCancelledException gracefully.

Finally, RefreshAsync is not very asynchronous. You’re using Invoke several times which is blocking on the UI thread. Ideally RefreshAsync would be asynchronous until completion and then update the GUI once at the end (via BeginInvoke).

With these changes you can cancel RefreshAsync so you’re not wasting cycles on old data or painting old results on the GUI.

Just a few quick shoots..

To reduce the small code duplication here

private void OnParseCompleted(object sender, ParseCompletedEventArgs e)
{
    ToggleParsingStatus(false);
    if (sender == this)
    {
        _needsResync = false;
        _parseResults = e.ParseResults;
        Task.Run(() => RefreshAsync());
    }
    else
    {
        _parseResults = e.ParseResults;
        _needsResync = true;
    }
}  

you can refactor this to

private void OnParseCompleted(object sender, ParseCompletedEventArgs e)
{
    ToggleParsingStatus(false);
    _parseResults = e.ParseResults;
    _needsResync = sender != this
    if (!needsResync)
    {
        Task.Run(() => RefreshAsync());
    }
}

Inside the private async Task RefreshAsync() method you have some more “problems”. First you should reduce the horizontal spacing by using a guard clause on if (VBE == null) and return early.

The next problem is here

 var parseResults = _parseResults.SingleOrDefault(p => p.Project == VBE.ActiveVBProject);  

can you see it ?? A call to SingleOrDefault() will based on the name return either a single item or a default item. A single item != plural ! So rename the local variable to parseResult.

At a closer look at these two if statements

    if (_parseResults == null || _needsResync)
    {
        _inspector.Parse(VBE, this);
        return;
    }

    var parseResults = _parseResults.SingleOrDefault(p => p.Project == VBE.ActiveVBProject);
    if (parseResults == null || _needsResync)
    {
        _inspector.Parse(VBE, this);
        return;
    }  

we see that at the second if condition we can omit the || _needsResync because this won’t ever be true.

Just seen the nice way how you write tenary expressions.

Small comment, I would refactor OnCopyResultsToClipboard like this:

    private void OnCopyResultsToClipboard(object sender, EventArgs e)
    {
        var results = string.Join(Environment.NewLine, _results.Select(FormatResultForClipboard));
        var plural = _results.Count != 1 ? "s" : string.Empty;
        var text = string.Format("Rubberduck Code Inspections - {0}{3}{1} issue{2} found.{3} {4}",
                        DateTime.Now, _results.Count, plural, Environment.NewLine, results);

        Clipboard.SetText(text);
    }

You were using string concatenations alongside string format which struck me as odd.