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

Ensure editor is visible before measuring block decorations #854

Conversation

savetheclocktower
Copy link
Contributor

Identify the Bug

The bug is probably the cause of this issue in the github package.

Description of the Change

For the reasons described in the link above, this change is certainly more correct than what we already had:

  • The goal of measureBlockDecorations is to pre-compute the heights of certain editor additions (like the “stage hunk” controls in the github diff view that exhibits the bug)
  • This bug is happening because the text editor is not yet visible when this code is running, so it is pre-computing a bunch of 0 values for things that definitely would have height if the editor were visible
  • It’s obviously wrong to do memoization of invalid values

But how do we know this fixes the issue? After all, the fact that this code block is being run before the editor is visible seems like it’s a bug in itself!

Perhaps so, but

  • we know that this code path will run at least once more — in Etch, it would be very unlikely for this editor to go from hidden to visible without going through the updateSync code path
  • hence if we’re hitting updateSync when the editor is not yet visible, it’s safe to assume that there will be later trip through the same code path, hence another opportunity to measure the block decorations when they will be more accurate
  • in this particular case, introducing this conditional made the issue impossible to reproduce (for me)

Alternate Designs

This was the only fix I could think of that didn’t involve doing a deep dive into the exact timing of editors being hidden and shown as a result of changing tabs in a workspace pane.

Possible Drawbacks

I can’t think of any. Any scenarios that could be affected negatively by this code change are scenarios in which this bug was already manifesting. At worst it’s a lateral move!

Verification Process

My comment linked above illustrates one way I was able to trigger this bug. I can attest that those instructions didn’t reproduce the bug anymore once this fix was applied. But I advise you to try it on your own.

Release Notes

Fix a measurement issue that was causing visual glitches in the github package’s diff views.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Thanks for the helpful PR body. From a code only standpoint this seems like a no-brainer, and I'm happy to trust your notes that at worse this is a lateral move. So lets get it merged, and cross our fingers that we are in fact solving that bug as well as even some others

@savetheclocktower savetheclocktower merged commit bf3cfde into pulsar-edit:master Jan 7, 2024
99 checks passed
@savetheclocktower savetheclocktower deleted the fix-block-decoration-measurements branch January 7, 2024 23:46
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