Fix for #2128 (Outlining cuts off closing } )#26551
Fix for #2128 (Outlining cuts off closing } )#26551jinujoseph merged 1 commit intodotnet:masterfrom josetr:master
Conversation
|
@jinujoseph Can this community PR be given a buddy? Preferably someone familiar with WPF. Thanks! |
There was a problem hiding this comment.
Curlies required in all IDE code.
There was a problem hiding this comment.
can you add a comment why this is necessary? For example, i can see: view.VisualElement.Height = view.LineHeight * view.TextBuffer.CurrentSnapshot.LineCount; a few lines above this. So why would would this have any effect? Thanks!
There was a problem hiding this comment.
Because they are executed at different points in time and may not give the same result since they rely on view.LineHeight which is mutable. They may give the same value sometimes, but by coincidence, and that's why this bug is not obvious and hasn't been fixed earlier.
I've removed the first view.VisualElement.Height = ... with the last commit since it's not necessary in VS2017.
There was a problem hiding this comment.
why do you only do this if 'currentHeight' is 'normal'?
There was a problem hiding this comment.
Mistake, fixed in the last commit.
It's probable because this codepath was only expected to be used by wpf views that we only expected a single layout to occur for. It's likely ok to add the |
Can you do some experimentation to find out? |
Sure. That seems somewhat reasonable. However, i would do that in another PR. |
|
|
|
This doesn't seem more terrifying than any other code that ever touches this. Anybody else from @dotnet/roslyn-ide want a crack? @jinujoseph We'll need your approval; looks fine to me. |
|
@dotnet-bot retest this please. |
|
@sharwell for second review |
This bug ocurred because the height was calculated too early. We can't rely on `view.LineHeight` too early because Visual Studio may still change it depending on your `Font Settings` or `Editor Zoom %`. I've also done some refactoring & removed the comments because they are no longer needed.
|
@jasonmalinowski thanks for approving the changes altho I think you may need to do it again because I've merged all the commits into 1 and cleaned up the description. This is something I wanted to do a long time ago but forgot, sorry for that. |
|
@josetr Not a problem! |
|
@jinujoseph This one is ready for delegated M2 approval. |
|
Approved to merge for 15.8.Preview4 |
For example,
view.LineHeightmay be 12 whenSizeToFit()is called, but when the firstLayoutChangedevent is received, it may have changed to 11, 13, etc. This bug is not obvious because it only happens with the right combination ofFont Sizeor Editor Zoom %.There is another pull request addressing this issue (#20825), but it breaks for me if the
zoomis set to some weird value like 99%.IsNormalis now insideSizeToFitbecause It only works in this context.Since we are talking about the outlining tooltip, I've noticed that it's trimmed based on the amount of characters (1000 max), is this really what we want? shouldn't it be trimmed based on the amount of lines instead?