Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ComCommand classes #4833

Merged
merged 49 commits into from Aug 29, 2019

Conversation

@bclothier
Copy link
Contributor

commented Mar 1, 2019

Closes #4269

The PR means to help address concerns identified from the issues #4767 and or #4757 -- the trend being that we are trying to access COM after the shutdown has started. Those issues are actually with the code explorer but the assumption is that it need not be just the code explorer; any subsystems that does COM access needs to be prevented once the shutdown has started.

For a while we've had VbeEvents which provides a EventsTerminated event to broadcast the shutdown but it has been unused... up to now.

This first stage of work focuses on controlling the commands. This required 2 changes:

  1. Because the commands are transient, it does no good to have them listening to an event which may have already happened. Thus, VbeEvents now exposes a property Terminated to ensures that any new instances created after the shutdown inadvertently will be disabled accordingly. Accordingly, the VbeEvents has been cleaned up a bit. Note that I opted to use Interlocked to provide very basic locking. I considered a more elaborate scheme but decided it was best to first demonstrate that there is in fact a need before going to the trouble.

  2. Commands that touches a COM object now derive from the ComCommandBase abstract class. The main difference between the ComCommandBase and CommandBase is simply that ComCommandBase takes IVBEEvents as a dependency and wraps checks around the CanExecute and Execute which prevents the commands from being used once we are shutting down.

  3. As part of refactoring, the tests has been updated in accordance with #4489 - namely the numbers of news has been cut down and converted into a set of overloaded static factory methods.

