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

Fix indefinite line tracker suspension after closing large dirty editor #3067

Merged

Conversation

gluxon
Copy link
Contributor

@gluxon gluxon commented Dec 20, 2023

Description

Fixes #3066. See issue link for an in-depth explanation of the bug, steps to reproduce, and a video.

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@@ -44,6 +44,7 @@ export class LineTracker<T> implements Disposable {
this._selections = toLineSelections(editor?.selections);

this.notifyLinesChanged('editor');
this.onActiveEditorChanged?.();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was brainstorming a few alternatives to this approach.

  1. Instead of the indirection through GitLineTracker, we could also call this.resume() right here. That felt strange since nothing else in lineTracker.ts suspends or resumes the tracker. This feels like a gitLineTracker.ts responsibility based on what's written today.
  2. We could call this.fireDocumentDirtyStateChanged in the window.onDidChangeActiveTextEditor handler. This feels like a reasonable alternative approach to me. The main downside is that it's a bit of a slippery slope with regard to what document-specific change events are re-fired when the active editor changes. Ex: Would we also want to call onBlameStateChanged when the active editor changes?

I settled on the current approach, but happy to consider others in more depth if others have thoughts.

@gluxon gluxon marked this pull request as ready for review December 20, 2023 06:28
@gluxon gluxon force-pushed the resume-after-dirty-changes-on-large-document branch from 67ac10e to 594c0a0 Compare December 20, 2023 06:33
@gluxon
Copy link
Contributor Author

gluxon commented Dec 20, 2023

Testing

Here's a test double checking that the original issue is fixed. A video of the original bug is present in #3066.

Fixed.mov

@gluxon
Copy link
Contributor Author

gluxon commented Dec 20, 2023

I found it surprising that the GitLineTracker disappears completely while a file is dirty for 5 seconds and completely disappears for large dirty files. It makes sense that GitLens shouldn't compute diff frequently while editing, but I wonder if we should show its status in the user interface?

One idea would be to show an hourglass similar to what GitLens currently does as you move between lines. On hover, the indicator would show why it's delaying blame computation with a suggestion to tweak advanced.blame.sizeThresholdAfterEdit or advanced.blame.delayAfterEdit.

@jnewell7

This comment was marked as spam.

@eamodio eamodio self-assigned this Jan 5, 2024
@eamodio eamodio added this to the 14.7 milestone Jan 5, 2024
@eamodio eamodio merged commit 6bc9793 into gitkraken:main Jan 5, 2024
@eamodio
Copy link
Member

eamodio commented Jan 5, 2024

Thank you so much for your contribution here! Great investigation work and resolution! I've merged this directly, but I will be making some further changes to unify the LineTracker and GitLineTracker (no real need for the separation at this point), and will look into some kind of "notification" statusbar or something to let the user know why things are suspended and be able to point them to changing values to tweak it if you want more responsiveness and can spare the extra compute overhead

Thank you!

@gluxon gluxon deleted the resume-after-dirty-changes-on-large-document branch January 8, 2024 19:16
@gluxon
Copy link
Contributor Author

gluxon commented Jan 8, 2024

Thanks so much for reviewing this! Further changes post-merge are all welcome. I appreciate you taking the time to consider my thoughts.

@eamodio
Copy link
Member

eamodio commented Jan 15, 2024

Refactored things a bit here:
e522866

And added the following when the delay is active -- clicking on the link will jump to the setting in the settings UI

image

And this when the file is over the threshold -- clicking on the link will jump to the setting in the settings UI

image

FYI, the default delay is 5000 ms and the default line threshold is 5000, the numbers above in the screenshots was just me testing

@gluxon
Copy link
Contributor Author

gluxon commented Jan 15, 2024

That looks fantastic! Love it.

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.

Dirtying Large File Indefinitely Suspends Git Line Blame
3 participants