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.

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

Conversation

zaksmolen
Copy link

This pull request has two fixes in it. The first, in RectangleSelection.cs, fixes an issue where the caret would jump to the top in some large block selections (especially if they were out of the visual). It would set the caret to the default value of a nullable, which put it at the top. This doesn't overwrite the caret position if it's null.

The next fix is a major one, that's been plaguing me for years at this point. Block selections seemed to get exponentially slower based on selection size. A large selection of a few hundred lines would take noticeably a few seconds per character typed. I tracked this down to the OnDocumentChanged event in TextArea.cs. Basically, every key press would trigger the event, which would trigger the event on the selection. This meant that every key press would cause a lot of calculating and rendering for every sub-selection within the rectangular selection. By not calling this extra update event for rectangular selections I have been able to do thousands of lines in a selection with no issue. This change has been live in my application for months with no visible side effects.

@@ -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:

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

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?

@zaksmolen
Copy link
Author

zaksmolen commented Mar 8, 2021 via email

I updated the solution based on Siegfried's suggestions, removing the hardcoded exception for rectangular selections and instead having the update event just return the selection itself
@zaksmolen
Copy link
Author

I tried that (and , as far as I can tell it seems to work just as fine. I didn't see any issues poking around the sample application but I wasn't sure of a great test for changing text while someone has an active selection. I think I pushed my changes but I'm new to doing pull requests through forked repos.

@siegfriedpammer
Copy link
Member

I wasn't sure of a great test for changing text while someone has an active selection.

You could add a DispatcherTimer to the sample application, which modifies the document near the selection every second, or something similar.

@zaksmolen
Copy link
Author

zaksmolen commented Mar 9, 2021 via email

@zaksmolen
Copy link
Author

zaksmolen commented Mar 9, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants