-
-
Notifications
You must be signed in to change notification settings - Fork 666
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
status-bar: Reflect actual current keybindings #1242
status-bar: Reflect actual current keybindings #1242
Conversation
A few more general things:
For the keybindings I'd prefer to do the first thing, because it will likely be most useful for other plugins when the need arises. Still working my way through the code to understand what this requires and where. But if you have another suggestion, please let me know! |
No worries, and sorry it's not more straightforward. Please feel free to ask questions/guidance here or on one of our chats. Would be happy to help get you unstuck.
That makes sense, it's what I'd opt for as well. |
I'm afraid I got a little lost on the way... No matter how I try to get the Keybinds into zellij-tile, it doesn't really work.
But right now I don't see a way around getting the Maybe I could integrate the Maybe I'm just blind or missing something obvious. Do you have an idea what else I could try? |
It isn't possible for the keybinds to change at runtime, right? I think in that case, since |
Hey @har7an - looking through this now I definitely agree it's a bit of a challenge the way things are. We're currently in the process of changing our configuration language from YAML to KDL and hopefully that'll help untangle some things there. For now - correct me if I'm wrong - I think what we need is essentially to create a struct like this: struct Shortcuts(HashMap<InputMode, HashMap<Key, Vec<Action>>>) The only problem I can see here is that struct Shortcuts(HashMap<InputMode, HashMap<Key, Vec<String>>>) And then implement |
Hmmm, yeah that may work! For reasons of overhead (since it's always the same data being transmitted) we could also drop the outer I'll try and replace the current |
Humm, that still doesn't do the trick... The issue is that I would somehow need to get the keybindings into the I generally don't really understand where the config goes, I see it's passed to In the meantime I've had some other thoughts. What about making a Also maybe we could put the currently active configuration into some static global location, wrapped into an And having looked into the code maybe I'll start a ZSDP before continuing here: The Zellij Sources Documentation Project. Personally I find it very hard to work with things I do not understand, and I'm not a seasoned programmer so the code alone doesn't tell me all I'd really like to know. ^^ |
Hey, so first about code quality and complexity: I very much hear what you're saying and can totally empathize. Working with an arcane and non-trivial code-base can be frustrating, and not being able to fully grasp basic concepts that you need in order to implement a seemingly simple feature can drive one mad. This is very much a pain point for us and I don't think anyone will deny it. Zellij is a volunteer led project, and a relatively large one at that. Because of the nature of the app, with users letting us into their daily work environment, we absolutely have to prioritize user experience over developer experience (a runtime panic is several orders of magnitude worse than code duplication or complex messaging paths). Add to that the fact that volunteers don't usually enjoy performing "upkeep" tasks such as mass refactoring and architecture changes, and you get more or less the state at hand. Zellij is also a beta still - meaning that any mass documentation effort is IMO doomed to fail. There have been some in the past (you can see them inside the code base itself), but without proper processes that will inevitably slow down development (see previous point) they grow stale. Every now and then, one of the maintainers tries to do some small refactoring task in order to fight back entropy - and as we get more and more stable, the situation improves. But we still have a ways to go :) The upside of all of this though, is that since Zellij is a volunteer project, the people who do work on it regularly are super pumped and enthusiastic to welcome and help others aboard. We think this is a great piece of software already and that it has massive potential. That means we do our absolute best to help onboard others and bring more people around the table. I personally find this specific feature incredibly important and am willing to devote a lot of time to help you succeed in implementing it. Even if it would end up being more time than I'd need to code it myself (though we're not there yet, ofc). So I'm happy to work through any lack of understanding you have, point you in the right direction and suggest solutions. I'm happy to keep going over this here, in one of our chat servers or even on a voice call. Let me know what you need in order to move forward and I'll do my best to oblige. About the issue at hand: I see the problem you're talking about with having a difficulty getting the shortcuts info into I think the best way to approach this would be to create the The |
I can absolutely relate to that, I very much enjoy not hitting runtime panics of course!
And I'd really like to take a seat at the table! I also see all the effort you and the other maintainers put into this, and e.g. kenji has been a tremendous help in extending the zellij documentation. Thank you for all the time and effort you devote to this!
That's very kind of you, but experiences like these make me feel as if I'm in sort of the wrong place, but I guess that's more of a personal issue of mine.
Since
Uhm, I think I'll have to read up on how multi-user sessions work before I fully understand the implications of this. How does Oh and thanks for taking the time to write such elaborate answers. I really appreciate it. :) |
For what it's worth - I don't think you're in the wrong place. But that's of course up to you. Right now Sorry for the extra bit of complexity with the I'd start implementing it (just so that you don't have so many more things on your mind) by ignoring the clients entirely and adding them in later. Maybe I jumped the gun with my recommendation above. It won't be very involved to add them, don't worry about it. |
Hey @har7an - do you think you'll be able to finish working on this sometime in the next week or two? |
Hi @imsnif! |
Thinking about this, I guess I should be honest with myself and admit that with everything I know about the code structure at this point this is probably too big for me. I think there are other things to hack at that are somewhat less involved. |
Hey @har7an - thank you for your honesty and openness. I very much appreciate all the work you've done on this. I'll be picking this up soon and will try to work off your branch as much as makes sense. |
a8a0d35
to
96719f5
Compare
@a-kenji Since imsnif is enjoying is vacation (I hope) I'm pinging you instead, I hope that's ok? I had a glance over the code and both the Now I tried to move the Another alternative would be to add another crate with shared content that is compatible with |
The rationale behind this is that all components of zellij access the data structures defined in this module, as they define some of the most basic types in the application. However, so far zellij-tile is treated like a separate crate from the rest of the program in that it is the only one that doesn't have access to `zellij-utils`, which contains a lot of other data structures used throughout zellij. This poses issues as discussed in zellij-org#1242 and is one of the reasons why the keybindings in the status bar default plugin can't be updated dynamically. It is also the main reason for why the keybindings are currently passed to the plugin as strings: The plugins only have access to `zellij-tile`, but since this is a dependency of `zellij-utils`, it can't import `zellij-utils` to access the keybindings. Other weird side-effect are that in some places `server` and `client` have to access the `zellij-tile` contents "through" `zellij-utils`, as in `use zellij_utils::zellij_tile::prelude::*`. By moving these central data structures to one common shared crate (`zellij-utils`), `zellij-tile` will be enabled to import `zellij-utils` like `screen` and `client` already do. This will, next to other things, allow dropping a lot of `std::fmt::Fmt` impls needed to convert core data structures into strings and as a consequence, a lot of string parsing in the first place.
@har7an, I am sure you can find the exact reasoning from @TheLostLambda somewhere in the repo.
I disagree here. I don't think that this is in general silly. I believe we could benefit substantial from exposing more individual crates. |
Uhm, okay in that case should I scratch #1541 and start again? I played around optional dependencies and feature flags, and it seems to work. It compiles fine on my end and it passes all the tests. I'm not proposing to import all of |
I think in this case it might be fine. |
@har7an, |
Keeping #1242 in mind, how would the content of this new data crate then access the Edit: I mean: There's no end to how many components you move out of |
I am not home yet, so I don't have a very good overview at the moment. In that case maybe clean the PR up a little more and we can later look at how to structure the crates more efficiently. In particular it seems that you have unintentional changes in the PR? |
I'm not in a rush, take your time.
None that I'm aware of. I did a bit of code formatting in some of the larger Anyways, that's why I'm already looking forward to your feedback. ^^ |
#1541 |
|
Then I am confused now too, maybe GH is confused and showing me something wrong. |
Hi, not back yet (on the train now) but please don't add more crates.
Sorry to interject here, but I don't want you to do extra work.
The release process is error prone and generally unstable as is and more
crates will make it worse. We're actively working on reducing the number of
crates (ideally to just the one).
…On Fri, Jun 24, 2022, 2:16 PM a-kenji ***@***.***> wrote:
Then I am confused now too, maybe GH is co fused and showing me something
wrong.
—
Reply to this email directly, view it on GitHub
<#1242 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGCHTSIOMYCFBMTYSVLYPDVQWRITANCNFSM5RENX2QQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
with what the status bar now really displays.
to the more descriptive name 'mode_shortcut', which better describes what this function does.
where the modifier would be shows as `Ctrl +`, without a trailing space. This isn't an issue in regular mode, where we have the spacing from the arrow gaps (`>>`) that "simulates" this effect.
so it doesn't rely on some "external" index for operation any more.
and fix a bug in the process where certain `Ctrl` keybindings would be displayed wrong.
responsible for printing the long and short shortcut keyhint tiles. Also add some documentation that explains their purpose and the arguments they accept.
introduced when rewriting `SplitSize::Percent` to not hold an `f64` type.
We use regular expressions to strip all ANSI escape sequences in the strings that are produced by the plugin functions during testing. We do not test for the style information, but merely for the raw text.
b47946d
to
80a84a3
Compare
Rebased onto main. |
Great work on the tests!
I played with it a little bit, and it seems that if I comment out the (just to make sure we understand each other: I don't know what the solution to both of these problems is - just wanted to break it down and give you a direction).
Here are some instructions about running the e2e tests locally: https://github.com/zellij-org/zellij/blob/main/CONTRIBUTING.md#running-the-end-to-end-tests I'd recommend taking the most basic test: https://github.com/zellij-org/zellij/blob/main/src/tests/e2e/cases.rs#L81
Alright - in this case I'm afraid I'm going to send you to do some homework here. I'm sorry, I know we're very much near the end and I'm piling up some more stuff on you, but I'm not confident making this big of an infrastructure change without understanding it. Could you please try to invest some time in finding out why exactly this is needed? What happened to require this change? We're almost there! Let's do this :) |
That's correct, but in that case it'll also build the plugin for your host target triple and not I've had a look and it seems that while Profiles define many settings, specifying a target triple isn't one of these. The cargo configuration (via I found an open issue against rust about this, where someone started implementing it and decided adding multiple different targets as configuration option is confusing. The gist of the discussion there is "use cargo-make". I see three ways out of this situation, I dislike all of them:
Thanks for the feedback! I'll work on the e2e test and |
I think 2 (or ideally some simplified version of 2) would indeed be the best option here. I'm afraid I also don't have a lot of context on I don't know if this makes sense, but if you can even add another task to the testing task that would run the tests with About bincode - I totally get you. It's frustrating and annoying, I know. Just imagine how less frustrating it would be to find out what's going on now (and ideally go back to bincode) than to find out 2 days after release that for some reason the new crate doesn't work with certain architectures. :) I of course respect you getting to this with you can, but I want to plan the release. Do you think you'll manage to get to it sometime this week, or shall we postpone this to the next release? |
Yeah all right, I can imagine. :P
Pretty sure I'll manage, yes. Let's say that if you haven't heard back until Friday you go ahead and we pull this in for the next release, is that okay? But I'm rather confident I'll have news for you by tomorrow evening. |
Sounds great. |
This allows the unit tests for all plugins to be run on the host as well (because their default compilation target is wasm32-wasi).
in the status bar. Makes sure that the modified bindings from a custom configuration file are read and applied to the UI.
efddae6
to
e2544bd
Compare
@imsnif I may have solved the bincode riddle. Allow me to present what I have gathered so far:
As per this issue, it is mentioned that Unfortunately the
I grepped the codebase and we have a total of 9 "forbidden" serde attributes in the code:
All the files under In #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)]
#[serde(untagged)] // <-- BAD!
pub enum CharOrArrow {
Char(char),
Direction(Direction),
} And in the
So do we need this? It very much looks like it. When I undo the offending commit that replaced
I also tried adding other |
@har7an - good find!!! This is actually very interesting, and it seems like we never hit this issue before because we weren't transferring any serde-untagged stuff across the wasm boundary. Alright, let's move forward with rmp-serde. Looking further into it, looks like this is a pure rust crate that hopefully won't cause us any surprises. I really appreciate you diving so deep into this and enjoyed reading your analysis. What else do we have left? |
Thank you for the kind words!
By now I think we have covered pretty much everything. Only thing that's not implemented at this point is how to correctly pass the keybindings to zellij, because all clients connecting to the same session will only see the keybindings of the first client that connected. Which is still an improvement over the current status bar in any case. 😂 Also I think that finding a good way to deal with this either involves fiddling with the plugin system (Which we shouldn't do anymore until the plugin system has been revised) or is ideally handled entirely inside Edit: And looking back through the discussion here we have:
|
Great. About multiple users only seeing the first user's keybindings: we have this same problem in lots of other areas (eg. pixel to cell ratio), it's a known quantity so it's fine for this to not work in the same way. This too lies behind the pile of our technical debt and we'll get to it at some point :) Right - this looks good to me (I also went over the e2e test). So I'm going to go ahead and merge this. Once again - thank you for your meticulous work on this. I'm very happy you didn't give up and came back to this. You did a great job! |
@imsnif Thank you for mentoring me, guiding me through the whole process and taking the time and patience to answer all my questions and explain things I didn't understand. I'm happy to contribute to this project! |
Make the
status-bar
plugin reflect the currently active key bindings.At the moment, the default key bindings are hard-coded into the plugin for the first line which indicates how to switch modes. This PR attempts to parse the currently active keybindings in the plugin and make the available in the status line instead.
Currently this will focus mostly on the first line of the status bar, but extending it to the second line is probably possible, too. Ideally, it will recognize if all keybindings use the same modifier and the print that as
superkey
(at the beginning of the line), while intelligently shortening the key hints the way it is currently done.