Adds configurable keymaps for navigation movements #77

LukeHalasy · 2023-09-29T23:37:38Z

Adds the ability to customize keymaps pertaining to navigation. First part of #35

LukeHalasy · 2023-09-29T23:39:33Z

Am I heading in the right direction with this? If so, I can continue on with setting up configurable keymaps for all actions/commands.

src/config.rs

Piturnah

All the configuration stuff looks great!

I did leave a couple of comments, the main thing at the moment is just that there is a bit of unnecessary repetition of logic (which can be fixed easily)

src/main.rs

src/config.rs

Piturnah · 2023-09-30T13:21:11Z

After thinking a bit further, I've realised another problem:

There are certain actions in gex that are bound to multiple keys. For example, to toggle expand, you can press either space or tab. I think that this should be preserved in the configuration implementation

If you could provide a list of keys, e.g.

toggle_expand = [ "Tab", " " ]

You could look up the relevant action thusly:

if config.keymap.navigation.toggle_expand.contains(&c1) {
    state.status.expand()?;
}

At this point, it does feel that since there are so many actions it may be worth the overhead of spinning up a hashmap. Then the API would look something like:

match config.keymap.navigation.get(keycode) {
    Action::ToggleExpand => state.status.expand()?,
    _ => todo!()
}

I think this approach makes the most sense, but it would make the deserialisation logic more involved as you would need an intermediate type (or custom Deserialize impl on Keymap).

What do you think?

LukeHalasy · 2023-09-30T14:50:55Z

After thinking a bit further, I've realised another problem:

There are certain actions in gex that are bound to multiple keys. For example, to toggle expand, you can press either space or tab. I think that this should be preserved in the configuration implementation

If you could provide a list of keys, e.g.
toggle_expand = [ "Tab", " " ]
You could look up the relevant action thusly:
if config.keymap.navigation.toggle_expand.contains(&c1) {
    state.status.expand()?;
}
At this point, it does feel that since there are so many actions it may be worth the overhead of spinning up a hashmap. Then the API would look something like:
match config.keymap.navigation.get(keycode) {
    Action::ToggleExpand => state.status.expand()?,
    _ => todo!()
}
I think this approach makes the most sense, but it would make the deserialisation logic more involved as you would need an intermediate type (or custom Deserialize impl on Keymap).

What do you think?

I think it makes sense to be able to provide a list of keys. That way we don't have hardcoded defaults like we do now with the current approach (where the arrow keys are hard-coded in).

With this approach will the user always provide a list ?

Like would you have to do
move_down = ['j']
if you only wanted j mapped to move_down ?

I'm all for spinning up a hashmap.

LukeHalasy · 2023-09-30T14:59:51Z

I appreciate the thorough review + suggestions. Still very new to Rust so I appreciate the explanations.

LukeHalasy · 2023-09-30T16:18:26Z

Another question: What's the difference between an action and a command ?

Piturnah · 2023-09-30T19:23:17Z

With this approach will the user always provide a list ?

This way would be the most straightforward to implement, but it's not necessary. You would just need a custom Deserialize impl. See Serde docs.

If you don't want to implement this, then the best way would be to have the user always provide a list, as then in future the "from single" implementation can be added in a way that's easily backwards compatible.

Another question: What's the difference between an action and a command ?

A command in gex is kind of vaguely defined as it's just an abstraction that sort've naturally occurred while I was working on it. It is a subset of an action, and one that performs some kind of operation on the repository state. This would be a sufficient distinction if it weren't for the fact that staging operations are not considered commands. (or Fetch, currently, but Fetch is in a weird place at the moment, pending refactors that I have locally)

The main reason for the distinction is practical -- the commands tend to have "subcommands" whereas an action does not. In fact, currently all commands have subcommands. For this reason their implementation is separated so that I could do some work to make implementing new ones more straightforward (hence the command macro)

I'm all for spinning up a hashmap.

Great!

LukeHalasy · 2023-10-02T00:17:02Z

Took me a while, but I figured out how to write the custom deserialize method.

This PR is still incomplete (need to uncomment the text, update README.md, update other navigation movements (in the branch section)).

I will hold off on completing that stuff until I can figure out how to deserialize the KeyCode from forms other than just Char + get the flow I have here reviewed.

src/config.rs

Piturnah

Looks like we are heading in the right direction!

src/config.rs

…eserialization logic

…gle char

lists

action

deserializing

branch window

LukeHalasy · 2023-10-06T21:03:45Z

Think v1 of this is done.
Will be confusing to think through how to add customizable keymaps for commands + actions

Piturnah

This is all looking great, and we're almost ready to merge.

The most important thing here is the comment I left about filling in default values, as soon as that's resolved, nothing else is really blocking and I think it's good to go!

Take a look at the strum crate, which provides some handy derivable constructs for enums. I haven't yet given a lot of thought as to how specifically you could fix the issue, but I do have a hunch that the EnumIter derive could be helpful.

src/main.rs

src/config.rs

that action in their config.

LukeHalasy · 2023-10-08T00:20:49Z

src/config.rs

@@ -171,6 +171,9 @@ impl<'de> Deserialize<'de> for Keymaps {
                                de::value::StringDeserializer::new(action),
                            )?;

+                            // over-write default key-map to action
+                            navigation.retain(|_, value| value != &ac);


Is there a less jank way to do this?

Looks fine to me, only happens once anyway!

LukeHalasy · 2023-10-08T00:31:35Z

Implemented requested changes.

It will be interesting to think through how to refactor this when we add keymaps for actions/commands.
Would love recommendations before I work on that PR (to follow this one up)

Piturnah

All looks good. Thanks for your hard work, and for implementing my feedback! Sorry if I've been a bit slow to respond to things.

