diffedit: start the builtin ui with all boxes checked #5393
Conversation
(fixes jj-vcs#5390) Currently, there is a blocker: when quitting the ui with 'q' after making no changes, you get prompted with 'you have modified N files, are you sure?', which seems like a bug in the scm_record library.
v-gb
force-pushed
the
push-uyrwsoquqrqr
branch
from
6307668 to
68c9aec
Compare
There was a problem hiding this comment.
Thanks for implementing this (and filing the bug upstream on scm-record)!
Re testing:
- I guess it would be good to add a unit test to
mod teststo ensure thatInitialSelection::Allis respected for all kinds of sections we know how to render, and ensure we have some coverage for that code path. - I don't see a good way of adding a test that confirms that the
InitialSelectionis threaded all the way down to the appropriate diff editor usage sites.- IIRC we don't have any TUI tests at present.
- You could also reasonably argue that it's a specification issue rather than an implementation issue, so such a test may be low-value compared to implementation effort.
- Having the
enumshould reduce likelihood of type confusion with other booleans.
| format_instructions, | ||
| InitialSelection::None, |
There was a problem hiding this comment.
comment (non-blocking): We could consider if we want to bundle the "configuration" kind of arguments here into a single struct for organizational reasons, but I don't feel strongly.
|
Thanks! I'll see about adding some unit tests. |
| @@ -255,14 +256,17 @@ impl DiffEditor { | |||
| right_tree: &MergedTree, | |||
| matcher: &dyn Matcher, | |||
| format_instructions: impl FnOnce() -> String, | |||
| initial_selection: InitialSelection, | |||
There was a problem hiding this comment.
Maybe better to always open builtin editor with everything checked?
It's weird that initial_selection applies only to the builtin editor. External editor opens with the right content (i.e. everything selected?)
There was a problem hiding this comment.
I don't have a strong opinion myself, but the per-command difference was discussed here: #5390 (comment)
It seems to make sense to me that we would want e.g. split to start with nothing but diffedit to start with everything. Maybe it's more of a limitation with external diff editors? Is there a way to indicate to an external diff editor "here is a set of changes, but don't apply any of them by default"?
There was a problem hiding this comment.
I don't think there's a way of telling external 2-way diff editors that. But we can tell external 3-way diff editors that by setting the middle state to be either the same as the left (for split) or the same as the right (for diffedit).
By the way, another aspect is what the effect of starting the diff editor and immediately quitting should do. One could argue that it should have the same effect as not even starting it. split and diffedit are always interactive (unless you pass paths to split), but squash and restore can be non-interactive. Is it important to us that jj squash has the same effect as running jj squash -i and immediately closing the diff editor? I'm fine with that inconsistency.
(fixes #5390)
Currently, there is a blocker: when quitting the ui with 'q' after making no changes, you get prompted with 'you have modified N files, are you sure?', which seems like a bug in the scm_record library.
Separately, I tested this manually but I'm unsure whether and how I should add automated tests. The problem is that this change only affects the builtin UI, which I don't know how to test automatically. If I make external tools support this new
InitialSelectionparameter, I could test thatdiffeditreceivesInitialSelection::All, but not the bulk of the change.Checklist
If applicable:
CHANGELOG.md