Skip to content
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

Merged
merged 90 commits into from
Jul 27, 2022

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Mar 19, 2022

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.

@har7an
Copy link
Contributor Author

har7an commented Mar 19, 2022

A few more general things:

  • At the moment I find the code to be a little intransparent in certain spots, still working to figure it out. So I'll likely be many commits behind master while still working on the basic feature, I'll rebase again once it works. :)
  • The biggest issue ATM is to get all of the keybindings into the plugin, either by:
    • Passing the actual Keybinds struct (which requires new dependencies)
    • Passing the keybindings as some String/Key vector to circumvent the dependency thing
    • Reading the keybindings from the config like the main program does

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!

@imsnif
Copy link
Member

imsnif commented Mar 21, 2022

A few more general things:

* At the moment I find the code to be a little intransparent in certain spots, still working to figure it out. So I'll likely be many commits behind master while still working on the basic feature, I'll rebase again once it works. :)

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.

* The biggest issue ATM is to get all of the keybindings into the plugin, either by:
  
  * Passing the actual `Keybinds` struct (which requires new dependencies)
  * Passing the keybindings as some String/`Key` vector to circumvent the dependency thing
  * Reading the keybindings from the config like the main program does

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!

That makes sense, it's what I'd opt for as well.

@har7an
Copy link
Contributor Author

har7an commented Apr 12, 2022

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.

  1. I can't add zellij-utils as extra dependency to zellij-tile because that creates a cyclic dependency
  2. Nor can I move the keybinds.rs from zellij-utils to zellij-tile because they are tightly integrated with other contents of zellij-utils (Such as the config submodule), which would require me to include zellij-utils to resolve which I can't do because of 1.

But right now I don't see a way around getting the keybinds into zellij-tile, because the information I can comfortably access is part of the ModeInfo struct, which is defined in zellij_tile::data...

Maybe I could integrate the keybinds into the zellij-tile-utils, but that would make the tile-utils a hard dependency for zellij-tile, which seems like a silly idea.

Maybe I'm just blind or missing something obvious. Do you have an idea what else I could try?

@har7an
Copy link
Contributor Author

har7an commented Apr 12, 2022

It isn't possible for the keybinds to change at runtime, right? I think in that case, since zellij-utils is already a dependency for the status-bar, I could create a separate instance of Keybinds just for the plugin to work with. But I'm not really sure how to create the Keybinds struct from the user configuration given the functions in the module... Where exactly are these instantiated and how?

@imsnif
Copy link
Member

imsnif commented Apr 12, 2022

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 Action is part of zellij-utils and so will have the same problem. Luckily though, the plugin doesn't really need the Action, but rather a String representation of it, right? So maybe we can do something like:

struct Shortcuts(HashMap<InputMode, HashMap<Key, Vec<String>>>)

And then implement fmt for Actions and send it along? We'll have to do some ugly String comparison on the plugin side, but other than that bit of hackery I think this will do the trick, right? Then what we'll need to do is define Shortcuts in zellij-tile, define the conversion from one type to the other in zellij-utils and we're good. Or am I missing something?

@har7an
Copy link
Contributor Author

har7an commented Apr 12, 2022

So maybe we can do something like:

struct Shortcuts(HashMap<InputMode, HashMap<Key, Vec<String>>>)

And then implement fmt for Actions and send it along?

