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

Introducing 'Peek Definition' commands #5751

Merged
merged 17 commits into from
May 8, 2021

Conversation

retailcoder
Copy link
Member

@retailcoder retailcoder commented Apr 28, 2021

Adds a Peek Definition command to the Code Explorer, code pane, and VBE Project Explorer context menus, and introduces a method in ISelectedDeclarationProvider to get the SelectedProjectExplorerModule; derived a PE-specific Find all References command from the existing one, overriding OnExecute to explicitly get the its target from the PE. This fixes a glitch with PE navigation commands where the command would acquire its target from the active code pane.

Opportunistically, also fixes a bug that looks like it went under the radar: derived commands' SpecialEvaluateCanExecute logic wouldn't be invoked because the method wasn't virtual and merely being shadowed, causing only the base method to run (and systematically return true). This PR makes the derived methods proper override that do get invoked. edit; nope, this was actually a misguided refactoring that was undone in the PeekDefinitionCommand but forgotten in the other ones

Copy link
Contributor

@bclothier bclothier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea! I do not think we should be changing the can execute conditions to a virtual method, however because we do want to preserve the hierarchy of parent's condition before checking the conditions required by the child. virtual destroys that hierarchy.

Copy link
Contributor

@bclothier bclothier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns about the protected usages. I think those should be simply private as well. I'm fine with renaming of EvaluateCanExecute but as a note, I believe the reason SpecialEvaluateCanExecute was used to avoid confusion with some EvalulateCanExecute method that already exists in the base.

@retailcoder
Copy link
Member Author

The ESC input binding isn't working, it may need to go on the actual parent Popup control. Will fool around and find out =)

@retailcoder
Copy link
Member Author

Unless there are further review comments, this feature is ready to ship.

@retailcoder retailcoder merged commit 18f9597 into rubberduck-vba:next May 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants