Fix #36060: Group Completion Providers using IsExclusive#36088
Fix #36060: Group Completion Providers using IsExclusive#36088jasonmalinowski merged 1 commit intodotnet:masterfrom
Conversation
|
Seems reasonable to me. tagging @dpoeschl @ivanbasov see linked conversation for more details (including my thoughts on why i think this is probably the best of the potential options). |
7cc1de7 to
d99c4e4
Compare
|
|
d99c4e4 to
0f1cc0e
Compare
ivanbasov
left a comment
There was a problem hiding this comment.
Looks good. Thank you!
|
Note: if we go this path, we'd certainly benefit from tests. We should also update our docs to explain what's going on here. But the design is 100% ok with me personally. |
|
I’ve added a fairly simple, happy path, test. Have you got any suggestions on how else I could test this? Happy to also update the docs if needed, TBH. |
| Private Shared s_providers As ImmutableArray(Of CompletionProvider) = ImmutableArray.Create(Of CompletionProvider)( | ||
| New TestCompletionProviderWithMockExclusivity(False, CompletionServiceTests_Exclusivity.CompletionItemNonExclusive, 1), | ||
| New TestCompletionProviderWithMockExclusivity(True, CompletionServiceTests_Exclusivity.CompletionItemExclusive, 2), | ||
| New TestCompletionProviderWithMockExclusivity(True, CompletionServiceTests_Exclusivity.CompletionItemExclusive, 3)) |
There was a problem hiding this comment.
thanks for the test!
The test is satisfactory to me :)
Please do. Right now this is in conflict with the specified text. -- @ivanbasov i'm not sure if the IDE has a compat meeting. But this likely shoudl go through that (or a design meeting) before merging, just to make sure everyone is ok. I doubt there is an issue. But as it is a chnage of design in a public API, we should be conscientious here. |
Thank you, @CyrusNajmabadi ! I would say that the change is a fix for a corner case bug. The original design missed the case when more than 1 provider declared itself exclusive. Taking the first one was just a temporarily solution. I understand and support the update to the design claiming that all providers declared themselves exclusive should be considered at the same level of exclusiveness. If we would see that one providers may be more exclusive than other exclusive providers, we would re-consider this. Adding @jasonmalinowski, @sharwell and @jinujoseph to see if they want to consider this change on a design meeting. |
|
Thank you for considering this! I just want to chime in and vote for this - without it, the user experience in VS Mac will regress in the Xamarin iOS workload with the new editor that we have ported from VS Windows, since we cannot feasibly shoehorn in protocol "overrides" at the editor level anymore. |
jasonmalinowski
left a comment
There was a problem hiding this comment.
Design seems fine by me. It seems the only way this would break an experience is:
- We had two providers all along saying they wanted to be exclusive, but we don't want both (i.e. one of them is wrong), and
- All along we were getting lucky that the buggy one was the one that was coming "second" so we never noticed.
If I recall the alternative design we had for "exclusive" providers years ago was we would order providers so the override provider went "first" -- it's ironic that we still created an ordering requirement anyways. 😄
@AmadeusW @ivanbasov @dpoeschl Does the new editor API also expose an equivalent concept of "exclusive"? Which way does it behave?
|
I'm also not sure this is worth discussing at today's design meeting. I'd vote to remove it from the agenda unless somebody else actually wants to discuss it. FYI to @sharwell. |
|
Thanks @avodovnik for the fix! |
|
Nice! I'm glad we could make such a simple solution that solved your needs! |
|
Should be available in 16.2.Preview4 onwards |
As per the discussion in #36060, this PR attempts to introduce the change that would allow a
CompletionProviderto add items to the list, even if another one setsIsExclusive, like in the example of anOverrideCompletionProvider. This is partially only there to support a feature used by VS Mac.It's marked as a WIP as I'd still like to introduce a test for this, but want comments on the change as soon as possible to see if I should change direction.