Hmmm, yeah that may work! For reasons of overhead (since it's always the same data being transmitted) we could also drop the outer HashMap and only transmit the HashMap<Key, Vec<String>>. We can then assemble the outer HashMap as needed plugin-side. (More optimizations, yay!)

I'll try and replace the current keybinds: Vec<(String, String)> in ModeInfo with this new struct and implement the missing trait. Thank you for the suggestion! :)

@har7an
Copy link
Contributor Author

har7an commented Apr 14, 2022

Humm, that still doesn't do the trick... The issue is that I would somehow need to get the keybindings into the zellij_utils::input::mode_info() function, but I'm not sure how. This is called twice from the server and once from the zellij client, but at least in the server I don't have access to the global config (which holds the keybindings) either, haven't checked for the clients yet. Or I'm just blind, which is at least as likely.

I generally don't really understand where the config goes, I see it's passed to start_client_impl in src/command.rs and from there onwards it's sealed into some arcane data structures.

In the meantime I've had some other thoughts. What about making a zellij-common subproject that contains only datatypes and definitions used across the various zellij components (and maybe even the built-in plugins)? I haven't investigated the feasibility of that approach but out of the blue, would you say that can be done? I was thinking maybe that could contain the Config, Keybindings, Input Modes, etc. in such a way that hopefully they aren't tightly coupled to other bits of zellij.

Also maybe we could put the currently active configuration into some static global location, wrapped into an Rc or Arc so it can neatly be accessed by anyone needing it? Or would you consider that something like a security risk?

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. ^^

@imsnif
Copy link
Member

imsnif commented Apr 14, 2022

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 get_mode_info. As you saw, Zellij is divided into a server/client architecture (in order to support detaching the session and attaching it again). The configuration is read and parsed on the client side, and we need this info on the server side.

I think the best way to approach this would be to create the Shortcuts struct we talked about from the config on the client side and then add it to the ClientAttributes struct: https://github.com/zellij-org/zellij/blob/main/zellij-utils/src/ipc.rs#L41

The Screen thread gets access to it when it's initialized here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/screen.rs#L740 (shortly before it calls the first get_mode_info - which would be very useful for you!)
And then is saves certain attributes from it here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/screen.rs#L222
You can do something similar with the Shortcuts struct we talked about. Save it as an attribute on Screen and then access it whenever you need (though it might be wise to have it as a HashMap<ClientId, Shortcuts>, since these can potentially be different in a multiple-user session). Makes sense?

@har7an
Copy link
Contributor Author

har7an commented Apr 14, 2022

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)

I can absolutely relate to that, I very much enjoy not hitting runtime panics of course!

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.

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!

Even if it would end up being more time than I'd need to code it myself (though we're not there yet, ofc)

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.

Save it as an attribute on Screen and then access it whenever you need

Since Screen already has a ClientAttributes member, wouldn't it suffice in this case to store the Shortcuts in the ClientAttributes, and then store that in Screen?

(though it might be wise to have it as a HashMap<ClientId, Shortcuts>, since these can potentially be different in a multiple-user session).

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 ClientAttributes relate to the clients that connect to the server? Since Screen holds only a single ClientAttributes I assume that ClientAttributes and Screen for that matter are a ''thing'' that is relevant in my local zellij session, right?

Oh and thanks for taking the time to write such elaborate answers. I really appreciate it. :)

@imsnif
Copy link
Member

imsnif commented Apr 14, 2022

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.

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 Screen doesn't have a ClientAttributes member (maybe it should - but that's not how things are right now). It takes members from ClientAttributes and palces them on itself, eg. here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/screen.rs#L233

Sorry for the extra bit of complexity with the ClientId - it's something you can totally leave for the end if you like. Shortly, the ClientId is an integer that identifies a client (you can think of a client as a user... imagine opening a Zellij session and attaching to it from 2 different terminal windows - you'll have 2 different clients in the same session).

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.

@imsnif
Copy link
Member

imsnif commented Apr 28, 2022

Hey @har7an - do you think you'll be able to finish working on this sometime in the next week or two?
As I said, I'd be super happy for you to see this through and would be willing to devote time to do it. Be it answering questions, pairing or whatever you need. However, this is a very important feature, and if you don't see yourself seeing this through, I'll have to pick it up myself in a week or two. Please let me know!

@har7an
Copy link
Contributor Author

har7an commented Apr 30, 2022

Hi @imsnif!
Sorry, I got a little sidetracked by PC-hardware related things... I'll try to keep working on it, but I cannot guarantee I'll really finish it. If you don't hear back from me by tuesday, it's probably best you take over. Is that okay for you?

@har7an
Copy link
Contributor Author

har7an commented Apr 30, 2022

Hi @imsnif! Sorry, I got a little sidetracked by PC-hardware related things... I'll try to keep working on it, but I cannot guarantee I'll really finish it. If you don't hear back from me by tuesday, it's probably best you take over. Is that okay for you?

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.
Do you still want to keep the two commits I made as sort of a small refactoring?