TODO:

  1. Update the commands outside the UI\Command (e.g. AutoComplete, CodeExplorer and UnitTesting -- there is also Refactoring but the refactorings will be skipped over at MDoerner's request).

  2. Add more unit tests? More live testing

NOTE:

There are subsystems that will need similar treatments. A good example is the test engine which performs several COM accesses. Such subsystem will need its own implementation of something to cut off the access after the shutdown. It is more likely that those will be addressed in separate PRs. For now, this PR deals with the commands.

Addendum: The PR also addresses the issues with the ShowIntellisenseCommand ShowQuickInfoCommand as identified in #4110, suppressing the beep message when executing. Thus we now have a working implementation. Furthermore, it has been tested in VB6 as well as VBA, and verified to be working in both environment so AutoCompletion is now able to trigger the quick info without annoying the users.

bclothier added 22 commits Feb 26, 2019
…(it originally was one file but got accidentally split; while we want one class per file, it did not feel clean to have helpers interwined in the folder, nor to make yet another subfolder, so kicking it down the road for now)
…commands are transient and could get created after the terminate event has been broadcast. Therefore we will use a Terminate property to block execution in the commands. Also set up tests for the ComCommandBase & VbeEvents.
bclothier added 2 commits Mar 8, 2019
…d, add the EasyHook for the VBA beep and message pump to dispatch it in order to suppress the annoying beep when QuickInfo is not available. Update the tests.
…ad of CommandBase; though not all commands technically interact with COM, this seemed the preferable given the diamond problem and also the fact that the code explorer is by nature interactive, so should not be functional once we're shutting down.
bclothier added 2 commits Jul 4, 2019
…nto ComDisconnect

Generally keep the changes from the branch for commands (e.g. ComCommands change, while taking changes from the remote branch's head for other files)

# Conflicts:
#	Rubberduck.CodeAnalysis/Inspections/Concrete/ApplicationWorksheetFunctionInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyCaseBlockInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyDoWhileBlockInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyElseBlockInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyForEachBlockInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyForLoopBlockInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyIfBlockInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyWhileWendBlockInspection.cs
#	Rubberduck.CodeAnalysis/Inspections/Inspector.cs
#	Rubberduck.Core/AutoComplete/ShowIntelliSenseCommand.cs
#	Rubberduck.Core/Properties/Settings.Designer.cs
#	Rubberduck.Core/Properties/Settings.settings
#	Rubberduck.Core/Rubberduck.Core.csproj
#	Rubberduck.Core/Settings/ReferenceConfigProvider.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddClassModuleCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddComponentCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddMDIFormCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddPropertyPageCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddStdModuleCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddTemplateCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddTestComponentCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddUserControlCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddUserDocumentCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddUserFormCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/AddVBFormCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/CodeExplorerCommandBase.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs
#	Rubberduck.Core/UI/CodeExplorer/Commands/RenameCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/AddRemoveReferencesCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/CodeExplorerCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/CodeMetricsCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/ExportAllCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/FindAllImplementationsCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/FindAllReferencesCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/FindSymbolCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/IndentCurrentModuleCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/IndentCurrentProcedureCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/IndentCurrentProjectCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/InspectionResultsCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/NoIndentAnnotationCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/ReparseCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/ShowParserErrorsCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/SyncCodeExplorerCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/TestExplorerCommand.cs
#	Rubberduck.Core/UI/Command/ComCommands/ToDoExplorerCommand.cs
#	Rubberduck.Core/UI/Command/FormDesignerFindAllReferencesCommand.cs
#	Rubberduck.Core/UI/Command/RefreshCommand .cs
#	Rubberduck.Core/UI/Refactorings/ReorderParameters/ReorderParametersViewModel.cs
#	Rubberduck.Core/UI/Settings/GeneralSettingsViewModel.cs
#	Rubberduck.Core/UI/ToDoItems/ToDoExplorerViewModel.cs
#	Rubberduck.Core/UI/UnitTesting/ComCommands/AddTestMethodCommand.cs
#	Rubberduck.Core/UI/UnitTesting/ComCommands/AddTestMethodExpectedErrorCommand.cs
#	Rubberduck.Core/UI/UnitTesting/ComCommands/AddTestModuleCommand.cs
#	Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs
#	Rubberduck.Resources/Inspections/InspectionInfo.cs.resx
#	Rubberduck.Resources/Inspections/InspectionInfo.de.resx
#	Rubberduck.Resources/Inspections/InspectionInfo.fr.resx
#	Rubberduck.Resources/Inspections/InspectionNames.Designer.cs
#	Rubberduck.Resources/Inspections/InspectionNames.cs.resx
#	Rubberduck.Resources/Inspections/InspectionNames.resx
#	Rubberduck.Resources/Inspections/InspectionResults.cs.resx
#	Rubberduck.Resources/Inspections/InspectionResults.de.resx
#	Rubberduck.Resources/Inspections/InspectionResults.fr.resx
#	Rubberduck.Resources/RubberduckUI.Designer.cs
#	Rubberduck.Resources/RubberduckUI.cs.resx
#	Rubberduck.Resources/RubberduckUI.resx
#	Rubberduck.Resources/Templates.cs.resx
#	Rubberduck.VBEEditor/SourceCodeHandling/SourceFileHandlerComponentSourceCodeHandlerAdapter.cs
#	Rubberduck.VBEEditor/Utility/AddComponentService.cs
#	RubberduckTests/CodeExplorer/MockedCodeExplorer.cs
#	RubberduckTests/TodoExplorer/TodoExplorerTests.cs
@bclothier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Apologies for the long delay. The changes requested should be incorporated. There were a number of tests that I had to revise or ignore due to the fact that the test setups were directly invoking the Execute method, which now indirectly invokes the CanExecute method. As such, it is not valid in all test setups (e.g. some test just passed in null parameter where the CanExecute prevents that). There should be no more changes needed to the PR but review will be appreciated to ensure everything is sane.

bclothier added 4 commits Jul 4, 2019
…urning false because resolver isn't showing the class as an interface. Thus, messagebox cannot be shown.
…handling the case where we want to use active VB Project)
Copy link
Contributor

left a comment

I am not really a fan of reexecuting any full CanExecute method on command execution and would like to propose the alternative approach outlined in my comment.

However, the main point why I request changes is the change in VbeEvents not to reset the singleton on termination. This makes it single-use, which does not seem necessary and is potentially very harmful.

{
declaration = node.Declaration;
}
else if (parameter is Declaration d)

This comment has been minimized.

Copy link
@MDoerner

MDoerner Aug 2, 2019

Contributor

Can we please turn this into a switch statent on the type of parameter (pulling the applicable nodes check into the first branch)?

Rubberduck.Core/UI/Command/CommandBase.cs Outdated Show resolved Hide resolved
Rubberduck.VBEEditor/Events/VbeEvents.cs Show resolved Hide resolved
bclothier added 4 commits Aug 6, 2019
…nto ComDisconnect

# Conflicts:
#	RubberduckTests/CodeExplorer/MockedCodeExplorer.cs
…executing the conditions on the OnExecute path by having a separate collection rather than re-using the whole collection of functions for CanExecute.
…ause statics do not die at addin's end. Instead, we keep the _terminated variable as a static variable which is reset internally to communicate the state.
Copy link
Contributor

left a comment

I am OK with the new design. However, personally, I think that one should be able to set the inherited CanExecute condition and the inherited OnExecute check independently. Moreover, I do not see why the design around their initialization should be different.

In addition, I found a number of deviations from the usual style in the code base. These look like changes from an automatic refactoring with a suboptimal automatic formatting.

bclothier added 3 commits Aug 29, 2019
…nto ComDisconnect

# Conflicts:
#	Rubberduck.CodeAnalysis/Rubberduck.CodeAnalysis.xml
#	Rubberduck.Core/UI/Command/ComCommands/ReparseCommand.cs
…. The changes now has all parameters on their own line, with the base in its own line, as well.
@retailcoder retailcoder merged commit 86b22cb into rubberduck-vba:next Aug 29, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.