This repository was archived by the owner on Sep 30, 2024. It is now read-only.
blob: Refactor blame decorations #58042
Merged
Merged
Conversation
fkling
commented
Nov 1, 2023
Contributor
Author
There was a problem hiding this comment.
Most of this is copied from blameRecency.tsx.
Contributor
|
Codenotify: Notifying subscribers in OWNERS files for diff a0c1aae...82a1bd2.
|
fkling
commented
Nov 1, 2023
Contributor
Author
There was a problem hiding this comment.
Optional effects was added to useCompartment to support the 'lock first line' use case.
Contributor
6fd447c to
a6d4093
Compare
taras-yemets
approved these changes
Nov 1, 2023
taras-yemets
left a comment
Contributor
There was a problem hiding this comment.
Great work, @fkling! Thank you!
On the demo video (sec 20-21) the version on the right (with current PR changes) has slightly misaligned background colors on hover. Is it fixed in the final version?
Contributor
Author
|
@taras-yemets That's also an issue in the current version. It happens when selecting an empty line and is somehow related to us using a layer to mark selected lines. I haven't figured out how to fix that yet. |
camdencheek
approved these changes
Nov 1, 2023
camdencheek
left a comment
Member
There was a problem hiding this comment.
I at least read through this PR to try to get a feel for things, but I don't really know enough to provide any valuable feedback. Looks good though 😄
Comment on lines
29
to
31
Member
There was a problem hiding this comment.
wouldn't want a scale that is too sensible 😄
Contributor
Author
There was a problem hiding this comment.
In my defense that's copy and paste :D
This commit refactors blame decorations to be more configuration driven and to fix the following issues: - Not working "lock first line" behavior - Wrong (no) blame decoration shown after a folded range The implementation is changed to only render a React node in a line where we actually show a decoration. This should slighlty improve performance. In order to still show the recency indicator across all lines it is moved into its own gutter and rendered directly via the DOM. Recency colors have been refactored to avoid having to use `useIsLightTheme`. `BlameColumn` wasn't used anymore and could be removed which also allowed changes to the BlameDecoration` itself.
fc83317 to
02f13cc
Compare
9497a44 to
82a1bd2
Compare
vovakulikov
pushed a commit
that referenced
this pull request
Dec 12, 2023
This commit refactors blame decorations to be more configuration driven. Specific changes and fixes: - Fixed "lock first line" behavior. It think this was because the lock line behavior was triggered multiple times due to data loading. Separating showing the column and showing the data seems to fix this. - Wrong (no) blame decoration shown after a folded range. This was an interesting issue. The root issue was that the blame decoration component would only render something when it was shown in its start line. But when a range is folded that start line might be hidden. Removing this condition together with other changes fixes this issue. - Previously every line was rendering a blame decoration component, but for most lines the component wouldn't display anything. Even without displaying anything, creating a react root per line contributed to unnecessary overhead. The implementation was changed to only render the a decoration in the first (visible) line of the hunk. In order to still show the recency indicator across all lines it is moved into its own gutter and rendered directly via the DOM. - Recency colors have been refactored to avoid having to use `useIsLightTheme`. - `BlameColumn` wasn't used anymore and could be removed which also allowed changes to the `BlameDecoration` component itself. - Instead of just highlighting the line when hovering over the blame information the whole hunk is highlighted. It should be possible to highlight the whole hunk when hovering anywhere in the blame column, but due to how we implement it (widget decorations) it would require some more work. Additional notes: - Using React for this, even with fewer occurrences, still has some performance penalty. This becomes apparent when scrolling in a larger document with blame enabled, and hovering over blame information. The more often this is done the slower the blame decorations seem to react (ha!) to hover events (to be fair, I assume it's due to React, it could also be for other reasons) - Scenario: `Hunk 1/Commit A | Hunk 2/Commit B | Hunk 3/Commit A` If code is folded so that Hunk 2 is completely hidden, it would be nice if we only displayed `Hunk 1/Commit A` instead of `Hunk 1/Commit A | Hunk 3/Commit A`. That's actually relatively easy to do but getting the new "highlight hunk lines" to work properly then is more difficult.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit refactors blame decorations to be more configuration driven. Specific changes and fixes:
The implementation was changed to only render the a decoration in the first (visible) line of the hunk. In order to still show the recency indicator across all
lines it is moved into its own gutter and rendered directly via the DOM.
useIsLightTheme.BlameColumnwasn't used anymore and could be removed which also allowed changes to theBlameDecorationcomponent itself.Additional notes:
Hunk 1/Commit A | Hunk 2/Commit B | Hunk 3/Commit AIf code is folded so that Hunk 2 is completely hidden, it would be nice if we only displayed
Hunk 1/Commit Ainstead ofHunk 1/Commit A | Hunk 3/Commit A. That's actually relatively easy to do but getting the new "highlight hunk lines" to work properly then is more difficult.Test plan
Manual testing:
sg-blame-changes-final.mp4