@imsnif
Copy link
Member

imsnif commented May 2, 2022

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.

@har7an har7an force-pushed the feature/update-keybinding-hints branch from a8a0d35 to 96719f5 Compare June 23, 2022 14:44
@har7an
Copy link
Contributor Author

har7an commented Jun 24, 2022

@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 client and server import things from zellij-tile in various places. My though was that if I managed to pull these dependencies (Which concerns mostly the zellij_tile::data module) out of there and drop it into zellij-utils, I could have tile import utils and then access all the keybindings in their "direct" form, without having to resort to String conversions.

Now I tried to move the data module into zellij-utils and import zellij-utils from inside zellij-tile but that doesn't work: Since cargo seemingly tries to build all of zellij-utils (although I'm only using the data module), it can't be used by the plugins since the socket2 crate (dependency of async-io) doesn't support the wasm targets (See rust-lang/socket2#200).

Another alternative would be to add another crate with shared content that is compatible with wasm, but that seems silly. I don't think this project needs yet another crate. Do you have a suggestion how to go about this? Could this be solved via cargo features maybe? I have never worked with features yet...

har7an added a commit to har7an/zellij that referenced this pull request Jun 24, 2022
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.
@a-kenji
Copy link
Contributor

a-kenji commented Jun 24, 2022

@har7an,
We kept the zellij-tile intentionally small, to not import everything from zellij-utils

I am sure you can find the exact reasoning from @TheLostLambda somewhere in the repo.

Another alternative would be to add another crate with shared content that is compatible with wasm, but that seems silly. I don't think this project needs yet another crate.

I disagree here. I don't think that this is in general silly. I believe we could benefit substantial from exposing more individual crates.

@har7an
Copy link
Contributor Author

har7an commented Jun 24, 2022

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 zellij-utils either. Currently tile only imports (and re-exports) zellij_utils::data.

@a-kenji
Copy link
Contributor

a-kenji commented Jun 24, 2022

I think in this case it might be fine.

@a-kenji
Copy link
Contributor

a-kenji commented Jun 24, 2022

@har7an,
I had a cursory look at the pr and I do think it might be better to create a separate data crate.

@har7an
Copy link
Contributor Author

har7an commented Jun 24, 2022

I had a cursory look at the pr and I do think it might be better to create a separate data crate.

Keeping #1242 in mind, how would the content of this new data crate then access the zellij_utils::input::keybinds::ModeKeybinds or Keybinds?

Edit: I mean: There's no end to how many components you move out of zellij_utils. Are you sure we should do that?

@a-kenji
Copy link
Contributor

a-kenji commented Jun 24, 2022

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?

@har7an
Copy link
Contributor Author

har7an commented Jun 24, 2022

I am not home yet

I'm not in a rush, take your time.

In particular it seems that you have unintentional changes in the PR?

None that I'm aware of. I did a bit of code formatting in some of the larger use blocks, and I also made sure to mode all the new includes to a block use statement when one was already there.

Anyways, that's why I'm already looking forward to your feedback. ^^

@a-kenji
Copy link
Contributor

a-kenji commented Jun 24, 2022

#1541
Removes a previously merged PR and adjusts snapshot tests.

@har7an
Copy link
Contributor Author

har7an commented Jun 24, 2022

Removes a previously merged PR and adjusts snapshot tests.

Oh you mean this PR, yeah that's not finished by a long shot. I meant #1541 for the changes with the refactoring. I added two commits here yesterday before I realized how I did it here wouldn't work. I haven't removed them yet, sorry. I'm currently working on a better version of this PR based on #1541. I'm confused now. #1541 doesn't touch the snapshot tests, that is #1242.

@a-kenji
Copy link
Contributor

a-kenji commented Jun 24, 2022

Then I am confused now too, maybe GH is confused and showing me something wrong.

@imsnif
Copy link
Member

imsnif commented Jun 24, 2022 via email

har7an added 11 commits July 25, 2022 09:44
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.
@har7an har7an force-pushed the feature/update-keybinding-hints branch from b47946d to 80a84a3 Compare July 25, 2022 07:48
@har7an
Copy link
Contributor Author

har7an commented Jul 25, 2022

Rebased onto main.

@imsnif
Copy link
Member

imsnif commented Jul 25, 2022

Great work on the tests!

The thing is I tested them on my PC now with cargo test --target x86_64-unknown-linux-gnu because I cannot execute that wasm executable directly. How do I apply that to cargo make? I can't just assume everyone else uses exactly the same target as me, right?

I played with it a little bit, and it seems that if I comment out the target field in status-bar/.cargo/config.toml I can run cargo test inside the status-bar crate. I think it's a matter of first getting this to work without commenting out (maybe using cargo profiles or some such?) and then getting it to run through cargo make. When doing the latter, please be very very sure that you are only getting it to run the tests through cargo make and not adding it to any other flow - otherwise I will hate you while releasing :) If you have to make it hard-coded and ugly, so be it.

(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).

And: How exactly do I add a new e2e test though? Do you have some instructions for that somewhere?

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
And then making it load a custom config that will change the UI in a very basic way. Just to make sure "everything is connected". To load this config, I'd add it as a fixture in the fixtures folder and then try to checkout the RemoteRunner to add a custom method that would load it on startup.

  1. About bincode:

It is necessary, sorry. When I switch back to "bincode", either I get errors about empty messages, or I get stack traces somewhere during tab creation (which is effectively application startup).

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 :)

