Fix https://github.com/mono/monodevelop/issues/4820#27007
Fix https://github.com/mono/monodevelop/issues/4820#27007sharwell merged 1 commit intodotnet:masterfrom
Conversation
Was mostly curious what this means precisely -- is Roslyn's IAccurateClassifier (through that interface itself) is now being called directly in the Visual Studio for Mac case? Or you're calling something else? |
|
This is the VSMac PR that switches to Roslyn classification: |
sharwell
left a comment
There was a problem hiding this comment.
Two of the integration test failures are caused by the change in behavior. These will need to be updated.
|
hey @KirillOsenkov Could you clarify why IAccurateClassifier being used? It is like IAccurateTagger? If so, are you guys using it for GetAllTags, or for GetTags? The former is good for purposes like printing (where you want accurate and up to date tags. but the latter is much better for performance as it is fully async. Thanks! |
|
Sorry for confusion, we are using We were using Roslyn classification before that PR but now it's using |
13f3b89 to
795fd0b
Compare
|
@DavidKarlas OK, awesome, good. Gave me a heart attack there for a moment! |
795fd0b to
0dd5b61
Compare
|
@jinujoseph for approval |
|
Pre-Approved to merge for 15.8.preview3 provided Dustins feedback is addressed |
DustinCampbell
left a comment
There was a problem hiding this comment.
There's another change that needs to be made to the VerifyProjectConfigChange integration test. After it switches to Release, Bar should be classified as a "method name"
|
@DustinCampbell integration tests are enabled again and I unskipped the relevant tests in #27106 so we should get good information from the build currently in progress 👍 |
9ee5b72 to
ce74111
Compare
MonoDevelop(VSfM) recently switched from using `Microsoft.CodeAnalysis.Classification.Classifier` to using `IAccurateClassifier` from VS API. But with this switch some classifications that were added in dotnet#24931 stopped working. Reason for this is that in that commit new classifications were filtered out by converting back to classification that was used before `ClassificationTypeNames.Identifier` so VS Windows would continue working fine. But later in dotnet#25723 this ClassificationTypes were added so I assume with removing this filtering VS Windows will work just fine since ClassificationTypes that were added dotnet#25723 have BaseType `ClassificationTypeNames.Identifier`.
ce74111 to
2fdfa4d
Compare
|
@DustinCampbell all tests passed. |
|
Please let us know how we can help to move this forward. VSMac needs this change. Thanks! |
DustinCampbell
left a comment
There was a problem hiding this comment.
Looks like everything is green!
|
👍 wonderful, thank you all! |
MonoDevelop(VSfM) recently switched from using
Microsoft.CodeAnalysis.Classification.Classifierto usingIAccurateClassifierfrom VS API.But with this switch some classifications that were added in #24931 stopped working. Reason for this is that in that commit new classifications were filtered out by converting back to classification that was used before
ClassificationTypeNames.Identifierso VS Windows would continue working fine. But later in #25723 this ClassificationTypes were added so I assume with removing this filtering VS Windows will work just fine since ClassificationTypes that were added #25723 have BaseTypeClassificationTypeNames.Identifier.Customer scenario
In VSfM some themes differentiate between property and method name highlighting, and this is considered regression, hence this PR.
Bugs this fixes
It's actually in monodevelop repository: mono/monodevelop#4820 I can open another one here if you want...
Workarounds, if any
None.
Risk
I'm not sure if this will cause any regression in VS Windows.
Performance impact
None to minimal improvement.
Is this a regression from a previous update?
No.
Root cause analysis
This was done intentionally so even if tests were added it wouldn't discover this problem.
How was the bug found?
Internal dogfooding of VSfM.
Test documentation updated?
This seems like too specific problem to be manually tested.