FindAllReferencesCommand 2.0

Question

Rubberduck 2.0 will feature a "Search Results" dockable toolwindow that will be used for displaying the results of Find all references, Find all implementations, and whatever else we need to find. It replaces the ugly 1.x "SimpleList" view:

It all starts with a command. Here, the FindAllReferencesCommand:

/// <summary>
/// A command that locates all references to a specified identifier, or of the active code module.
/// </summary>
[ComVisible(false)]
public class FindAllReferencesCommand : CommandBase
{
    private readonly INavigateCommand _navigateCommand;
    private readonly RubberduckParserState _state;
    private readonly IActiveCodePaneEditor _editor;
    private readonly ISearchResultsWindowViewModel _viewModel;
    private readonly SearchResultPresenterInstanceManager _presenterService;

    public FindAllReferencesCommand(INavigateCommand navigateCommand, RubberduckParserState state, IActiveCodePaneEditor editor, ISearchResultsWindowViewModel viewModel, SearchResultPresenterInstanceManager presenterService)
    {
        _navigateCommand = navigateCommand;
        _state = state;
        _editor = editor;
        _viewModel = viewModel;
        _presenterService = presenterService;
    }

    public override void Execute(object parameter)
    {
        if (_state.Status != ParserState.Ready)
        {
            return;
        }

        var declaration = FindTarget(parameter);
        if (declaration == null)
        {
            return;
        }

        var viewModel = CreateViewModel(declaration);
        _viewModel.AddTab(viewModel);
        _viewModel.SelectedTab = viewModel;

        try
        {
            var presenter = _presenterService.Presenter(_viewModel);
            presenter.Show();
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
        }
    }

    private SearchResultsViewModel CreateViewModel(Declaration declaration)
    {
        var results = declaration.References.Select(reference =>
            new SearchResultItem(
                reference.QualifiedModuleName.QualifyMemberName(reference.ParentScope.Split('.').Last()),
                reference.Selection,
                reference.Context.GetText()));

        var viewModel = new SearchResultsViewModel(_navigateCommand,
            string.Format(RubberduckUI.SearchResults_AllReferencesTabFormat, declaration.IdentifierName), results);

        return viewModel;
    }

    private Declaration FindTarget(object parameter)
    {
        var declaration = parameter as Declaration;
        if (declaration == null)
        {
            var selection = _editor.GetSelection();
            if (selection != null)
            {
                declaration = _state.AllUserDeclarations
                    .SingleOrDefault(item => item.QualifiedName.QualifiedModuleName == selection.Value.QualifiedName 
                        && (item.QualifiedSelection.Selection.ContainsFirstCharacter(selection.Value.Selection)
                            || 
                            item.References.Any(reference => reference.Selection.ContainsFirstCharacter(selection.Value.Selection))));
            }

            if (declaration == null)
            {
                return null;
            }
        }
        return declaration;
    }
}

That weird "Instance Manager" class is a hack/work-around to ensure the dockable presenter (which interacts with COM) is always in a stable state; Ninject is configured to inject it in SingletonScope; same with ISearchResultsWindowViewModel - that way every feature gets the same window ViewModel and "Find all references" adds tabs to the same window as "Find all implementations".

It's also responsible for hiding the docked presenter when the last tab gets closed:

/// <summary>
/// A "disposable singleton" factory that creates/returns the same instance to all clients.
/// </summary>
public class SearchResultPresenterInstanceManager : IDisposable
{
    private readonly VBE _vbe;
    private readonly AddIn _addin;
    private SearchResultWindow _view;

    public SearchResultPresenterInstanceManager(VBE vbe, AddIn addin)
    {
        _vbe = vbe;
        _addin = addin;
        _view = new SearchResultWindow();
    }

    private SearchResultsDockablePresenter _presenter;
    public SearchResultsDockablePresenter Presenter(ISearchResultsWindowViewModel viewModel)
    {
        if (_presenter == null || _presenter.IsDisposed)
        {
            if (_view.ViewModel == null)
            {
                _view.ViewModel = viewModel;
                _view.ViewModel.LastTabClosed += viewModel_LastTabClosed;
            }
            _presenter = new SearchResultsDockablePresenter(_vbe, _addin, _view);
        }

        return _presenter;
    }

