Allow removing lines from patch directly#5277
Conversation
jesseduffield
force-pushed
the
remove-lines-from-patch-directly
branch
from
9775c4e to
590719d
Compare
jesseduffield
requested review from
stefanhaller
and removed request for
Copilot
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
jesseduffield
force-pushed
the
remove-lines-from-patch-directly
branch
from
590719d to
15e3017
Compare
There was a problem hiding this comment.
Pull request overview
Adds support for using the universal remove keybinding (typically d) in the patch building view to remove selected lines directly from a commit (mirroring staging view muscle memory), backed by new i18n strings, docs updates, and an integration test.
Changes:
- Add
d/Universal.Remove keybinding in patch building view to remove selected lines from the commit via interactive rebase. - Introduce new i18n strings for the new action (labels, tooltips, prompts, disabled reasons).
- Add an integration test and update generated keybindings docs accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/gui/controllers/patch_building_controller.go | Adds Universal.Remove binding, disabled-reason logic, confirmation flow, and rebase-backed removal implementation. |
| pkg/i18n/english.go | Adds translation keys/strings for the new remove-lines-from-commit action and related errors/tooltips. |
| pkg/integration/tests/patch_building/remove_lines_from_commit.go | New integration test covering d in patch building view to remove a selected line from a commit. |
| pkg/integration/tests/test_list.go | Registers the new integration test. |
| docs-master/keybindings/Keybindings_*.md | Updates generated keybindings docs to include the new d action in patch building view across languages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jesseduffield
force-pushed
the
remove-lines-from-patch-directly
branch
from
15e3017 to
81c96ef
Compare
|
Great, you beat me to it. I have had this on my todo list for quite a while. It might take me a while to review it, I'm busy right now with work and private stuff, but I'll get to it eventually (and the other two PRs). Looking forward to using it. |
"Remove" can be confusing when a deleted file is selected; in this cases it actually "un-removes" it. "Discard" hopefully makes it clearer that we are talking about the change to a file, and not the file itself.
Only mention resetting the patch when there actually is one. This way users have to read less text in the normal case, and the added note hopefully stands out more if there is one. Also, separate the note from the previous text by a blank line.
We don't add the commands to the menu when we're rebasing, so it's pointless to check this in the handlers. I guess alternatively we could add all commands always, and strike out the ones that are not applicable using DisabledReasons. But we don't usually do this in menus (except the global keybindings menu).
That's how we usually do this; it's useful to see in the keybindings menu that the command is disabled.
I always press 'd' in the patch building view, expecting that I can do exactly what I can do in the staging view, to find out I need to go space -> ctrl+p -> d and I think it's time to honour the muscle memory and support this convenience keybinding.
Show errors in panel. I find the toast too subtle for this case.
We have an existing text for this already, which fits well enough.
Turn the diff context check into a disabled reason too.
Allow discarding lines from patch directly I always press 'd' in the patch building view, expecting that I can do exactly what I can do in the staging view, to find out I need to go space -> ctrl+p -> d and I think it's time to honour the muscle memory and support this convenience keybinding.
No need for this to be exported.
Reset existing patch instead of showing an error. We do this also for discarding entire files, and I find it more convenient.
Add confirmation text about resetting the current patch if there is one.
Remove some code duplication; since we reset the patch first, we know that toggleSelection does the right thing. It does a little bit more than we need here, but I don't think that's a problem.
stefanhaller
force-pushed
the
remove-lines-from-patch-directly
branch
from
81c96ef to
5b890bb
Compare
|
@jesseduffield I force-pushed the branch and added a bunch of additional commits; some before yours to fix and cleanup things in the existing discard-entire-files feature, and then a number of fixups for your commit. They all have commit messages, please review and comment on the ones that you disagree with. I'm basically fine with the current state now; there's just one cosmetic problem that bothers me a bit, which is that the custom patch that we're creating behind the scenes is visible for a moment until everything is refreshed. I wonder if we can do a sync or blocks-ui refresh to prevent this; didn't look into it myself. |
I always press 'd' in the patch building view, expecting that I can do exactly what I can do in the staging view, to find out I need to go space -> ctrl+p -> d and I think it's time to honour the muscle memory and support this convenience keybinding. I recall we may have had a debate about this in the past and if so I'm renewing my stance.
PR Description
Please check if the PR fulfills these requirements
go generate ./...)