Skip to content

Commit

Permalink
Fix sliders in a single global pass
Browse files Browse the repository at this point in the history
Previously we fixed sliders in each 'possibly changed' region. This
meant that we couldn't fix sliders that needed to move outside the
region. The most common case was code of the form `foo, bar, baz`
where `, baz` was unchanged but we wanted to slide to `,`.

We now call `fix_all_sliders` for the toplevel tree on both
sides. This required some minor changes to the slider logic, as the
unchanged/novel regions could occur at any level of the tree.

(It was probably also the case that we were missing slider
opportunities previously, because we terminated as soon as we found an
outer slider for the nested case.)

This change has no performance impact, probably because tree diffing
is vastly more expensive (O(N^2)) than sliders (O(N)).

Fixes #327
  • Loading branch information
Wilfred committed Sep 3, 2022
1 parent 114235e commit b104c4b
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 15 deletions.
3 changes: 3 additions & 0 deletions sample_files/compare.expected
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ b1fe2c184a9a358e314e21aabb0f3cb7 -
sample_files/simple_before.txt sample_files/simple_after.txt
4b653ebe89321835c35722dd065cf6a2 -

sample_files/slider_at_end_before.json sample_files/slider_at_end_after.json
1d6162aab8e59c4422e9c14f09ceac3e -

sample_files/slider_before.rs sample_files/slider_after.rs
50e1df5af0bf4a1fa7211e079196f1a9 -

Expand Down
5 changes: 5 additions & 0 deletions sample_files/slider_at_end_after.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[
"one",
"two",
"three"
]
7 changes: 7 additions & 0 deletions sample_files/slider_at_end_before.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[
"one",
"novel-1",
"two",
"novel-2",
"three"
]
20 changes: 10 additions & 10 deletions src/diff/sliders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,11 @@ fn fix_nested_slider_prefer_outer<'a>(node: &'a Syntax<'a>, change_map: &mut Cha
}
}
ReplacedComment(_, _) => {}
Novel => {
for child in children {
fix_nested_slider_prefer_outer(child, change_map);
}
}
Novel => {}
}

for child in children {
fix_nested_slider_prefer_outer(child, change_map);
}
}
}
Expand All @@ -155,11 +155,7 @@ fn fix_nested_slider_prefer_inner<'a>(node: &'a Syntax<'a>, change_map: &mut Cha
.get(node)
.expect("Changes should be set before slider correction")
{
Unchanged(_) => {
for child in children {
fix_nested_slider_prefer_inner(child, change_map);
}
}
Unchanged(_) => {}
ReplacedComment(_, _) => {}
Novel => {
let mut found_unchanged = vec![];
Expand All @@ -170,6 +166,10 @@ fn fix_nested_slider_prefer_inner<'a>(node: &'a Syntax<'a>, change_map: &mut Cha
}
}
}

for child in children {
fix_nested_slider_prefer_inner(child, change_map);
}
}
}

Expand Down
12 changes: 7 additions & 5 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,6 @@ fn diff_file_content(
break;
}
}

let language = language.unwrap();
fix_all_sliders(language, &lhs_section_nodes, &mut change_map);
fix_all_sliders(language, &rhs_section_nodes, &mut change_map);
}

if exceeded_graph_limit {
Expand All @@ -342,10 +338,16 @@ fn diff_file_content(
rhs_positions,
)
} else {
// TODO: Make this .expect() unnecessary.
let language =
language.expect("If we had a ts_lang, we must have guessed the language");
fix_all_sliders(language, &lhs, &mut change_map);
fix_all_sliders(language, &rhs, &mut change_map);

let lhs_positions = syntax::change_positions(&lhs, &change_map);
let rhs_positions = syntax::change_positions(&rhs, &change_map);
(
language.map(|l| language_name(l).into()),
Some(language_name(language).into()),
lhs_positions,
rhs_positions,
)
Expand Down

0 comments on commit b104c4b

Please sign in to comment.