Skip to content

feat: allow defining overriding and removing keyboard shortcuts #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

scrabsha
Copy link

@scrabsha scrabsha commented Apr 29, 2025

Part of #95.

The title says everything. The implementation is a bit primitive and many things are missing, but that's something we can iterate on :)

For what it's worth, here's the content of my .jj/repo/config.toml now:

[jjj.keys.change]
t = "move_visual_line_up"
h = "move_visual_line_down"
H = "extend_line_down"
j = "noop"
k = "noop"

I cherry-picked commit 59fd346 which written by you (@icorbrey) and started working over it. I removed some things that were too complicated for a first MR. If that's ok for you, we could tackle them later.

For what it's worth, my experience with Bevy is exactly 0, so please report anything that does not look correct.

Copy link
Owner

@icorbrey icorbrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! There are some larger conversations we need to have on how we want to structure modes and commands, but your work so far will help support this.

#[serde(untagged)]
pub enum KeyDefinition {
Command(Command),
// TODO: minor mode
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to flesh this out before merging. My goal is to be able to define the space menu as a minor mode rather than hard coding it, that way users are able to customize it to their liking (as well as to support future features like bookmark management)

#[serde(untagged)]
pub enum Command {
Static(StaticCommand),
// TODO: typable commands
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want this fleshed out before merging as a base for future features.

Comment on lines +71 to +89
fn resolve_command(config: &Config, key: char) -> Option<Command> {
config
.keys
.change
.get(&key)
.map(|key_def| {
let KeyDefinition::Command(command) = key_def;
command
})
.copied()
}

fn read_keys(
mut ev_selection: EventWriter<ChangeBufferSelectionEvent>,
mut change_buffer: Query<&mut ChangeBuffer>,
mut ev_keypresses: EventReader<KeyEvent>,
mut exit: EventWriter<AppExit>,
mut navigation: Navigation,
config: Res<Config>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't bad. I think a good next step is to extract this into its own event mapper (so that instead of listening to EventReader<KeyEvent> we can listen to e.g. EventReader<Command>.

}

Some(Command::Static(StaticCommand::MoveVisualLineUp)) => {
Some(IndexSelection::Single(usize::min(i.wrapping_sub(1), max)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid wrapping selections. We'll add goto commands later on via minor modes, a la gg and ge

}

Some(Command::Static(StaticCommand::ExtendLineUp)) => {
// TODO: I had a long day at work today - I have exactly zero clue what
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unresolving, as apparently i forgot to fix this 💀)

@icorbrey
Copy link
Owner

We'll also want to clean up commits before we merge 😁

@scrabsha scrabsha force-pushed the push-yvvvvlsntxku branch from 2f30023 to 7adedce Compare April 30, 2025 21:05
ExtendLineUp,

/// Does nothing. Allows to delete a binding.
#[serde(alias = "no_op")]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure this alias is necessary (the snake_case of NoOp is... no_op)

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