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

Fixes for block select slowness and jumping caret issues #269

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixes for block select slowness and jumping caret issues #269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a518dc1
Fixes for block select slowness and jumping caret issues
Mar 8, 2021
757a54c
Update solution based on suggestions
Mar 8, 2021
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
5 changes: 4 additions & 1 deletion ICSharpCode.AvalonEdit/Editing/RectangleSelection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,10 @@ public override void ReplaceSelectionWithText(string newText)
pos = new TextViewPosition(document.GetLocation(editOffset + firstInsertionLength));
textArea.ClearSelection();
}
textArea.Caret.Position = textArea.TextView.GetPosition(new Point(GetXPos(textArea, pos), textArea.TextView.GetVisualTopByDocumentLine(Math.Max(startLine, endLine)))).GetValueOrDefault();
TextViewPosition? calculatedPositon = textArea.TextView.GetPosition(new Point(GetXPos(textArea, pos), textArea.TextView.GetVisualTopByDocumentLine(Math.Max(startLine, endLine))));
if (calculatedPositon != null) {
textArea.Caret.Position = calculatedPositon.GetValueOrDefault();
}
}
}

Expand Down
5 changes: 4 additions & 1 deletion ICSharpCode.AvalonEdit/Editing/TextArea.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,10 @@ void OnDocumentChanging()
void OnDocumentChanged(DocumentChangeEventArgs e)
{
caret.OnDocumentChanged(e);
this.Selection = selection.UpdateOnDocumentChange(e);

if (! (selection is RectangleSelection)) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not look like a clean solution to the problem. Why should RectangleSelection be a hard-coded exception? If you want to ignore changes for RectangleSelection, you could simply fix this by replacing the body of RectangleSelection.UpdateOnDocumentChange with return this;.

However, I don't like the asymmetry this introduces. All selections get notified and handle document changes except RectangleSelection, this is rather unexpected, I would argue... I think the better solution would be to fix the actual culprit: Each line is changed individually and produces a separate changed event. This causes a lot of work to be repeated for each line, instead of doing it once, after all lines are changed.

Copy link
Member

Choose a reason for hiding this comment

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

Update: I had a look at the code again and I found the reason why this change is not causing any problems:

AvalonEdit/ICSharpCode.AvalonEdit/Editing/RectangleSelection.cs

Line 323 in 9e8ab4b

textArea.Selection = new RectangleSelection(textArea, pos, Math.Max(startLine, endLine), GetXPos(textArea, pos));
always replaces the selection with a valid one after all changes are made. So I think the best solution would be to change RectangleSelection.UpdateOnDocumentChange to simply return this;, but with a comment added that explains this exception.

I suspect however that this would fall apart as soon as there would be someone other than the user editing the text in the document while the user has an active rectangular selection.

Copy link
Member

@siegfriedpammer siegfriedpammer Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Another idea: Instead of removing the logic in UpdateOnDocumentChange you could simply clear the selection while editing is in progress... this should not cause any problems, because the selection is replaced after all changes are applied anyway. This way, we correctly respond to changes that are not initiated by user input and get rid of all the extra useless work that is done and makes editing slow. What do you think?

this.Selection = selection.UpdateOnDocumentChange(e);
}
}

void OnUpdateStarted()
Expand Down