    private void viewModel_LastTabClosed(object sender, EventArgs e)
    {
        _presenter.Hide();
    }

    public void Dispose()
    {
        _view.ViewModel.LastTabClosed -= viewModel_LastTabClosed;
        _presenter.Dispose();
    }
}

The window ViewModel exposes and manages a collection of tabs:

public class SearchResultsWindowViewModel : ViewModelBase, ISearchResultsWindowViewModel
{
    private readonly ObservableCollection<SearchResultsViewModel> _tabs = 
        new ObservableCollection<SearchResultsViewModel>();

    public void AddTab(SearchResultsViewModel viewModel)
    {
        viewModel.Close += viewModel_Close;
        _tabs.Add(viewModel);
    }

    void viewModel_Close(object sender, EventArgs e)
    {
        RemoveTab(sender as SearchResultsViewModel);
    }

    public IEnumerable<SearchResultsViewModel> Tabs { get { return _tabs; } }

    private SearchResultsViewModel _selectedTab;

    public SearchResultsViewModel SelectedTab
    {
        get { return _selectedTab; }
        set
        {
            if (_selectedTab != value)
            {
                _selectedTab = value;
                OnPropertyChanged();
            }
        }
    }

    private void RemoveTab(SearchResultsViewModel viewModel)
    {
        if (viewModel != null)
        {
            _tabs.Remove(viewModel);
        }

        if (!_tabs.Any())
        {
            OnLastTabClosed();
        }
    }

    public event EventHandler LastTabClosed;
    private void OnLastTabClosed()
    {
        var handler = LastTabClosed;
        if (handler != null)
        {
            handler.Invoke(this, EventArgs.Empty);
        }
    }
}

The SearchResultsViewModel represents a single tab, so it manages a collection of search results:

public class SearchResultsViewModel : ViewModelBase, INavigateSelection
{
    private readonly INavigateCommand _navigateCommand;
    private readonly string _header;

    public SearchResultsViewModel(INavigateCommand navigateCommand, string header, IEnumerable<SearchResultItem> searchResults)
    {
        _navigateCommand = navigateCommand;
        _header = header;
        _searchResults = new ObservableCollection<SearchResultItem>(searchResults);
        _searchResultsSource = new CollectionViewSource();
        _searchResultsSource.Source = _searchResults;
        _searchResultsSource.GroupDescriptions.Add(new PropertyGroupDescription("QualifiedMemberName.QualifiedModuleName.Name"));
        _searchResultsSource.SortDescriptions.Add(new SortDescription("QualifiedMemberName.QualifiedModuleName.Name", ListSortDirection.Ascending));
        _closeCommand = new DelegateCommand(ExecuteCloseCommand);
    }

    private readonly ObservableCollection<SearchResultItem> _searchResults;
    public ObservableCollection<SearchResultItem> SearchResults { get { return _searchResults; } }

    private readonly CollectionViewSource _searchResultsSource;
    public CollectionViewSource SearchResultsSource { get { return _searchResultsSource; } }

    public string Header { get { return _header; } }

    private readonly ICommand _closeCommand;
    public ICommand CloseCommand { get { return _closeCommand; } }

    private SearchResultItem _selectedItem;
    public SearchResultItem SelectedItem
    {
        get { return _selectedItem; }
        set
        {
            if (_selectedItem != value)
            {
                _selectedItem = value;
                OnPropertyChanged();
            }
        }
    }

    private void ExecuteCloseCommand(object parameter)
    {
        OnClose();
    }

    public event EventHandler Close;
    private void OnClose()
    {
        var handler = Close;
        if (handler != null)
        {
            handler.Invoke(this, EventArgs.Empty);
        }
    }

    public INavigateCommand NavigateCommand { get { return _navigateCommand; } }
    INavigateSource INavigateSelection.SelectedItem { get { return SelectedItem; } }
}

