fix: clarify conflict pane roles and status indicators
Use Resolution and Other version borders, omit branch/path metadata, and show unsaved output state only on the writable pane. Track native explorer conflict C from authoritative jj state across draft/check/error transitions and clear it only after confirmed resolution. Focused resolver and save tests pass; live terminal labels and explorer verified without saving the mock conflicts. Full library run: 795 pass, one unrelated async plugin fixture_barrier startup failure.
This commit is contained in:
Valdemar Lorenzen
+3
-1
@@ -460,7 +460,9 @@ Binary data, missing final newlines, permission/type changes, symlink targets, a
|
||||
|
||||
Run **`:resolve`** in Normal mode for the current jj working copy. It opens a dedicated tab and selects the current conflicted file (otherwise the first conflict). Historical comparisons remain read-only; resolving never implicitly checks out or creates another change.
|
||||
|
||||
**Resolution** shows an editable **Draft** on the left and a read-only **Alternative** on the right, with aligned rows, line/character highlighting, context folds and synchronized scrolling. Both are projected from the current working buffer, including pre-existing unsaved edits: only conflict-marker regions are replaced by their selected alternatives. Already-merged text and edits outside those regions appear on both sides. The initial Draft is seeded from the first side, labelled in its border, and is a normal undoable buffer change; opening never writes it to disk. Alternatives remain frozen while later edits change only the Draft.
|
||||
The resolver shows the editable **Resolution** on the left and a read-only **Other version** on the right, with aligned rows, line/character highlighting, context folds and synchronized scrolling. Both are projected from the current working buffer, including pre-existing unsaved edits: only conflict-marker regions are replaced by their selected alternatives. Already-merged text and edits outside those regions appear on both sides. The initial output is seeded from the first side as a normal undoable buffer change; opening never writes it to disk. References remain frozen while later edits change only the output.
|
||||
|
||||
Pane borders label the editable output **Resolution** and its frozen reference **Other version**. Paths and branch-comparison metadata are omitted from these titles; unsaved state belongs only to the output, and source identity appears with the reference. Explorer **C** marks files jj still considers conflicted, including while edited/checking or confirmation is unavailable; it clears only when jj confirms resolution.
|
||||
|
||||
| Command | Behavior |
|
||||
| --- | --- |
|
||||
|
||||
+11
-3
@@ -359,8 +359,8 @@ impl DiffState {
|
||||
}
|
||||
pub(crate) fn set_resolver_paths(&mut self, root: PathBuf, paths: Vec<PathBuf>, file: usize) {
|
||||
let comparison = self.comparison.get_or_insert_with(|| Comparison {
|
||||
before: "Draft".into(),
|
||||
after: "Alternative".into(),
|
||||
before: "Resolution".into(),
|
||||
after: "Other version".into(),
|
||||
files: Vec::new(),
|
||||
root: root.clone(),
|
||||
editable_paths: BTreeSet::new(),
|
||||
@@ -432,7 +432,7 @@ impl DiffState {
|
||||
self.loading = false;
|
||||
self.pending = None;
|
||||
self.request_kind = None;
|
||||
self.side_labels = ["Draft", "Alternative"];
|
||||
self.side_labels = ["Resolution", "Other version"];
|
||||
self.comparison_id = Some(self.comparison_id.unwrap_or(0).wrapping_add(1));
|
||||
self.live_files.clear();
|
||||
let file = &mut self.comparison.as_mut().unwrap().files[self.file];
|
||||
@@ -2465,6 +2465,12 @@ mod tests {
|
||||
files.entries().nth(files.selected).unwrap().path,
|
||||
Path::new("first.txt")
|
||||
);
|
||||
assert!(!files.is_conflicted(Path::new("first.txt")));
|
||||
state
|
||||
.files
|
||||
.as_mut()
|
||||
.unwrap()
|
||||
.set_conflicted(Path::new("first.txt"), true);
|
||||
state.set_resolver_paths(
|
||||
PathBuf::new(),
|
||||
vec!["binary.bin".into(), "first.txt".into(), "second.txt".into()],
|
||||
@@ -2475,6 +2481,8 @@ mod tests {
|
||||
files.entries().nth(files.selected).unwrap().path,
|
||||
Path::new("second.txt")
|
||||
);
|
||||
assert!(files.is_conflicted(Path::new("first.txt")));
|
||||
assert!(!files.is_conflicted(Path::new("second.txt")));
|
||||
}
|
||||
|
||||
fn excerpt_state(len: usize, changed: &[usize]) -> DiffState {
|
||||
|
||||
+102
-11
@@ -1482,6 +1482,8 @@ pub struct ResolverFile {
|
||||
pub path: PathBuf,
|
||||
pub draft_buffer: usize,
|
||||
pub status: ResolverStatus,
|
||||
/// Retained through draft edits and failed/pending jj confirmation.
|
||||
jj_conflicted: bool,
|
||||
pub sources: Vec<ResolverSource>,
|
||||
pub text: bool,
|
||||
pub whole_file_choice: Option<usize>,
|
||||
@@ -1492,6 +1494,22 @@ pub struct ResolverFile {
|
||||
comparison_key: Option<(u64, usize)>,
|
||||
}
|
||||
|
||||
impl ResolverFile {
|
||||
fn apply_jj_status(&mut self, status: crate::resolve::Status) {
|
||||
self.status = match status {
|
||||
crate::resolve::Status::Unresolved => {
|
||||
self.jj_conflicted = true;
|
||||
ResolverStatus::Unresolved
|
||||
}
|
||||
crate::resolve::Status::Resolved => {
|
||||
self.jj_conflicted = false;
|
||||
ResolverStatus::Resolved
|
||||
}
|
||||
crate::resolve::Status::Unconfirmed => ResolverStatus::Unconfirmed,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
pub struct ResolverWorkspace {
|
||||
pub files: Vec<ResolverFile>,
|
||||
pub current_file: usize,
|
||||
@@ -2285,6 +2303,16 @@ impl Editor {
|
||||
};
|
||||
}
|
||||
}
|
||||
if let Some(mut picker) = resolver
|
||||
.files
|
||||
.get_mut(resolver.current_file)
|
||||
.and_then(|file| file.diff.files.take())
|
||||
{
|
||||
for file in &resolver.files {
|
||||
picker.set_conflicted(&file.path, file.jj_conflicted);
|
||||
}
|
||||
resolver.files[resolver.current_file].diff.files = Some(picker);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2394,6 +2422,7 @@ impl Editor {
|
||||
files.push(ResolverFile {
|
||||
path: file.path.clone(), draft_buffer,
|
||||
status: ResolverStatus::Unresolved, text, whole_file_choice: None,
|
||||
jj_conflicted: file.status != crate::resolve::Status::Resolved,
|
||||
error: file.notice.clone(),
|
||||
seed_source,
|
||||
projection,
|
||||
@@ -2478,12 +2507,8 @@ impl Editor {
|
||||
.map(split_lines);
|
||||
let was_text = resolver.files[file].text;
|
||||
resolver.files[file].text = saved_text.is_some();
|
||||
let status = match model.files[file].status {
|
||||
crate::resolve::Status::Resolved => ResolverStatus::Resolved,
|
||||
crate::resolve::Status::Unresolved => ResolverStatus::Unresolved,
|
||||
crate::resolve::Status::Unconfirmed => ResolverStatus::Unconfirmed,
|
||||
};
|
||||
resolver.files[file].status = status;
|
||||
resolver.files[file].apply_jj_status(model.files[file].status);
|
||||
let status = resolver.files[file].status;
|
||||
resolver.files[file].error = outcome.confirmation.clone();
|
||||
resolver.files[file].whole_file_choice = None;
|
||||
if let Some(buffer) = if self.active.id == buffer {
|
||||
@@ -2639,6 +2664,7 @@ impl Editor {
|
||||
resolver.files[file].comparison_key = None;
|
||||
self.blur_file_picker();
|
||||
self.refresh_resolver_comparison();
|
||||
self.refresh_resolver_status();
|
||||
if self.resolver().unwrap().view == ResolverView::History {
|
||||
self.set_resolver_view(ResolverView::History);
|
||||
}
|
||||
@@ -12552,6 +12578,7 @@ mod tests {
|
||||
path: "file.txt".into(),
|
||||
draft_buffer: result,
|
||||
status: ResolverStatus::Unsaved,
|
||||
jj_conflicted: true,
|
||||
text: true,
|
||||
whole_file_choice: None,
|
||||
error: None,
|
||||
@@ -12688,11 +12715,6 @@ mod tests {
|
||||
ordinary.diff().unwrap().display_rows
|
||||
);
|
||||
assert!(resolver.diff_editing());
|
||||
assert_eq!(resolver.diff().unwrap().side_label(Focus::Before), "Draft");
|
||||
assert_eq!(
|
||||
resolver.diff().unwrap().side_label(Focus::After),
|
||||
"Alternative"
|
||||
);
|
||||
assert!(resolver.diff().unwrap().side_editable(Focus::Before));
|
||||
assert!(!resolver.diff().unwrap().side_editable(Focus::After));
|
||||
let draft_buffer = resolver.buffer_id();
|
||||
@@ -12791,6 +12813,7 @@ mod tests {
|
||||
path: "conflict.txt".into(),
|
||||
draft_buffer,
|
||||
status: ResolverStatus::Unsaved,
|
||||
jj_conflicted: true,
|
||||
text: true,
|
||||
whole_file_choice: None,
|
||||
error: None,
|
||||
@@ -12812,6 +12835,73 @@ mod tests {
|
||||
editor
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolver_explorer_retains_last_jj_conflict_state_until_confirmation() {
|
||||
let mut editor = resolver_editor("draft", &[("draft", false), ("other", false)]);
|
||||
let path = Path::new("conflict.txt");
|
||||
let marker = |editor: &Editor| {
|
||||
editor
|
||||
.resolver_workspace_diff()
|
||||
.unwrap()
|
||||
.files
|
||||
.as_ref()
|
||||
.unwrap()
|
||||
.is_conflicted(path)
|
||||
};
|
||||
assert_eq!(
|
||||
editor.resolver().unwrap().files[0].status,
|
||||
ResolverStatus::Unsaved
|
||||
);
|
||||
assert!(marker(&editor));
|
||||
for confirmed in [
|
||||
crate::resolve::Status::Unresolved,
|
||||
crate::resolve::Status::Resolved,
|
||||
crate::resolve::Status::Unresolved,
|
||||
] {
|
||||
editor.resolvers.get_mut(&editor.tab_id).unwrap().files[0].apply_jj_status(confirmed);
|
||||
editor.refresh_resolver_status();
|
||||
let conflicted = confirmed == crate::resolve::Status::Unresolved;
|
||||
assert_eq!(marker(&editor), conflicted);
|
||||
assert_eq!(
|
||||
editor.resolver().unwrap().files[0].status,
|
||||
ResolverStatus::Unsaved
|
||||
);
|
||||
|
||||
editor.resolvers.get_mut(&editor.tab_id).unwrap().files[0].status =
|
||||
ResolverStatus::Checking;
|
||||
editor.refresh_resolver_status();
|
||||
assert_eq!(
|
||||
editor.resolver().unwrap().files[0].status,
|
||||
ResolverStatus::Checking
|
||||
);
|
||||
assert_eq!(marker(&editor), conflicted);
|
||||
|
||||
editor.resolvers.get_mut(&editor.tab_id).unwrap().files[0]
|
||||
.apply_jj_status(crate::resolve::Status::Unconfirmed);
|
||||
assert_eq!(
|
||||
editor.resolver().unwrap().files[0].status,
|
||||
ResolverStatus::Unconfirmed
|
||||
);
|
||||
editor.active.saved_lines.clone_from(&editor.active.lines);
|
||||
editor.refresh_resolver_status();
|
||||
assert_eq!(
|
||||
editor.resolver().unwrap().files[0].status,
|
||||
ResolverStatus::Unconfirmed
|
||||
);
|
||||
assert_eq!(marker(&editor), conflicted);
|
||||
editor.apply(vec![Effect::Insert(" edit".into())]).unwrap();
|
||||
editor.refresh_resolver_status();
|
||||
assert_eq!(
|
||||
editor.resolver().unwrap().files[0].status,
|
||||
ResolverStatus::Unsaved
|
||||
);
|
||||
assert_eq!(marker(&editor), conflicted);
|
||||
|
||||
editor.select_resolver_file(0);
|
||||
assert_eq!(marker(&editor), conflicted);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolver_takes_only_current_diff_hunk_and_undo_preserves_native_newlines() {
|
||||
for (draft, alternative, chosen) in [
|
||||
@@ -12944,6 +13034,7 @@ mod tests {
|
||||
path: "second.txt".into(),
|
||||
draft_buffer: buffer,
|
||||
status: ResolverStatus::Unresolved,
|
||||
jj_conflicted: true,
|
||||
text: true,
|
||||
whole_file_choice: None,
|
||||
error: None,
|
||||
|
||||
@@ -101,6 +101,7 @@ pub struct FilePicker {
|
||||
tree_searching: bool,
|
||||
warning: Option<String>,
|
||||
repository_changes: Vec<WorkingFileChange>,
|
||||
conflicted_paths: HashSet<PathBuf>,
|
||||
has_virtual_entries: bool,
|
||||
}
|
||||
|
||||
@@ -306,6 +307,7 @@ impl FilePicker {
|
||||
tree_searching: false,
|
||||
warning,
|
||||
repository_changes: Vec::new(),
|
||||
conflicted_paths: HashSet::new(),
|
||||
};
|
||||
for root in roots {
|
||||
let tree = picker.compact(root, None);
|
||||
@@ -917,12 +919,28 @@ impl FilePicker {
|
||||
topology_changed || metadata_changed
|
||||
}
|
||||
|
||||
/// Last authoritative conflict state, independent of ordinary change status.
|
||||
pub fn is_conflicted(&self, path: &Path) -> bool {
|
||||
self.conflicted_paths.contains(path)
|
||||
}
|
||||
|
||||
pub(crate) fn set_conflicted(&mut self, path: &Path, conflicted: bool) {
|
||||
if conflicted {
|
||||
if !self.conflicted_paths.contains(path) {
|
||||
self.conflicted_paths.insert(path.to_owned());
|
||||
}
|
||||
} else {
|
||||
self.conflicted_paths.remove(path);
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn replace_paths(&mut self, paths: impl IntoIterator<Item = (usize, PathBuf)>) {
|
||||
self.replace_catalog(Self::from_paths(self.root.clone(), paths));
|
||||
}
|
||||
|
||||
fn replace_catalog(&mut self, mut replacement: Self) {
|
||||
replacement.presentation = self.presentation;
|
||||
replacement.conflicted_paths = std::mem::take(&mut self.conflicted_paths);
|
||||
let by_path: HashMap<_, _> = replacement
|
||||
.nodes
|
||||
.iter()
|
||||
|
||||
+113
-33
@@ -3942,6 +3942,31 @@ fn diff_pane_panel(
|
||||
return area;
|
||||
}
|
||||
let focus = if before { Focus::Before } else { Focus::After };
|
||||
if let Some(resolver) = resolver {
|
||||
let file = resolver.files.get(resolver.current_file);
|
||||
let title = if before {
|
||||
" Resolution "
|
||||
} else {
|
||||
" Other version "
|
||||
};
|
||||
let state = if before {
|
||||
format!(
|
||||
" editable · {} ",
|
||||
file.map_or("loading", |file| resolver_status(&file.status).0)
|
||||
)
|
||||
} else {
|
||||
" read-only ".to_owned()
|
||||
};
|
||||
let source = if before {
|
||||
String::new()
|
||||
} else {
|
||||
file.and_then(|file| file.sources.get(resolver.selected_source))
|
||||
.map_or_else(String::new, |source| {
|
||||
format!(" {} ", display_text(&source.label))
|
||||
})
|
||||
};
|
||||
return diff_panel(frame, area, title, &state, &source, diff.focus == focus);
|
||||
}
|
||||
let label = diff.side_label(focus);
|
||||
let title = file.map_or_else(
|
||||
|| format!(" Diff · {} ", diff.comparison_mode.label()),
|
||||
@@ -3964,25 +3989,8 @@ fn diff_pane_panel(
|
||||
)
|
||||
},
|
||||
);
|
||||
let revision = if let Some(resolver) = resolver {
|
||||
let source = resolver
|
||||
.files
|
||||
.get(resolver.current_file)
|
||||
.and_then(|file| {
|
||||
file.sources.get(if before {
|
||||
file.seed_source
|
||||
} else {
|
||||
resolver.selected_source
|
||||
})
|
||||
})
|
||||
.map_or("", |source| source.label.as_str());
|
||||
if before {
|
||||
format!(" seeded from {} ", display_text(source))
|
||||
} else {
|
||||
format!(" {} ", display_text(source))
|
||||
}
|
||||
} else {
|
||||
diff.comparison
|
||||
let revision = diff
|
||||
.comparison
|
||||
.as_ref()
|
||||
.map_or_else(String::new, |comparison| {
|
||||
if diff.view_mode == ViewMode::Split && diff.pane_layout() == PaneLayout::Split {
|
||||
@@ -4001,23 +4009,13 @@ fn diff_pane_panel(
|
||||
display_text(&comparison.after)
|
||||
)
|
||||
}
|
||||
})
|
||||
};
|
||||
});
|
||||
let access = if diff.side_editable(focus) {
|
||||
" editable "
|
||||
} else {
|
||||
" read-only "
|
||||
};
|
||||
let state = if let Some(resolver) = resolver {
|
||||
let status = resolver
|
||||
.files
|
||||
.get(resolver.current_file)
|
||||
.map_or("loading", |file| resolver_status(&file.status).0);
|
||||
Cow::Owned(format!("{access}· {status} "))
|
||||
} else {
|
||||
Cow::Borrowed(access)
|
||||
};
|
||||
diff_panel(frame, area, &title, &state, &revision, diff.focus == focus)
|
||||
diff_panel(frame, area, &title, access, &revision, diff.focus == focus)
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
@@ -5465,7 +5463,10 @@ fn draw_file_list(
|
||||
));
|
||||
spans.push(Span::raw(" "));
|
||||
}
|
||||
if let Some(status) = status {
|
||||
if picker.is_conflicted(entry.path) {
|
||||
spans.push(Span::styled("C", Style::default().fg(Color::Yellow)));
|
||||
spans.push(Span::raw(" "));
|
||||
} else if let Some(status) = status {
|
||||
let color = match status {
|
||||
FileStatus::Added => Color::Green,
|
||||
FileStatus::Removed => Color::Red,
|
||||
@@ -6994,7 +6995,6 @@ mod tests {
|
||||
assert!(draft.height > 20);
|
||||
assert!(views.panels.files.is_none());
|
||||
let screen = screen_text(&terminal);
|
||||
assert!(screen.contains("Draft") && screen.contains("Alternative"));
|
||||
assert!(screen.contains("unchanged lines hidden"));
|
||||
assert!(!screen.contains("context 30"));
|
||||
assert!(!screen.contains("<<<<<<<") && !screen.contains("%%%%%%%"));
|
||||
@@ -7003,6 +7003,27 @@ mod tests {
|
||||
.map(|x| terminal.backend().buffer()[(x, y)].symbol())
|
||||
.collect::<String>()
|
||||
};
|
||||
let left_heading = line_at(&terminal, draft, draft.y);
|
||||
let right_heading = line_at(&terminal, alternative, alternative.y);
|
||||
assert!(left_heading.contains("Resolution") && left_heading.contains("editable"));
|
||||
assert!(right_heading.contains("Other version") && right_heading.contains("read-only"));
|
||||
for heading in [&left_heading, &right_heading] {
|
||||
assert!(!heading.contains("Branch changes"));
|
||||
assert!(
|
||||
!heading.contains(resolver.files[current_file].path.to_string_lossy().as_ref())
|
||||
);
|
||||
assert!(!heading.split_whitespace().any(|word| word == "M"));
|
||||
}
|
||||
assert!(
|
||||
line_at(&terminal, alternative, alternative.bottom() - 1)
|
||||
.contains(&resolver.files[current_file].sources[alternative_index].label)
|
||||
);
|
||||
assert!(
|
||||
line_at(&terminal, draft, draft.bottom() - 1)
|
||||
.chars()
|
||||
.all(|ch| !ch.is_alphanumeric()),
|
||||
"The output must not repeat its seed source metadata"
|
||||
);
|
||||
for (side, area) in [draft, alternative].into_iter().enumerate() {
|
||||
let content = diff_inner(area);
|
||||
let x = content.right() - geometry.text_widths[side];
|
||||
@@ -7233,6 +7254,11 @@ mod tests {
|
||||
let geometry = views.panels.diff.as_ref().unwrap();
|
||||
let left = diff_inner(geometry.regions[1].0);
|
||||
let right = diff_inner(geometry.regions[2].0);
|
||||
let left_heading = line_at(&terminal, geometry.regions[1].0, geometry.regions[1].0.y);
|
||||
let right_heading = line_at(&terminal, geometry.regions[2].0, geometry.regions[2].0.y);
|
||||
assert!(left_heading.contains("unsaved"));
|
||||
assert!(!right_heading.contains("unsaved"));
|
||||
assert!(right_heading.contains("read-only"));
|
||||
assert!(line_at(&terminal, left, left.y + 1).contains("manual opening context 00"));
|
||||
assert!(line_at(&terminal, right, right.y + 1).contains("opening context 00"));
|
||||
assert!(!line_at(&terminal, right, right.y + 1).contains("manual"));
|
||||
@@ -9175,6 +9201,60 @@ mod tests {
|
||||
assert!(!text.contains('✓'));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn native_explorer_marks_only_confirmed_conflicts() {
|
||||
let directory = Directory::new();
|
||||
directory.file("file.txt", "");
|
||||
let mut picker = FilePicker::scan(
|
||||
directory.0.clone(),
|
||||
FilePresentation::Floating,
|
||||
FileView::List,
|
||||
)
|
||||
.unwrap();
|
||||
picker.set_repository_changes(vec![crate::repository::WorkingFileChange {
|
||||
path: "file.txt".into(),
|
||||
status: FileStatus::Modified,
|
||||
old_path: None,
|
||||
}]);
|
||||
let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(80, 12)).unwrap();
|
||||
for view in [FileView::List, FileView::Tree] {
|
||||
picker.set_view(view);
|
||||
for conflicted in [true, false] {
|
||||
picker.set_conflicted(std::path::Path::new("file.txt"), conflicted);
|
||||
terminal
|
||||
.draw(|frame| {
|
||||
draw_file_list(
|
||||
frame,
|
||||
&picker,
|
||||
frame.area(),
|
||||
"",
|
||||
&mut WheelPanels::default(),
|
||||
None,
|
||||
);
|
||||
})
|
||||
.unwrap();
|
||||
let text = screen_text(&terminal);
|
||||
let row = text.lines().find(|row| row.contains("file.txt")).unwrap();
|
||||
assert!(row.contains(if conflicted {
|
||||
"C file.txt"
|
||||
} else {
|
||||
"M file.txt"
|
||||
}));
|
||||
if conflicted {
|
||||
let buffer = terminal.backend().buffer();
|
||||
let (x, y) = (0..buffer.area.height)
|
||||
.find_map(|y| {
|
||||
(0..buffer.area.width)
|
||||
.find(|&x| buffer[(x, y)].symbol() == "C")
|
||||
.map(|x| (x, y))
|
||||
})
|
||||
.unwrap();
|
||||
assert_eq!(buffer[(x, y)].fg, Color::Yellow);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn normal_file_names_stay_put_when_status_arrives_or_clears() {
|
||||
let directory = Directory::new();
|
||||
|
||||
Reference in New Issue
Block a user