Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SLVS-1483 Implement apply fix suggestion #5726

Merged
merged 11 commits into from
Oct 7, 2024
+647 −13
Merged

SLVS-1483 Implement apply fix suggestion #5726

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2f463d0
SLVS-1483 Add parameter that contains the info to apply a fix suggestion
gabriela-trutan-sonarsource Oct 2, 2024
3679763
SLVS-1483 Add functionality to calculate the span for a given line range
gabriela-trutan-sonarsource Oct 2, 2024
afd98a4
SLVS-1483 Add basic implementation for directly applying a fix sugges…
gabriela-trutan-sonarsource Oct 2, 2024
a8b68d0
SLVS-1483 Calculate the file's absolute path based on the config scope
vnaskos-sonar Oct 3, 2024
4934958
SLVS-1483 When applying change bring focus to changed line
vnaskos-sonar Oct 3, 2024
7f00766
SLVS-1483 When applying change bring window to front
vnaskos-sonar Oct 3, 2024
dc99c04
SLVS-1483 Fix CalculateSpan TODO with edge cases
vnaskos-sonar Oct 3, 2024
4dae51e
SLVS-1483 Remove unnecessary internal method
vnaskos-sonar Oct 3, 2024
6f62921
SLVS-1483 Cancel edit when exception occurs
vnaskos-sonar Oct 4, 2024
186552c
SLVS-1483 Process changes from bottom to top in order to avoid line m…
vnaskos-sonar Oct 4, 2024
78973a7
SLVS-1483 Move caret once, when all changes applied
vnaskos-sonar Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/IssueViz.UnitTests/FixSuggestion/FixSuggestionHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,17 +136,19 @@ public void ApplyFixSuggestion_WhenApplyingChange_BringWindowToFront()
}

[TestMethod]
public void ApplyFixSuggestion_WhenApplyingChange_BringFocusToChangedLines()
public void ApplyFixSuggestion_WhenApplyingChange_BringFocusToFirstChangedLines()
{
var suggestionParams = CreateFixSuggestionParams();
var suggestedChange = suggestionParams.fixSuggestion.fileEdit.changes[0];
var affectedSnapshot = MockCalculateSpan(suggestedChange);
List<ChangesDto> changes = [CreateChangesDto(1, 1), CreateChangesDto(3, 3)];
var suggestionParams = CreateFixSuggestionParams(changes: changes);
var firstSuggestedChange = suggestionParams.fixSuggestion.fileEdit.changes[0];
var firstAffectedSnapshot = MockCalculateSpan(firstSuggestedChange);
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

[TestMethod]
Expand Down
13 changes: 9 additions & 4 deletions src/IssueViz/FixSuggestion/FixSuggestionHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,23 +95,28 @@ private void ApplySuggestedChanges(string absoluteFilePath, List<ChangesDto> cha
{
ideWindowService.BringToFront();
var textView = documentNavigator.Open(absoluteFilePath);
var textEdit = textView.TextBuffer.CreateEdit();
for (var i = changes.Count - 1; i >= 0; i--)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were two considerations behind this decision:

  1. The first change is what triggered the error, so it would make sense to bring the focus to that line.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same behavior applies to SLI, it focuses on the first change.

{
var changeDto = changes[i];
var textEdit = textView.TextBuffer.CreateEdit();

try
{
var spanToUpdate = issueSpanCalculator.CalculateSpan(textView.TextSnapshot, changeDto.beforeLineRange.startLine, changeDto.beforeLineRange.endLine);
textView.Caret.MoveTo(spanToUpdate.Start);
textView.ViewScroller.EnsureSpanVisible(spanToUpdate, EnsureSpanVisibleOptions.AlwaysCenter);
if (i == 0)
{
textView.Caret.MoveTo(spanToUpdate.Start);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be a coming back to the refactoring, though? There is no task for it...

textView.ViewScroller.EnsureSpanVisible(spanToUpdate, EnsureSpanVisibleOptions.AlwaysCenter);
}
textEdit.Replace(spanToUpdate, changeDto.after);
textEdit.Apply();
}
catch (Exception)
{
textEdit.Cancel();
throw;
}
}

textEdit.Apply();
}
}