Skip to content

Conversation

@txemaotero
Copy link
Contributor

  • New commit_view.cr_on_hunk_jumps_to_current option to interchange the behaviour of <cr> with the mapping for the OpenFileInWorktree command.
  • This allows to set <cr> as the mapping to jump to the current version of the file.
  • Defaulted to false, i.e., preserve existing behavior (same as magit).

@txemaotero
Copy link
Contributor Author

I see tests failed but due to unrelated changes. I've checked locally that they also fail with the code in master.

@txemaotero
Copy link
Contributor Author

txemaotero commented Nov 26, 2025

Failure in unit tests introduced here. Proposing a fix in this PR.

@txemaotero txemaotero force-pushed the improve/interchange_cr_on_commit_view_to_jump_file_current_worktree branch from 2c318ff to 70f7318 Compare November 29, 2025 20:12
@txemaotero
Copy link
Contributor Author

I don't think the test failure is related. It seems an error due to a tight timing:

       Expected [" Stashes (1)                                                                    ", "stash@{0} On master: test                                           1 second ago"]
          to eq [" Stashes (1)                                                                    ", "stash@{0} On master: test                                          0 seconds ago"]

@CKolkey
Copy link
Member

CKolkey commented Nov 30, 2025

Yea, the e2e tests are a bit weird. They run way more reliably locally than on CI, and I don't know why... and debugging that feels more like "work" than "fun".

Anyways, instead of boolean configs like this, I'd rather solve it with mappings more like how the status buffer is set up: { "key" = "ActionNane" }

I think thats more flexible - having a load of boolean configs seems like a road to madness - as well as consistent.

- New `commit_view.cr_on_hunk_jumps_to_current` option to interchange
  the behaviour of `<cr>` with the mapping for the `OpenFileInWorktree`
  command.
- This allows to set `<cr>` as the mapping to jump to the current
  version of the file.
- Defaulted to false, i.e., preserve default magit behaviour.
@txemaotero txemaotero force-pushed the improve/interchange_cr_on_commit_view_to_jump_file_current_worktree branch from 70f7318 to 1eee02b Compare November 30, 2025 21:59
@txemaotero
Copy link
Contributor Author

I think thats more flexible - having a load of boolean configs seems like a road to madness - as well as consistent.

I completely agree with that. The thing is that I discarded initially that approach because I was reusing a mapping for the "open file in commit" functionality (<cr>, if hit on a file path line, jumps to that section in the commit view). This is similar to what is done with the GoToFile mapping in the status view (depending where you hit <cr> you can open the commit view or jump to the file). However, for this use case, I didn't want to define a new map that overrides both the "open the file" and the "jump to the hunks section" behaviors. As I didn't see anything in the code doing this kind of decoupling, I decided to go with the boolean approach to minimize code changes.

That said, after your feedback, I decided to give it a try and implement a logic to allow clashing mappings that target different components in the commit view. With that infrastructure, we can now define a new mapping for "OpenFileInCommit" that can be set to <cr>. Let me explain what I did to facilitate the review:

  • Declare mappings before assigning to the buffer creation options
  • Remove from that declaration the definition of mappings for <cr> and commit_view_maps["OpenFileInWorktree"].
  • Attach those removed mappings via the attach_jump_mappings function. This function is the one that handles the combination of multiple mappings with the same key but with different actions, each targeting different components in the view. In other words, we can attach one behavior to <cr> if the cursor is on a file path and a different one if we are on a hunk.

If you think this is too complicated just let me know and we can discuss a better way 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants