Skip to content

Don't handle TextView so opening results from find works#4297

Merged
davidwengier merged 2 commits into
dotnet:masterfrom
davidwengier:DontOpenAppDesignerFromFind
Nov 26, 2018
Merged

Don't handle TextView so opening results from find works#4297
davidwengier merged 2 commits into
dotnet:masterfrom
davidwengier:DontOpenAppDesignerFromFind

Conversation

@davidwengier
Copy link
Copy Markdown
Member

@davidwengier davidwengier commented Nov 21, 2018

Our app designer wasn't using the Designer logical view properly which means that when find results were being opened, if the app designer was open, the shell thought it was a valid window to show instead of opening a window with the text view. This fixes that.

This goes with a CPS pull request (https://devdiv.visualstudio.com/DevDiv/_git/CPS/pullrequest/152382) to open the designer with the write logical view. If these changes are inserted without the CPS changes then the app designer will not open from the Properties context menu. It will still open from the Debug -> Properties menu item, but still worth holding off on merging this PR until CPS is ready.
Turns out the CPS change wasn't necessary, I was just incorrectly not handling the Primary logical view. Fixing that means everything works just fine, including legacy projects.

Fixes #4070

@davidwengier davidwengier requested a review from a team as a code owner November 21, 2018 00:09
Copy link
Copy Markdown
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

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

:shipit:

If rguidLogicalView = VSConstants.LOGVIEWID.TextView_guid OrElse rguidLogicalView = VSConstants.LOGVIEWID.Primary_guid Then
Return NativeMethods.E_NOTIMPL
Else
pbstrPhysicalView = ""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why empty? What does this mean Vs. null?

@davkean
Copy link
Copy Markdown
Member

davkean commented Nov 21, 2018

As discussed:

  • I think this will break legacy, it uses Primary
  • There's descrepency between what you are registering as the logic view of the editor factory and between what you are handling in MapLogicalView. These need to be the same.

@davidwengier
Copy link
Copy Markdown
Member Author

davidwengier commented Nov 22, 2018

I think this will break legacy, it uses Primary

Fixed.

There's descrepency between what you are registering as the logic view of the editor factory and between what you are handling in MapLogicalView. These need to be the same.

That seems to not be the case, though I certainly don't have the context as to why. The Xml editor, which the project file editor wraps, does the same thing. It returns empty strings and nulls in MapLogicalView but its pkgdef contains "standard" descriptions.

@davkean
Copy link
Copy Markdown
Member

davkean commented Nov 22, 2018

I'll step back, what does the following do and why "Design"?

[$RootKey$\Editors\{04b8ab82-a572-4fef-95ce-5222444b6b64}\LogicalViews]
"{7651a702-06e5-11d1-8ebd-00a0c90f26ea}"="Design"

Maybe we need to pull in something that knows how this stuff is supposed to work and can tell us what to do.

@davidwengier
Copy link
Copy Markdown
Member Author

Happy for anyone with expertise to take a look, I'm certainly not claiming that :)

As to what that does, it doesn't do anything at the moment. I originally added it when I was (incorrectly) returning E_NOTIMPL for the primary view because when that happens the system falls back to using the registry (this calls into our editor factory and if that returns E_NOTIMPL it calls GetPhysicalView which reads the registry here). Now that we correctly support primary views (by returning S_OK) this could be removed but I left it assuming it was best practice to list the views that are supported. See this doco, which also mentions that the values are irrelevant (though also says they should be empty strings, which doesn't seem to be the done thing from looking around).

Given our discussion this morning with the Menus stuff, perhaps this is another case of me assuming something is important when it actually doesn't matter at all. ¯\_(ツ)_/¯
Happy to remove it if you think its cleaner.

@davidwengier davidwengier changed the title Properly register the app designer as a designer, so we can open the project file from find results Don't handle TextView so opening results from find works Nov 22, 2018
@davidwengier davidwengier merged commit df940c8 into dotnet:master Nov 26, 2018
@davidwengier davidwengier deleted the DontOpenAppDesignerFromFind branch April 4, 2019 05:49
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.

Figure out how AppDesigner and Project File editor play with each other to unblock Find when the former is opened

4 participants