Plumb through should activate#45641
Conversation
|
@davidwengier looks like the nav bar integration tests implicitly relied on the olde behavior? |
No, this change broke the nav bar. The |
…ion to match existing behaviour
dibarbet
left a comment
There was a problem hiding this comment.
seems good to me, just one question on behavior
| var state = __VSNEWDOCUMENTSTATE.NDS_Provisional; | ||
| var state = options.GetOption(NavigationOptions.PreferProvisionalTab) | ||
| ? __VSNEWDOCUMENTSTATE.NDS_Provisional | ||
| : __VSNEWDOCUMENTSTATE.NDS_Permanent; |
There was a problem hiding this comment.
this seems like a slight change in behavior, but I'm not sure it matters
Previously, it seems like permanent tabs will always be activated (or not not activated), but now we can have a permanent tab created but not activated. Does that case ever exist?
There was a problem hiding this comment.
It's definitely a change, the reasoning is simply to copy the platform code here, and because my reading of the documentation doesn't tie the NoActivate flag to provisional in any way.
I don't know if there is a scenario now where permanent tabs should be not activated, but it's possible something, now or in the future, passes a TableEntryNavigateEventArgs with isPreview and shouldActivate both set to false.
There was a problem hiding this comment.
This aligns with my understand of how we should ingest the shouldActivate from the platform. Before we only cared about activation for provisional because there were no cases where we wouldn't activate. The platform has changed that assumption though, and we're going to honor that flag. It is a little odd that they tell us what they expect us to tell them, but there's more nuances there.
|
@ryzngard Looks like you added the |
The platform added a
ShouldActivateflag in 16.5 but we weren't honouring it.Fixes AB#1048288
Because this option maps to the
DoNotActivateflag in VS, setting the option to true doesn't actually do anything, but setting it to false aggressively blocks activation, so I've changed the default for the option to true. This means by default navigation is left to VS, and turning this option off will set theDoNotActivateflag.I've tested all of the places where
IDocumentNavigationServiceis used and all functioned as I would expect. Call Hierarchy was the odd one out that changed behaviour so I've also modified that.