There's one point, but it's so minor that I'm happy to just merge this and fix it in another commit, so we can get this closed.

It will be interesting to think through how to refactor this when we add keymaps for actions/commands.

I have a bit of a refactor in the works for the command system anyway to make it a bit more coherent, which should be coming soon. So, it might make sense to hold off on this for now as it's likely to change

Piturnah · 2023-10-09T13:46:01Z

src/config.rs

+#[allow(unused_imports)]
+use strum::{EnumIter, IntoEnumIterator};


If we're only using strum in the unit test, you can move it to dev-dependencies, and move this use statement into the test module

Piturnah · 2023-11-12T20:11:44Z

Released in 0.6.4 🎉

LukeHalasy commented Sep 29, 2023

View reviewed changes

src/config.rs Show resolved Hide resolved

Piturnah reviewed Sep 30, 2023

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

LukeHalasy commented Oct 2, 2023

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

LukeHalasy commented Oct 2, 2023

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

LukeHalasy requested a review from Piturnah October 2, 2023 00:22

Piturnah reviewed Oct 3, 2023

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

Piturnah mentioned this pull request Oct 3, 2023

Tracking: gex config #36

Open

7 tasks

LukeHalasy added 12 commits October 5, 2023 14:00

Adds configurable keymaps for navigation movements

46105ca

Changes navigation keymaps to a hashmap instead of a struct. Custom d…

9e209fe

…eserialization logic

Changes configurable keymaps to take a list of chars instead of a sin…

736bc67

…gle char

allows specifying both chars and KeyCode enum strings within the keycode

34d68ee

lists

don't panic (todo!) when no match occurs with a specified navigation

d35b6a8

action

Correct defaults for KeyMaps

f0ad446

adds context comment surrounding why we must detect characters before

8e467b2

deserializing

fix lint issues

19e9967

add moveup and movedown as configurable keycommand options for the

69183f2

branch window

updates README documentation config test

24c4524

merge in recent changes

16a04c0

fix linting issues

7af7fad

LukeHalasy force-pushed the configurable-keymaps branch from 08e30ee to 7af7fad Compare October 5, 2023 18:14

LukeHalasy added 2 commits October 5, 2023 14:20

delete commented out block

73abe8a

Adds expecting block to the KeyMaps deserializer

4ee9411

LukeHalasy marked this pull request as ready for review October 5, 2023 18:26

LukeHalasy requested a review from Piturnah October 5, 2023 18:30

Piturnah requested changes Oct 7, 2023

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/config.rs Show resolved Hide resolved

src/config.rs Show resolved Hide resolved

LukeHalasy added 5 commits October 7, 2023 20:09

add strum crate

0dcdef3

Match configurable GotoTop and GotoBottom actions in the branch list

a8f279c

Use default keymappings for an action if a user didn't add a mapping for

ad6e296

that action in their config.

Adds a unit-test to ensure that every Action has a default keybinding

9f74a7f

fix clippy warnings

c2663d2

LukeHalasy commented Oct 8, 2023

View reviewed changes

LukeHalasy requested a review from Piturnah October 8, 2023 00:21

Piturnah approved these changes Oct 9, 2023

View reviewed changes

Piturnah merged commit 8399a23 into Piturnah:main Oct 9, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds configurable keymaps for navigation movements #77

Adds configurable keymaps for navigation movements #77

LukeHalasy commented Sep 29, 2023 •

edited

Loading

LukeHalasy commented Sep 29, 2023 •

edited

Loading

Piturnah left a comment

Piturnah commented Sep 30, 2023 •

edited

Loading

LukeHalasy commented Sep 30, 2023

LukeHalasy commented Sep 30, 2023

LukeHalasy commented Sep 30, 2023

Piturnah commented Sep 30, 2023

LukeHalasy commented Oct 2, 2023

Piturnah left a comment •

edited

Loading

LukeHalasy commented Oct 6, 2023

Piturnah left a comment

LukeHalasy Oct 8, 2023

Piturnah Oct 9, 2023

LukeHalasy commented Oct 8, 2023 •

edited

Loading

Piturnah left a comment

Piturnah Oct 9, 2023

Piturnah commented Nov 12, 2023

		#[allow(unused_imports)]
		use strum::{EnumIter, IntoEnumIterator};

Adds configurable keymaps for navigation movements #77

Adds configurable keymaps for navigation movements #77

Conversation

LukeHalasy commented Sep 29, 2023 • edited Loading

LukeHalasy commented Sep 29, 2023 • edited Loading

Piturnah left a comment

Choose a reason for hiding this comment

Piturnah commented Sep 30, 2023 • edited Loading

LukeHalasy commented Sep 30, 2023

LukeHalasy commented Sep 30, 2023

LukeHalasy commented Sep 30, 2023

Piturnah commented Sep 30, 2023

LukeHalasy commented Oct 2, 2023

Piturnah left a comment • edited Loading

Choose a reason for hiding this comment

LukeHalasy commented Oct 6, 2023

Piturnah left a comment

Choose a reason for hiding this comment

LukeHalasy Oct 8, 2023

Choose a reason for hiding this comment

Piturnah Oct 9, 2023

Choose a reason for hiding this comment

LukeHalasy commented Oct 8, 2023 • edited Loading

Piturnah left a comment

Choose a reason for hiding this comment

Piturnah Oct 9, 2023

Choose a reason for hiding this comment

Piturnah commented Nov 12, 2023

LukeHalasy commented Sep 29, 2023 •

edited

Loading

LukeHalasy commented Sep 29, 2023 •

edited

Loading

Piturnah commented Sep 30, 2023 •

edited

Loading

Piturnah left a comment •

edited

Loading

LukeHalasy commented Oct 8, 2023 •

edited

Loading