Lastly, the SearchResultItem represents a single search result. It implements INavigateSource, which supplies a NavigateCodeEventArgs, which the NavigateCommand uses to double-click navigate to the code pane.

public class SearchResultItem : ViewModelBase, INavigateSource
{
    private readonly NavigateCodeEventArgs _navigateArgs;
    private readonly QualifiedMemberName _member;
    private readonly Selection _selection;
    private string _resultText;

    public SearchResultItem(QualifiedMemberName member, Selection selection, string resultText)
    {
        _navigateArgs = new NavigateCodeEventArgs(member.QualifiedModuleName, selection);
        _member = member;
        _selection = selection;
        _resultText = resultText;
    }

    public QualifiedMemberName QualifiedMemberName { get { return _member; }}
    public Selection Selection { get { return _selection; } }

    public string ResultText
    {
        get { return _resultText; }
        set
        {
            if (_resultText != value)
            {
                _resultText = value;
                OnPropertyChanged();
            }
        }
    }

    public NavigateCodeEventArgs GetNavigationArgs()
    {
        return _navigateArgs;
    }
}

---

I don't like exposing the CollectionViewSource like this in my ViewModel - I'd much rather have an all-XAML solution.. but I couldn't get that to work so I resorted to Plan B.

I've not yet completely made my mind up about which columns I'm going to show in the GridView, that's why there are unused public members in SearchResultItem.

Any/all feedback and improvements are welcome.

Answer 1

If FindAllReferencesCommand is ICommand implementation, then shouldn't simple checks such as

if (_state.Status != ParserState.Ready)
{
    return;
}

go into CanExecute portion of a command?

---

I'm not sure I like the idea of creating viewmodel in a command and then passing it to presenter. First, I feel like it should be presenter's job to create viewmodel and manage its lifetime. Second, the current implementation has really confusing semantics. Consider this:

var p1 = _presenterService.Presenter(new SearchResultsViewModel());
var p2 = _presenterService.Presenter(new SearchResultsViewModel());

What should happen? Well, it's hard to tell. First guess that comes to mind: there are now two different presenters with different views and different viewmodels. Second guess: there is a single presenter, and each call of Presenter method simply replaces the DataContext of a view. But both of those guesses are incorrect, and I would have never guessed correctly what actually happens.

---

CreatePresenter() instead of Presenter()? It might be a matter of taste, but I like it when there is a verb in method name.

---

I'm not a fan of having ViewModel property on my views. One of the advantages of MVVM is that it allows you to have your view and viewmodel loosely coupled. By declaring a strongly typed ViewModel property on view you lose this advantage. I mean can't you just store the viewmodel reference as a field in your presenter?

---

You have a lot of nested if's which can be inverted to reduce nesting or even removed completely, such as this one:

    if (declaration == null)
    {
        return null;
    }
}
return declaration;

Removing the first three lines does not change anything.

---

There are also a lot of returns, but very little explanation to the user why the command returned early. Is this handled somewhere else? I mean if, say, the selection is null (which I assume is the text user selected) and your command returns early without doing anything, how does the user know why it doesn't work? If such situation should be impossible you should throw an exception. If it is possible, you should provide some sort of feedback for the sake of good UX.

---

I think you forgot to unsubscribe from viewModel.Close += viewModel_Close;. Better safe than sorry.

You should also consider renaming the handler, so it does not look so out of place in otherwise conventional code. OnTabClosed is a better name.

---

You should have an extension method for that:

var handler = LastTabClosed;
if (handler != null)
{
    handler.Invoke(this, EventArgs.Empty);
}

I also tend to use Action instead of EventHandler whenever I need neither sender, nor arguments. Yes, someone, somewhere on MSDN says that you should always use EventHandler for events blah-blah, but unless the event is exposed as part of public API I see no reason why you should be dragging those empty arguments around.

---

P.S. I'll be honest, this is not the first time I review a code marked with rubberduck tag, but I still have no clue what rubberduck (or VBA) is. But oh well, perhaps one day... Hopefully MVVM is the same everywhere you go. :)