SLVS-1483 Implement apply fix suggestion #5726
Conversation
There was a problem hiding this comment.
I have a weird behavior when applying this quick fix multiple times in the src/CFamily/CMake/EnvironmentVarsProvider.cs class (https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&id=sonarlint-visualstudio&open=AXvvZrcGMCXQ68kD9EcE&tab=suggested_fix&get-fix=true): a line is being removed each time the "View fix in IDE" button is clicked.
Also please see some feedback bellow,
| try | ||
| { | ||
| var spanToUpdate = issueSpanCalculator.CalculateSpan(textView.TextSnapshot, changeDto.beforeLineRange.startLine, changeDto.beforeLineRange.endLine); | ||
| textView.Caret.MoveTo(spanToUpdate.Start); |
There was a problem hiding this comment.
We should not move the caret and make each change visible (lines 105-106). In doing so, we will affect performance and execute logic that is not really needed. E.g. If there are 20 changes for a fix, we don't have to always move the caret and ensure it is visible, only for the last one.
| throw new NotImplementedException(); | ||
| ideWindowService.BringToFront(); | ||
| var textView = documentNavigator.Open(absoluteFilePath); | ||
| for (var i = changes.Count - 1; i >= 0; i--) |
There was a problem hiding this comment.
Because it is in descending order, the last change that is visible is actually the first change. Do you think it makes sense? Or should the last change be the visible one?
There was a problem hiding this comment.
There were two considerations behind this decision:
- The first change is what triggered the error, so it would make sense to bring the focus to that line.
- Sometimes, the last change is just a removal of the last empty line; bringing the focus to the first would make more sense than showing the last removed line.
There was a problem hiding this comment.
I understand 1. But I don't think we can really judge that 2 is something that will always happen (it might be that the first change is removing a line).
Anyway, we can leave it like this and see the feedback from users. :)
There was a problem hiding this comment.
The same behavior applies to SLI, it focuses on the first change.
|
| var textView = MockOpenFile(); | ||
| MockConfigScopeRoot(); | ||
|
|
||
| testSubject.ApplyFixSuggestion(suggestionParams); | ||
|
|
||
| textView.ViewScroller.Received().EnsureSpanVisible(affectedSnapshot, EnsureSpanVisibleOptions.AlwaysCenter); | ||
| textView.ViewScroller.ReceivedWithAnyArgs(1).EnsureSpanVisible(Arg.Any<SnapshotSpan>(), default); | ||
| textView.ViewScroller.Received().EnsureSpanVisible(firstAffectedSnapshot, EnsureSpanVisibleOptions.AlwaysCenter); |
There was a problem hiding this comment.
Here it should also be Received(1).EnsureSpanVisible to ensure it is received once and not any amount of times (Received() will not check for the amount of times it is called)
| textView.ViewScroller.EnsureSpanVisible(spanToUpdate, EnsureSpanVisibleOptions.AlwaysCenter); | ||
| if (i == 0) | ||
| { | ||
| textView.Caret.MoveTo(spanToUpdate.Start); |
There was a problem hiding this comment.
Consider extracting the lines 106-110 into a separate method. Clean code usually recommends to avoid having huge methods as this crowds the logic and makes it more difficult to understand.
There was a problem hiding this comment.
In this particular case, I think that it's better not to extract the if to a separate method. If I had to move something for clarity I would consider reworking the try. I would leave it as is at the moment and come back for refactoring when all the tasks affecting this piece are done.
There was a problem hiding this comment.
Will there be a coming back to the refactoring, though? There is no task for it...
SLVS-1483