@har7an
Copy link
Contributor Author

har7an commented Jul 25, 2022

I played with it a little bit, and it seems that if I comment out the target field in status-bar/.cargo/config.toml I can run cargo test inside the status-bar crate. I think it's a matter of first getting this to work without commenting out (maybe using cargo profiles or some such?) and then getting it to run through cargo make.

That's correct, but in that case it'll also build the plugin for your host target triple and not wasm-wasi. I'd like to avoid that since when developing I frequently call cargo build in the plugin dir and expect that to produce a plugin I can test with a zellij build.

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 .cargo/config.toml) offers only a single option to override the default target, and that's already used here.

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:

  1. Break users expectation and remove the default wasm32-wasi build target. This breaks cargo build since it compiles against the host target triple now. However, we can easily add --target wasm32-wasi to cargo-make, since that target is always the same
    • I tried that by removing the .cargo folders and adapting the build-plugins* tasks in Makefile.toml to use the --target wasm32-wasi argument, but now clippy fails (and rightfully so). I'm afraid that many more things will fall apart if I add another clippy task only for the plugins...
  2. Improve my duckscript-foo and parse the host target triple from the output of rustc -vV to use that in cargo-make. This still leaves the tests unusable (But they are unusable atm, so nothing breaks) but we can call the tests with cargo make test and don't break cargo build in the process
    • I favor this one, but I don't fully understand what happens in this Makefile... Also I see that all wasm32-wasi targets are skipped when testing, so I'd have to make some manual exceptions there. Are you fluent in cargo make and could help me out here?
  3. Make it hard-coded and ugly, this way the tests would probably only run in CI.
    • So I guess that'll be the solution then unless I implement 2 right? How would I best do that?

Thanks for the feedback! I'll work on the e2e test and bincode as time permits, since especially the latter I very much dislike. 😂

@imsnif
Copy link
Member

imsnif commented Jul 25, 2022

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 cargo make or duckscript to help you out here. One of the reasons I'm trying to push them out of the project.

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 cargo test --target $(rustc -vV) or whatever is needed, that would do for now.

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?

@har7an
Copy link
Contributor Author

har7an commented Jul 25, 2022

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. :)

Yeah all right, I can imagine. :P

Do you think you'll manage to get to it sometime this week

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.

@imsnif
Copy link
Member

imsnif commented Jul 25, 2022

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.
@har7an har7an force-pushed the feature/update-keybinding-hints branch from efddae6 to e2544bd Compare July 27, 2022 07:01
@har7an
Copy link
Contributor Author

har7an commented Jul 27, 2022

@imsnif I may have solved the bincode riddle. Allow me to present what I have gathered so far:

bincode is not a self-describing data format. As such, it cannot derive (deserialize) the shape of the data it receives only from the serialized stream it receives.

