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 bugs in diff hunk highlighting #18454

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Fix bugs in diff hunk highlighting #18454

merged 2 commits into from
Sep 27, 2024

Conversation

maxbrunsfeld
Copy link
Collaborator

@maxbrunsfeld maxbrunsfeld commented Sep 27, 2024

Fixes #18405

In #18313, we introduced a problem where git addition highlights might spuriously return when undoing certain changes. It turned out, there were already some cases where git hunk highlighting was incorrect when editing at the boundaries of expanded diff hunks.

In this PR, I've introduced a test helper method for more rigorously (and readably) testing the editor's git state. You can assert about the entire state of an editor's diff decorations using a formatted diff:

    cx.assert_diff_hunks(
        r#"
        - use some::mod1;
          use some::mod2;
          const A: u32 = 42;
        - const B: u32 = 42;
          const C: u32 = 42;
          fn main() {
        -     println!("hello");
        +     //println!("hello");
              println!("world");
        +     //
        +     //
          }
          fn another() {
              println!("another");
        +     println!("another");
          }
        - fn another2() {
              println!("another2");
          }
        "#
        .unindent(),
    );

This will assert about the editor's actual row highlights, not just the editor's internal hunk-tracking state.

I rewrote all of our editor diff tests to use these more high-level assertions, and it caught the new bug, as well as some pre-existing bugs in the highlighting of added content.

The problem was how we remove highlighted rows. Previously, it relied on supplying exactly the same range as one that we had previously highlighted. I've added a remove_highlighted_rows(ranges) APIs which is much simpler - it clears out any row ranges that intersect the given ranges (which is all that we need for the Git diff use case).

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Sep 27, 2024
@zed-industries-bot
Copy link

Messages
📖

This PR includes links to the following GitHub Issues: #18405
If this PR aims to close an issue, please include a Closes #ISSUE line at the top of the PR body.

Generated by 🚫 dangerJS against 6d40929

@maxbrunsfeld maxbrunsfeld merged commit c3075df into main Sep 27, 2024
10 checks passed
@maxbrunsfeld maxbrunsfeld deleted the diff-hunk-bugs branch September 27, 2024 18:14
maxbrunsfeld added a commit that referenced this pull request Sep 27, 2024
Follow-up to #18454

Release Notes:

- N/A
schpet added a commit to schpet/zed that referenced this pull request Oct 3, 2024
* upstream/main: (33 commits)
  Fine-tune hunk control spacing (zed-industries#18463)
  Add tooltip for code actions icon button (zed-industries#18461)
  More git hunk highlighting fixes (zed-industries#18459)
  Move git hunk controls to the left side (zed-industries#18460)
  Capitalize tooltip labels on buffer search (zed-industries#18458)
  Install cargo-edito without extra features (zed-industries#18457)
  Fix bugs in diff hunk highlighting (zed-industries#18454)
  Remove Qwen2 model (zed-industries#18444)
  vim: Command selection fixes (zed-industries#18424)
  Add a `get-release-notes-since` script (zed-industries#18445)
  Fix GoToDefinition changing the viewport unnecessarily (zed-industries#18441)
  docs: Ollama api_url improvements (zed-industries#18440)
  Fix missing tooltips for selected buttons (zed-industries#18435)
  Add missing shortcuts in tooltips (zed-industries#18282)
  assistant: Fix copy/cut not working when selection is empty (zed-industries#18403)
  Fix the numeration in line wrap docs (zed-industries#18428)
  ssh remoting: Show error if opening connection timed out (zed-industries#18401)
  project: Fix worktree store event missing in remote projects (zed-industries#18376)
  Fix read timeout for ollama (zed-industries#18417)
  vim: Support za (zed-industries#18421)
  ...
noaccOS pushed a commit to noaccOS/zed that referenced this pull request Oct 19, 2024
Fixes zed-industries#18405

In zed-industries#18313, we introduced a
problem where git addition highlights might spuriously return when
undoing certain changes. It turned out, there were already some cases
where git hunk highlighting was incorrect when editing at the boundaries
of expanded diff hunks.

In this PR, I've introduced a test helper method for more rigorously
(and readably) testing the editor's git state. You can assert about the
entire state of an editor's diff decorations using a formatted diff:

```rust
    cx.assert_diff_hunks(
        r#"
        - use some::mod1;
          use some::mod2;
          const A: u32 = 42;
        - const B: u32 = 42;
          const C: u32 = 42;
          fn main() {
        -     println!("hello");
        +     //println!("hello");
              println!("world");
        +     //
        +     //
          }
          fn another() {
              println!("another");
        +     println!("another");
          }
        - fn another2() {
              println!("another2");
          }
        "#
        .unindent(),
    );
```

This will assert about the editor's actual row highlights, not just the
editor's internal hunk-tracking state.

I rewrote all of our editor diff tests to use these more high-level
assertions, and it caught the new bug, as well as some pre-existing bugs
in the highlighting of added content.

The problem was how we *remove* highlighted rows. Previously, it relied
on supplying exactly the same range as one that we had previously
highlighted. I've added a `remove_highlighted_rows(ranges)` APIs which
is much simpler - it clears out any row ranges that intersect the given
ranges (which is all that we need for the Git diff use case).

Release Notes:

- N/A
noaccOS pushed a commit to noaccOS/zed that referenced this pull request Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lingering phantom diff in editor
2 participants