gh-icsharpcode/AvalonEdit

Watch

Fork 0

Fixes for block select slowness and jumping caret issues #269

Open

zaksmolen wants to merge 2 commits from zaksmolen/block_select_fixes into master

zaksmolen commented

2021-03-08 12:08:12 -06:00

(Migrated from github.com)

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.

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.

siegfriedpammer (Migrated from github.com) reviewed

2021-03-08 13:04:21 -06:00

ICSharpCode.AvalonEdit/Editing/TextArea.cs

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.

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.

siegfriedpammer (Migrated from github.com) reviewed

2021-03-08 13:28:02 -06:00

ICSharpCode.AvalonEdit/Editing/TextArea.cs

Update: I had a look at the code again and I found the reason why this change is not causing any problems: github.com/icsharpcode/AvalonEdit@9e8ab4b177/ICSharpCode.AvalonEdit/Editing/RectangleSelection.cs (L323) 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.

Update: I had a look at the code again and I found the reason why this change is not causing any problems: https://github.com/icsharpcode/AvalonEdit/blob/9e8ab4b177b537c8e59159e05b5f5f23ff28ceb8/ICSharpCode.AvalonEdit/Editing/RectangleSelection.cs#L323 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.

zaksmolen commented

2021-03-08 14:30:35 -06:00

(Migrated from github.com)

I'll give that a test on my end!

On Mon, Mar 8, 2021 at 2:28 PM Siegfried Pammer notifications@github.com
wrote:

@siegfriedpammer commented on this pull request.

In ICSharpCode.AvalonEdit/Editing/TextArea.cs
https://github.com/icsharpcode/AvalonEdit/pull/269#discussion_r589688762
:

@@ -317,7 +317,10 @@ void OnDocumentChanging()
void OnDocumentChanged(DocumentChangeEventArgs e)
{
caret.OnDocumentChanged(e);
  	this.Selection = selection.UpdateOnDocumentChange(e);
  	if (! (selection is RectangleSelection)) {
Update: I had a look at the code again and I found the reason why this
change is not causing any problems:
github.com/icsharpcode/AvalonEdit@9e8ab4b177/ICSharpCode.AvalonEdit/Editing/RectangleSelection.cs (L323)
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.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/icsharpcode/AvalonEdit/pull/269#discussion_r589688762,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AC7P4IGUZZ5PR3B3V2GUZBLTCUQNFANCNFSM4YZ3ZH2Q
.

I'll give that a test on my end! On Mon, Mar 8, 2021 at 2:28 PM Siegfried Pammer <notifications@github.com> wrote: > *@siegfriedpammer* commented on this pull request. > ------------------------------ > > In ICSharpCode.AvalonEdit/Editing/TextArea.cs > <https://github.com/icsharpcode/AvalonEdit/pull/269#discussion_r589688762> > : > > > @@ -317,7 +317,10 @@ void OnDocumentChanging() > void OnDocumentChanged(DocumentChangeEventArgs e) > { > caret.OnDocumentChanged(e); > - this.Selection = selection.UpdateOnDocumentChange(e); > + > + if (! (selection is RectangleSelection)) { > > Update: I had a look at the code again and I found the reason why this > change is not causing any problems: > https://github.com/icsharpcode/AvalonEdit/blob/9e8ab4b177b537c8e59159e05b5f5f23ff28ceb8/ICSharpCode.AvalonEdit/Editing/RectangleSelection.cs#L323 > 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. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/icsharpcode/AvalonEdit/pull/269#discussion_r589688762>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AC7P4IGUZZ5PR3B3V2GUZBLTCUQNFANCNFSM4YZ3ZH2Q> > . >

siegfriedpammer (Migrated from github.com) reviewed

2021-03-08 14:41:25 -06:00

ICSharpCode.AvalonEdit/Editing/TextArea.cs

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?

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 commented

2021-03-08 15:01:21 -06:00

(Migrated from github.com)

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 commented

2021-03-09 08:29:15 -06:00

(Migrated from github.com)

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.

> 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 commented

2021-03-09 08:32:23 -06:00

(Migrated from github.com)

Yeh that makes sense, I'll give that a try this afternoon.

On Tue, Mar 9, 2021 at 9:29 AM Siegfried Pammer notifications@github.com
wrote:

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.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/icsharpcode/AvalonEdit/pull/269#issuecomment-793970909,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AC7P4IA2VKA6W2O4ADDTXRLTCYWEZANCNFSM4YZ3ZH2Q
.

Yeh that makes sense, I'll give that a try this afternoon. On Tue, Mar 9, 2021 at 9:29 AM Siegfried Pammer <notifications@github.com> wrote: > 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. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/icsharpcode/AvalonEdit/pull/269#issuecomment-793970909>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AC7P4IA2VKA6W2O4ADDTXRLTCYWEZANCNFSM4YZ3ZH2Q> > . >

zaksmolen commented

2021-03-09 09:44:55 -06:00

(Migrated from github.com)

I set up a test where I just did a character replacement on the whole text
block in a DispatcherTimer. Here's the behavior I see:

If the selection contains text that's changed, the selection goes away. I
think that makes sense? The content of the selection changed externally
so the selection is no longer valid.

If the selection is just a column, i.e. you're inserting text, it stays
even when the insert happens. This occurs even if your column selection is
right next to the changed characters.

To replicate, I just put this in the constructor of Window1 in the sample
application:

DispatcherTimer changeTestingTimer = new DispatcherTimer();
changeTestingTimer.Interval = TimeSpan.FromSeconds(10);
changeTestingTimer.Tick += delegate { textEditor.Text =
textEditor.Text.Replace('!', '*'); };
changeTestingTimer.Start();

Then I copied about 20 lines of "Welcome to! AvalonEdit!" and could paste
that into the editor and watch it change every 10 seconds while I was doing
different selections.

On Tue, Mar 9, 2021 at 9:32 AM Zak Smolen zaksmolen@gmail.com wrote:

Yeh that makes sense, I'll give that a try this afternoon.

On Tue, Mar 9, 2021 at 9:29 AM Siegfried Pammer notifications@github.com
wrote:

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.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/icsharpcode/AvalonEdit/pull/269#issuecomment-793970909,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AC7P4IA2VKA6W2O4ADDTXRLTCYWEZANCNFSM4YZ3ZH2Q
.

I set up a test where I just did a character replacement on the whole text block in a DispatcherTimer. Here's the behavior I see: If the selection contains text that's changed, the selection goes away. I _think_ that makes sense? The content of the selection changed externally so the selection is no longer valid. If the selection is just a column, i.e. you're inserting text, it stays even when the insert happens. This occurs even if your column selection is right next to the changed characters. To replicate, I just put this in the constructor of Window1 in the sample application: DispatcherTimer changeTestingTimer = new DispatcherTimer(); changeTestingTimer.Interval = TimeSpan.FromSeconds(10); changeTestingTimer.Tick += delegate { textEditor.Text = textEditor.Text.Replace('!', '*'); }; changeTestingTimer.Start(); Then I copied about 20 lines of "Welcome to! AvalonEdit!" and could paste that into the editor and watch it change every 10 seconds while I was doing different selections. On Tue, Mar 9, 2021 at 9:32 AM Zak Smolen <zaksmolen@gmail.com> wrote: > Yeh that makes sense, I'll give that a try this afternoon. > > On Tue, Mar 9, 2021 at 9:29 AM Siegfried Pammer <notifications@github.com> > wrote: > >> 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. >> >> — >> You are receiving this because you authored the thread. >> Reply to this email directly, view it on GitHub >> <https://github.com/icsharpcode/AvalonEdit/pull/269#issuecomment-793970909>, >> or unsubscribe >> <https://github.com/notifications/unsubscribe-auth/AC7P4IA2VKA6W2O4ADDTXRLTCYWEZANCNFSM4YZ3ZH2Q> >> . >> >

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'zaksmolen/block_select_fixes:zaksmolen/block_select_fixes'

git switch 'zaksmolen/block_select_fixes'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'

git merge --no-ff 'zaksmolen/block_select_fixes'

git switch 'zaksmolen/block_select_fixes'

git rebase 'master'

git switch 'master'

git merge --ff-only 'zaksmolen/block_select_fixes'

git switch 'zaksmolen/block_select_fixes'

git rebase 'master'

git switch 'master'

git merge --no-ff 'zaksmolen/block_select_fixes'

git switch 'master'

git merge --squash 'zaksmolen/block_select_fixes'

git switch 'master'

git merge --ff-only 'zaksmolen/block_select_fixes'

git switch 'master'

git merge 'zaksmolen/block_select_fixes'

git push origin 'master'

No reviewers

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-icsharpcode/AvalonEdit!269

No description provided.

Rows
Columns

Fixes for block select slowness and jumping caret issues #269

@siegfriedpammer commented on this pull request.

Checkout

Merge