Skip to content

Chore: improve getDiff perfomance for the editor #2517

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

Closed
wants to merge 16 commits into from

Conversation

CatHood0
Copy link
Collaborator

@CatHood0 CatHood0 commented Mar 18, 2025

Description

Improved the performance of the getDiff method, which calculates the differences between two texts (oldText and newText). The main focus is to optimize the code to handle extremely long strings (e.g., 24,000 words or more) without affecting the editor's responsiveness.

Note

The videos for preview how this PR will be added as soon as i can fix a little part of code that degrades the perfomance.

Related Issues

Type of Change

  • Feature: New functionality without breaking existing features.
  • 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • 🧹 Refactor: Code reorganization, no behavior change.
  • Breaking: Alters existing functionality and requires updates.
  • 🧪 Tests: New or modified tests
  • 📝 Documentation: Updates or additions to documentation.
  • 🗑️ Chore: Routine tasks, or maintenance.
  • Build configuration change: Build/configuration changes.

@CatHood0 CatHood0 added the enhancement New feature or request label Mar 18, 2025
@CatHood0 CatHood0 self-assigned this Mar 18, 2025
@CatHood0 CatHood0 changed the title Fix: improve getDiff perfomance for the editor Chore: improve getDiff perfomance for the editor Mar 18, 2025
@CatHood0 CatHood0 marked this pull request as draft March 18, 2025 21:11
@CatHood0 CatHood0 added the in progress This issue or feature is currently being worked on by someone. label Mar 18, 2025
@CatHood0 CatHood0 closed this Mar 18, 2025
@CatHood0 CatHood0 reopened this Mar 18, 2025
@EchoEllet

This comment was marked as resolved.

@CatHood0 CatHood0 closed this Mar 21, 2025
@CatHood0 CatHood0 reopened this Mar 26, 2025
@CatHood0 CatHood0 removed the in progress This issue or feature is currently being worked on by someone. label Mar 26, 2025
@CatHood0 CatHood0 marked this pull request as ready for review March 26, 2025 06:08
@CatHood0 CatHood0 requested review from EchoEllet and removed request for EchoEllet March 26, 2025 06:08
@CatHood0 CatHood0 marked this pull request as draft March 26, 2025 06:12
@CatHood0
Copy link
Collaborator Author

This PR is ready to be tested for anyone. Please, if you detect that something is not working, report it here.

@CatHood0 CatHood0 marked this pull request as ready for review March 26, 2025 07:10
@EchoEllet
Copy link
Collaborator

EchoEllet commented Mar 26, 2025

This PR is ready to be tested for anyone.

Please provide them with minimal steps to test your branch:

dependency_overrides:
  flutter_quill:
    git:
      url: https://github.com/CatHood0/flutter-quill.git
      ref: improve_diff

BTW, I remember you telling this doesn't make a performance different. is it different now?

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 26, 2025

BTW, I remember you telling this doesn't make a performance different. is it different now?

Yes, that was because it only improved the previous implementation, but didn't change its foundation. The problem with the previous one is that it didn't limit the ranges for searching for changes. That is, with the previous getDiff, with a text of 50,000 words, if we inserted only one character at any position, it would unnecessarily go through the ENTIRE STRING, only to realize that only one character had been inserted.

Unlike that, the new getDiff uses the previous selection (the one in the QuillController, which hasn't been updated) and the new one (which comes from the TextEditingValue provided by updateEditingValue) as range limiters. By default (and it's better), the range for searching for changes is limited to a 20% of the selections passed (expands the search to left and right directions). However, when the selection isn't collapsed, we use its values ​​to correctly calculate the ranges we need to check without losing what we're looking for: limiting the search for changes to places where there aren't any.

By limiting the range, performance improves significantly, since it doesn't have to loop all the way to the end. Of course, always keep in mind that all this is relative to the cursor position.

Here is where we compute the exact ranges that we will process:

TextRange _getAffectedRange(
  String oldStr,
  String newStr,
  TextSelection oldSel,
  TextSelection newSel,
) {
  // Calculate combined selection area
  final start = math.min(oldSel.start, newSel.start);
  final end = math.max(oldSel.end, newSel.end);

  // Expand by 20% to capture nearby changes
  //
  // We use this to avoid check all the string length
  // unnecessarily when we can use the selection as a base
  // to know where, and how was do it the change
  final expansion = ((end - start) * 0.2).round();

  return TextRange(
    start: math.max(0, start - expansion),
    end: math.min(math.max(oldStr.length, newStr.length), end + expansion),
  );
}

@EchoEllet

This comment was marked as resolved.

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 26, 2025

The video has been removed. Could you please consider re-adding it with before vs after in the PR description?

I'm trying to record the video well so that it works better, but also so that it doesn't look so laggy, because, due to external factors (node ​​rendering), no matter how much I improve getDiff, it won't be able to solve certain performance losses.

For some quick context, remember that when there are new lines in the text, the nodes are split, and this means, there are more paragraphs, and there are more nodes. That means: more to render and we don't check which nodes are visible and which are not (which causes this), so, we are rendering all at the same time.

@CatHood0 CatHood0 marked this pull request as draft March 26, 2025 08:10
@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 26, 2025

Yeah, i found the issue what is making like the PR does not do nothing:

I was tested removing it. And the performance went from roughly 2000 ms to 120 ms (doing the same insert operation with the same values into a string of 13000 words). What's going on?

@EchoEllet probably comparing characters by index is too heavy?

And it would actually explain why I'm unable to demonstrate the power of this PR in the videos. I previously tried adding 50,000 words into the editor and it worked just fine (literally as you'd expect from an editor), but adding the code that (ironically) I removed in the video significantly degrades the performance.

clideo_editor_d41a1d16217a41b797f3cf36e2a4c586.mp4

@CatHood0 CatHood0 closed this Mar 26, 2025
@CatHood0 CatHood0 reopened this Mar 26, 2025
@CatHood0 CatHood0 mentioned this pull request Mar 26, 2025
3 tasks
@CatHood0

This comment was marked as outdated.

@CatHood0
Copy link
Collaborator Author

Since #2510 is working on avoid use of getDiff, i thinking on close this.

@CatHood0 CatHood0 closed this Mar 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants