`get_mode_info` based on keybinds #319

horasal · 2021-04-22T10:38:25Z

This pr changes the default static get_mode_info to dynamically strings generated from keybinds.

Now get_mode_info accepts Keybind and will search Key for each Action. key is handled by the following rules:

order: Left/Right arrow > up/down arrow > pgup/pgdn > other. e.g. if both "→" and "L" exists for the same action, only "→" will be shown.
if a action has multiple arms (e.g. Resize has 4 arms Left/Right/Up/Down), all arms will be shown
if any arm has more than one character(e.g. PGDN, Ctrl-L, etc.), arms will be joined with "/"

TheLostLambda

This is excellent to have! Another thing that's been on my todo list!

I've left a couple of comments for consideration and might like to get some second opinions about the Display implementation, but things are looking great so far 😄

The status bar plugin might also need the mode removed where it does this:

The <p>ane thing won't really work once that key is remapped. Try shrinking your terminal to see it switch to this mode :)

zellij-tile/src/data.rs

src/common/input/handler.rs

horasal · 2021-04-23T10:35:16Z

Now Action is a part of zellij-tile
get_mode_info is moved to status-bar plugin
Mode Update will pass a Key / Action map instead of Strings. (in a vec, because of json's limitation)

horasal · 2021-04-24T00:40:06Z

I'm also working on status bar improvement, but it may take a while horasal@18ff3a7

TheLostLambda · 2021-04-26T17:02:45Z

Hey @horasal ! Thanks for doing all of this! Is this ready for a review or are you looking to finish your status-bar improvements first?

horasal · 2021-04-26T23:09:41Z

@TheLostLambda
This is ready for a review :) !

Status-bar improvements needs many extra works so I decide to file another pr for that

TheLostLambda

Overall looking great! I would personally simplify the const fn into a HashMap stored in State and would merge the actions module into the data module, but aside from that, this looks ready to merge!

If you want to sort out the merge conflicts and address those two points, I'm happy to press the button for you!

TheLostLambda · 2021-04-28T16:23:10Z

default-plugins/status-bar/src/mode_info.rs

+use std::collections::HashMap;
+
+// const fn now does not support PartialEq/Eq, we have to implement our own compare fn
+const fn compare_key(l: &Key, r: &Key) -> bool {


These are really cool (and I also realize I told you to try using these when this code was still in Zellij 😅 ), but if you'd like, the State struct here is secretly stored in a lazy_static! already! You could go back to your shorter first solution and pop the HashMap in there and implement either Default or populate it during the call to load().

Sorry again to do the 180 on you here, but that would probably clean things back up a little!

I tried to put key_order in State. However, it's necessary to pass Hashmap everywhere and it's really annoying. I prefer a lazy_static in mode_info.rs or just use current const. How do you think about it? or is there any better way to pass Hashmap around?

I'll give this a more thorough review today, but I think this vastly simplifies these two const fns into one! (Let me know if this actually is not doing the right thing 😅)

const fn get_key_order(key: &Key) -> Option<i32> { match key { Key::Left | Key::Right => Some(0), Key::Up | Key::Down => Some(1), Key::PageUp | Key::PageDown => Some(2), Key::Char(_) => Some(3), _ => None, } }

Match doesn't actually need Eq to function in this basic manner :)

you're right. this looks awesome!

TheLostLambda · 2021-04-28T16:32:04Z

zellij-tile/src/lib.rs

@@ -1,3 +1,4 @@
+pub mod actions;


I'd personally merge this file in with the data module, but if you disagree, I'm happy to chat about it!

Merged into data 9c0767a

TheLostLambda · 2021-04-28T16:33:07Z

zellij-tile/src/data.rs

+    }
+}
+
+impl Display for Key {


Looks great!

TheLostLambda · 2021-04-28T16:33:09Z

zellij-tile/src/data.rs

@@ -71,8 +105,7 @@ impl Default for InputMode {
 #[derive(Default, Debug, Clone, Serialize, Deserialize)]
 pub struct ModeInfo {
    pub mode: InputMode,
-    // FIXME: This should probably return Keys and Actions, then sort out strings plugin-side
-    pub keybinds: Vec<(String, String)>, // <shortcut> => <shortcut description>
+    pub keybinds: Vec<(Key, Vec<Action>)>,


This looks awesome! You said the HashMap had some issues with JSON, right?

Yes, Json can not serialize HashMap<Key, ..> because Key will be serialized as a map but a map can not be a key in json.

horasal · 2021-04-29T09:07:26Z

rebased
The conflict is a bit complicate. I carefully fixed them to make sure not break anything but this pr should be carefully reviewed.

TheLostLambda

This is coming along well! I think there is a bit of behavior that we need to fix though. Currently, the first part of the status bar will always show the defaults and never custom keybinds (as far as I can tell).

Also, this mode:

Still needs removing I think!

Sorry this is taking so long, it's a quite complex change, but certainly an important one!

TheLostLambda · 2021-04-29T11:57:37Z

default-plugins/status-bar/src/mode_info.rs

+use std::collections::HashMap;
+
+// const fn now does not support PartialEq/Eq, we have to implement our own compare fn
+const fn compare_key(l: &Key, r: &Key) -> bool {


I'll give this a more thorough review today, but I think this vastly simplifies these two const fns into one! (Let me know if this actually is not doing the right thing 😅)

const fn get_key_order(key: &Key) -> Option<i32> { match key { Key::Left | Key::Right => Some(0), Key::Up | Key::Down => Some(1), Key::PageUp | Key::PageDown => Some(2), Key::Char(_) => Some(3), _ => None, } }

Match doesn't actually need Eq to function in this basic manner :)

horasal · 2021-04-30T03:28:20Z

This is coming along well! I think there is a bit of behavior that we need to fix though. Currently, the first part of the status bar will always show the defaults and never custom keybinds (as far as I can tell).

Also, this mode:

Still needs removing I think!

Sorry this is taking so long, it's a quite complex change, but certainly an important one!

I totally agree with you. I'm working on this in another branch. but there are many things to be done, e.g.

Normal Mode do not contain keybinds so nothing will be shown on startup
Keybind does not contain key for current mode
decide how to handle keybinds with different prefix (e.g. Ctrl-G for lock but Alt-P for Pane mode)

I will file another PR once I make some progress!

EDIT: or do you think this improvement should also be done in this pr?

TheLostLambda · 2021-04-30T09:24:01Z

Thanks for all of your continued hard work on this!

I think it might be a little confusing if we had a half-functional rebinding UI like this in main, so I'd personally like to combine this PR with the status-bar fixes, but I know that might make for a big diff.

Let me know if you'd be up for that!

horasal · 2021-04-30T11:14:11Z

make first line generated from keybinds
make second line generated from keybinds
adjust prefix based on keybinds

horasal · 2021-05-01T01:42:21Z

@TheLostLambda
Now everything is done & ready for a review!

status bar now correctly show new keybinds:

TheLostLambda

This is great! I really like the new status bar behavior! It does seem like we've lost a little bit of spacing though if you'd like to add that back in. It may be hard to see without the proper fonts installed.

Current main:

This PR:

The spacing before Ctrl and around each item of the first line seems to have gone missing.

Additionally, were there some complications with including the [] keybindings in the tip?

Thanks again for all of your hard work! This is incredible stuff!

horasal · 2021-05-02T00:47:05Z

fix style, add [] navigate.

Now

if navigate key only one char, do not show "/"
works well even when some keybind is missing
add some spaces

horasal · 2021-05-02T00:53:24Z

rebased

horasal · 2021-05-02T01:06:26Z

@TheLostLambda
do you know why is the tests failing?
it seems work well on my local pc

TheLostLambda · 2021-05-02T09:14:41Z

@horasal Not sure about the tests! I've rerun them a couple of times and it seems to fail on different resizing tests... I'll poke @imsnif to see if he has any idea (it looks like the resizing snapshots recently changed somehow in main.

imsnif · 2021-05-02T18:39:13Z

I'll poke @imsnif to see if he has any idea (it looks like the resizing snapshots recently changed somehow in main.

Unfortunately our tests are a little flaky at times, especially on the CI :( It's on my TODO. I re-ran them and they pass now.

horasal · 2021-05-03T10:35:31Z

@TheLostLambda
I think all works are done and ready to be merged.
However, the conflicts are too complicated because 98d9eac introduced color theme, but that patch seems start from half of this pr (only contain the very first version of dynamic keybind).

I'm wondering if anyone (maybe @denismaxim0v can provide some hints?) can resolve these conflicts.

p.s. I'm pretty sure that we can just change the color of ShortCutTextElement::to_styled_text to achieve this but I haven't got enough time to follow color theme commits.

impl ShortCutTextElement {
    fn to_styled_text(&self, style: CtrlKeyMode) -> ANSIGenericString<str> { // all colors are defined in this function
        match (style, self) {
            (CtrlKeyMode::Unselected, ShortCutTextElement::Prefix) => {
                Style::new().fg(GRAY).on(BRIGHT_GRAY) // just change things here
            }

imsnif · 2021-05-04T16:19:08Z

@TheLostLambda
I think all works are done and ready to be merged.
However, the conflicts are too complicated because 98d9eac introduced color theme, but that patch seems start from half of this pr (only contain the very first version of dynamic keybind).

I'm wondering if anyone (maybe @denismaxim0v can provide some hints?) can resolve these conflicts.

p.s. I'm pretty sure that we can just change the color of ShortCutTextElement::to_styled_text to achieve this but I haven't got enough time to follow color theme commits.
impl ShortCutTextElement {
    fn to_styled_text(&self, style: CtrlKeyMode) -> ANSIGenericString<str> { // all colors are defined in this function
        match (style, self) {
            (CtrlKeyMode::Unselected, ShortCutTextElement::Prefix) => {
                Style::new().fg(GRAY).on(BRIGHT_GRAY) // just change things here
            }

Hey @horasal - I'm afraid I now had to change things even further in regards to color themes and the api of the plugins!
I have a lot on my plate now, but I can try to give input to help you resolve the conflicts. Is there anything specific that is causing you trouble?
(btw, in such a case I personally like to forgo the merge and just reapply my changes with the new world order - I find it's quicker! But however is convenient for you, of course)
This code-base is moving quite fast these days! It can be frustrating, I know - but maybe also adventurous? :D

a-kenji · 2022-07-10T10:56:44Z

Closing in favor of #1242, thanks @horasal for starting this work!

horasal changed the title ~~get_mode_info according to keybinds~~ get_mode_info based on keybinds Apr 22, 2021

TheLostLambda reviewed Apr 23, 2021

View reviewed changes

zellij-tile/src/data.rs Outdated Show resolved Hide resolved

src/common/input/handler.rs Outdated Show resolved Hide resolved

src/common/input/handler.rs Outdated Show resolved Hide resolved

horasal marked this pull request as draft April 23, 2021 08:05

horasal marked this pull request as ready for review April 23, 2021 10:24

horasal mentioned this pull request Apr 23, 2021

Plugin API Requests #280

Open

12 tasks

a-kenji mentioned this pull request Apr 24, 2021

Feature(Status-Bar): React to Keybind Changes #246

Closed

TheLostLambda suggested changes Apr 28, 2021

View reviewed changes

horasal force-pushed the auto_mode_info branch from 9c0767a to ecfe900 Compare April 29, 2021 09:04

TheLostLambda suggested changes Apr 29, 2021

View reviewed changes

TheLostLambda suggested changes May 1, 2021

View reviewed changes

horasal added 10 commits May 2, 2021 09:51

show help info based on keybinds

264addd

make key order stable

f05088c

cargo fmt

44afdff

remove lazy_static and use const fn

ed08fc3

more informative Display for Key

4a9650c

move get_mode_info to default_plugin

d22401d

fix json serialize

47c7de8

cargo fmt

7bf01a2

merge action into data

a37805d

resolve conflit with main

be95ec4

horasal added 12 commits May 2, 2021 09:51

cleanup get_key_order

d67d9f9

cargo fmt

30b4b9b

generate status_bar based on keybinds

8268869

generate first line based on keybinds

c8b8201

cargo fmt

76c0aa7

dynamic generate prefix

566c9cd

shortened text should be lowercase

c06fc60

capitalize shortened text

a409401

generate second_line

7d5849e

Update style

72ff457

cargo fmt

8846fe7

minor improvement

742e6f3

horasal force-pushed the auto_mode_info branch from 10789ba to 742e6f3 Compare May 2, 2021 00:53

a-kenji mentioned this pull request May 11, 2021

Config location for OS/X #485

Closed

a-kenji mentioned this pull request Sep 20, 2021

Update keybind tip when changed from defaults #734

Closed

a-kenji mentioned this pull request Mar 21, 2022

update tips/status bar accoring to config #1252

Closed

a-kenji closed this Jul 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`get_mode_info` based on keybinds #319

`get_mode_info` based on keybinds #319

horasal commented Apr 22, 2021 •

edited

Loading

TheLostLambda left a comment •

edited

Loading

horasal commented Apr 23, 2021

horasal commented Apr 24, 2021 •

edited

Loading

TheLostLambda commented Apr 26, 2021

horasal commented Apr 26, 2021

TheLostLambda left a comment

TheLostLambda Apr 28, 2021

horasal Apr 29, 2021 •

edited

Loading

TheLostLambda Apr 29, 2021

horasal Apr 30, 2021

TheLostLambda Apr 28, 2021

horasal Apr 29, 2021

TheLostLambda Apr 28, 2021

TheLostLambda Apr 28, 2021

horasal Apr 29, 2021

horasal commented Apr 29, 2021

TheLostLambda left a comment

TheLostLambda Apr 29, 2021

horasal commented Apr 30, 2021 •

edited

Loading

TheLostLambda commented Apr 30, 2021

horasal commented Apr 30, 2021 •

edited

Loading

horasal commented May 1, 2021

TheLostLambda left a comment

horasal commented May 2, 2021

horasal commented May 2, 2021

horasal commented May 2, 2021 •

edited

Loading

TheLostLambda commented May 2, 2021

imsnif commented May 2, 2021

horasal commented May 3, 2021 •

edited

Loading

imsnif commented May 4, 2021

a-kenji commented Jul 10, 2022

get_mode_info based on keybinds #319

get_mode_info based on keybinds #319

Conversation

horasal commented Apr 22, 2021 • edited Loading

TheLostLambda left a comment • edited Loading

Choose a reason for hiding this comment

horasal commented Apr 23, 2021

horasal commented Apr 24, 2021 • edited Loading

TheLostLambda commented Apr 26, 2021

horasal commented Apr 26, 2021

TheLostLambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

horasal Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

horasal commented Apr 29, 2021

TheLostLambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

horasal commented Apr 30, 2021 • edited Loading

TheLostLambda commented Apr 30, 2021

horasal commented Apr 30, 2021 • edited Loading

horasal commented May 1, 2021

TheLostLambda left a comment

Choose a reason for hiding this comment

horasal commented May 2, 2021

horasal commented May 2, 2021

horasal commented May 2, 2021 • edited Loading

TheLostLambda commented May 2, 2021

imsnif commented May 2, 2021

horasal commented May 3, 2021 • edited Loading

imsnif commented May 4, 2021

a-kenji commented Jul 10, 2022

`get_mode_info` based on keybinds #319

`get_mode_info` based on keybinds #319

horasal commented Apr 22, 2021 •

edited

Loading

TheLostLambda left a comment •

edited

Loading

horasal commented Apr 24, 2021 •

edited

Loading

horasal Apr 29, 2021 •

edited

Loading

horasal commented Apr 30, 2021 •

edited

Loading

horasal commented Apr 30, 2021 •

edited

Loading

horasal commented May 2, 2021 •

edited

Loading

horasal commented May 3, 2021 •

edited

Loading