As per this issue, it is mentioned that serde under some circumstances calls a deserialize_any function on the data it attempts to deserialize. This way, the deserializer tells the underlying code what type the input has, given only the data stream. This has the implication that deserialization of types that need deserialize_any, can only deserialize from self-describing formats (See the docs). This explicitly excludes bincode, which is also stated in the serde docs.

Unfortunately the bincode docs for the latest stable don't mention this, but 2.0.0-rc1 version has a known issues section (Which is also linked to from this issue) which says that some serde attributes aren't supported because bincode isn't a self-describing data format:

    #[serde(skip)]
    #[serde(skip_serializing)]
    #[serde(skip_deserializing)]
    #[serde(skip_serializing_if = "path")]
    #[serde(flatten)]
    #[serde(untagged)]

I grepped the codebase and we have a total of 9 "forbidden" serde attributes in the code:

zellij-utils/src/data.rs
92:#[serde(untagged)]

zellij-utils/src/input/layout.rs
156:    #[serde(flatten)]

zellij-utils/src/input/keybinds.rs
21:    #[serde(flatten)]
29:#[serde(untagged)]
58:#[serde(untagged)]

zellij-utils/src/input/theme.rs
43:    #[serde(flatten)]
66:#[serde(untagged)]

zellij-utils/src/input/config.rs
26:    #[serde(flatten)]
30:    #[serde(flatten)]

All the files under zellij-utils/src/input/* carry the "forbidden" serde attributes in some intermediate structs used to deserialize things from YAML configs and the like. These are never passed around via IPC and thus aren't the cause of the issue.

In zellij-utils/src/data.rs:92 we find the culprit, being:

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)]
#[serde(untagged)] // <-- BAD!
pub enum CharOrArrow {
    Char(char),
    Direction(Direction),
}

And in the serde documentation we find the explanation what this actually does:

There is no explicit tag identifying which variant the data contains. Serde will try to match the data against each variant in order and the first one that deserializes successfully is the one returned.

So do we need this? It very much looks like it. When I undo the offending commit that replaced bincode with rmp and remove the #[serde(untagged)] attribute from the code, some of the early tests start failing right away:

[cargo-make][1] INFO - Execute Command: "cargo" "test" "--target" "x86_64-unknown-linux-gnu" "--"
   Compiling bincode v1.3.3
   Compiling zellij-utils v0.31.0 (/var/home/ahartmann/repos/other/zellij/zellij-utils)
   Compiling zellij-client v0.31.0 (/var/home/ahartmann/repos/other/zellij/zellij-client)
    Finished test [unoptimized + debuginfo] target(s) in 17.05s
     Running unittests (/var/home/ahartmann/repos/other/zellij/target/x86_64-unknown-linux-gnu/debug/deps/zellij_client-db08947d7c834421)

running 4 tests
test stdin_tests::quit_breaks_input_loop ... FAILED
test stdin_tests::pixel_info_sent_to_server ... FAILED
test stdin_tests::move_focus_left_in_normal_mode ... FAILED
test stdin_tests::terminal_info_queried_from_terminal_emulator ... ok

I also tried adding other tags as attributes, but that leads to the same result: The tests panic. Hence, I think that the change is necessary. Sorry...

@imsnif
Copy link
Member

imsnif commented Jul 27, 2022

@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?

@har7an
Copy link
Contributor Author

har7an commented Jul 27, 2022

I really appreciate you diving so deep into this and enjoyed reading your analysis.

Thank you for the kind words!

What else do we have left?

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 zellij-utils. Imo that's something we can look into after this PR, what do you think?

Edit: And looking back through the discussion here we have:

  • The remark about the domain-language: Should I change it, or are you fine with leaving this as is? It's exactly as you said: I tried to save precious space to keep the monster readable...
  • The Default trait derivation for the HashTable HashMap. I don't think that works due to the orphan rule.

@imsnif
Copy link
Member

imsnif commented Jul 27, 2022

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 imsnif merged commit f76b828 into zellij-org:main Jul 27, 2022
@har7an
Copy link
Contributor Author

har7an commented Jul 27, 2022

@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!

@har7an har7an deleted the feature/update-keybinding-hints branch October 23, 2022 15:34
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.

4 participants