Refactor all shared contents from zellij tile to zellij utils #1541
Conversation
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.
Integrates the `data` module that was previously part of `zellij-tile` to allow sharing the contained data structures between all components of zellij. This allows `zellij-tile` to use `utils` as a dependency. However, since `tile` is build against the wasm target, it cannot include all of `zellij-utils`, since a lot of dependencies there cannot compile with `wasm` as target (Examples include: termwiz, log4rs, async-std). Thus we make all the dependencies that cannot compile against `wasm` optional and introduce a new feature `full` that will compile the crate with all dependencies. Along with this, modify `lib.rs` to include most of the data structures only when compiling against the `full` feature. This makes the compiles of `zellij-tile` lighter, as it doesn't include all of `utils`. As a side effect, due to the dependency notation for the optional dependencies (See https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies), we bump the rust toolchain version to 1.60.0.
Add `zellij-utils` as a dependency to `zellij-tile` and allow us access to the `data` module defined there. Update the re-export in the `prelude` such that from all of the plugins points of view *absolutely nothing changes*.
Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`.
Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. Add the "full" feature flag to the `zellij-utils` dependency so it includes all the components we need.
Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. Add the "full" feature flag to the `zellij-utils` dependency so it includes all the components we need.
Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now.
har7an
changed the title
|
I played around a little with basing #1242 on this. While in theory it allows us to use the I'm currently trying to sort out the bits we can use, but it doesn't look very promising so far. I also must confess that the sheer amount of code to go through is a bit over my head... |
There was a problem hiding this comment.
Hey @har7an - great work on this!
This looks good in principle, but I'm wondering about a few things:
-
Why the upgrade to rust 1.60.0? Did you need some specific feature that was missing in 1.59.0? Generally I prefer to only upgrade if we absolutely have to, because it has lots and lots of other consequences (eg. running the dev build on this PR is noticeable slower - not sure why, but before I start debugging I'd like to minimize the amount of changes)
-
Would it be possible to use the
target_family(https://doc.rust-lang.org/reference/conditional-compilation.html#target_family) cargo feature in the zellij-utils crate instead of adding a custom feature? I think that'll make it a little clearer and include less clutter. What do you think?
That's explained in 40a9342: As a side effect, due to the dependency notation for the I'm not a huge fan of it either, but I saw no other way to achieve this truly conditional compilation. :/ I need this to exclude all the non-wasm compatible crates from the compilation. Without it, cargo will compile all crates listed in
I don't know about that, see the explanation for why I had to raise the MSRV. If there is a way to tell in cargo.toml that some dependencies are only for certain compilation targets we can sure do that, but I haven't really found something like that yet. If you know a way to do this I'd be happy to change it! |
|
@har7an, |
|
Quoted from #1242:
If you prefer you can first bump the MSRV in a separate PR (IF we need that, mind), then we merge this, and then I adapt #1242 on top of the changes from here. Would that be alright for you? I don't mind my changes hanging around a little longer as long as I don't end up fixing dozens of merge conflicts each week. :) |
Do you have an example of how to achieve this? I'd be glad to do it, but I don't know how. I did some research before I raised the MSRV but couldn't find anything that works. |
|
Here's another example of how mio does it with dependencies: https://github.com/tokio-rs/mio/blob/master/Cargo.toml#L48 I'll say again - ideally if possible this should be a |
Iirc that propagates the feature flag from the base crate to the child crates. What I need in order for zellij-utils to compile is to disable some dependencies from ever being touched by cargo when targeting I have made all dependencies that don't cooperate with |
See, that's what I meant: I had no idea this existed. This is great, and that should work without bumping the MSRV. Thank you. ^^ |
No, you can specify that in the crate itself. |
|
For the record, I think this could have been done with @a-kenji 's suggestion as well and that is what he meant - but I'm happy we resolved this :) Once again though: let's try to use the |
|
I agree, I think the |
|
It looks muuuuch better now, thank you for the suggestion and examples! Since supporting Windows isn't actively pursued but not strictly out of the question I went with the more verbose Or do you strongly prefer |
|
This looks great! I totally agree it's much better. |
in favor of conditional compilation using `target_family`. Replace the rust 1.60 method of specifying optional dependencies based on features and optionally include the dependencies only when not building for wasm instead. (I.e. `cfg(not(target_family = "wasm"))`)
since `client`, `server` and `tile` now all depend on `utils` only.
har7an
force-pushed
the
refactor/shared-contents-to-zellij-utils
branch
from
b279383
to
9fa650b
Compare
That's just stupidity on my part, it looks like at some point throughout developing this I did a |
…ls (zellij-org#1541) * zellij-tile: Move `data` to zellij-utils 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. * utils: Integrate new `data` module, bump rust ver Integrates the `data` module that was previously part of `zellij-tile` to allow sharing the contained data structures between all components of zellij. This allows `zellij-tile` to use `utils` as a dependency. However, since `tile` is build against the wasm target, it cannot include all of `zellij-utils`, since a lot of dependencies there cannot compile with `wasm` as target (Examples include: termwiz, log4rs, async-std). Thus we make all the dependencies that cannot compile against `wasm` optional and introduce a new feature `full` that will compile the crate with all dependencies. Along with this, modify `lib.rs` to include most of the data structures only when compiling against the `full` feature. This makes the compiles of `zellij-tile` lighter, as it doesn't include all of `utils`. As a side effect, due to the dependency notation for the optional dependencies (See https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies), we bump the rust toolchain version to 1.60.0. * tile: Import `data` from zellij-utils Add `zellij-utils` as a dependency to `zellij-tile` and allow us access to the `data` module defined there. Update the re-export in the `prelude` such that from all of the plugins points of view *absolutely nothing changes*. * utils: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. * client: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. Add the "full" feature flag to the `zellij-utils` dependency so it includes all the components we need. * server: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. Add the "full" feature flag to the `zellij-utils` dependency so it includes all the components we need. * tests: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. * utils: Remove "full" feature in favor of conditional compilation using `target_family`. Replace the rust 1.60 method of specifying optional dependencies based on features and optionally include the dependencies only when not building for wasm instead. (I.e. `cfg(not(target_family = "wasm"))`) * cargo: Update module dependencies since `client`, `server` and `tile` now all depend on `utils` only.
* WIP: First draft of searching in panes. * Add ability to highlight search-results in viewport and move forwards/backwards * Clear search results when leaving search * Search newly scrolled in lines and have live-search when entering search-term * search_forward/backward() now doesn't get the needle again, since we already know it * Use red and yellow from theme. No idea if we should introduce new 'search'-colors * Implement moving the viewport for searches outside the current one. * Implement hacky case-insensitivity (ASCII only at the moment) * Implement wrap-search and prepare infrastructure for whole-word search * Add a bunch of tests and an embarrasing amount of bugfixes * Remember search selection when toggling case-sensitivity (if possible) * New tab integration tests and make search work with floating panes * Make highlights work with resize (not keeping the active-selection in most cases) * Switch the search-algo a bit in order to make multi-line search work * Don't forget active selection when nothing more is found, reflow found selections and scroll correctly * Make all search-related function calls in plugin-pane No-ops * Activate whole word search (ASCII only) * Run cargo fmt * Make clippy happy * Remove unneeded transferred_rows_count * Remove boilerplate and use macro instead * Add explanatory comments * Move search-related functions into SearchResults impl and try to remove duplicate code * Move clearing of search-results upon mode-switch to appropriate place * Jump to the first occurence while typing (EnterSearch), if none is found in the current viewport * Always show needle and also show search modifiers in pane title * Integration tests now use correct InputMode, so we can test the pane title when doing searches * Move no-op implementation of search-functions from plugin-pane to pane-trait * Move SearchResult to its own file * Try to clean up search_row() a bit * Make clippy happy * fix: various typos (#1553) Because they were wrong. * flake.lock: Update (#1554) Flake lock file updates: • Updated input 'crate2nix': 'github:kolloch/crate2nix/805cdaf084c859c2ea0c084b74f4527b0483f6aa' (2022-06-17) → 'github:kolloch/crate2nix/91f333aca414ee346bc5bdea76fe9938f73a15f9' (2022-07-01) • Updated input 'flake-utils': 'github:numtide/flake-utils/1ed9fb1935d260de5fe1c2f7ee0ebaae17ed2fa1' (2022-05-30) → 'github:numtide/flake-utils/bee6a7250dd1b01844a2de7e02e4df7d8a0a206c' (2022-06-24) • Updated input 'nixpkgs': 'github:nixos/nixpkgs/3d7435c638baffaa826b85459df0fff47f12317d' (2022-06-16) → 'github:nixos/nixpkgs/0ea7a8f1b939d74e5df8af9a8f7342097cdf69eb' (2022-07-02) • Updated input 'rust-overlay': 'github:oxalica/rust-overlay/da04f39d50ad2844e97a44015048c2510ca06c2f' (2022-06-18) → 'github:oxalica/rust-overlay/bbba5e73a21c8c67d5fe1d4d8b3fde60ab6946cd' (2022-07-03) * fix: fallback to default values when terminal rows/cols are 0 (#1552) * fix: fallback to default values when terminal rows/cols = 0 * increase retry_pause for failing test * e2e: load fixtures with cat * use variable for fixture path * docs(changelog): fix 0 rows or cols crash * fix(ci): clippy (#1559) Install `cargo-make` explicitly in the workflow, even tough it should be cached from the previous steps. There are some corner cases in which gh messes the caching up and can't access it. * add(nix): add `compact-bar` to the flake outputs (#1560) The compact bar wasn't an output yet. * refactor(crates): move shared contents from zellij tile to zellij utils (#1541) * zellij-tile: Move `data` to zellij-utils 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 #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. * utils: Integrate new `data` module, bump rust ver Integrates the `data` module that was previously part of `zellij-tile` to allow sharing the contained data structures between all components of zellij. This allows `zellij-tile` to use `utils` as a dependency. However, since `tile` is build against the wasm target, it cannot include all of `zellij-utils`, since a lot of dependencies there cannot compile with `wasm` as target (Examples include: termwiz, log4rs, async-std). Thus we make all the dependencies that cannot compile against `wasm` optional and introduce a new feature `full` that will compile the crate with all dependencies. Along with this, modify `lib.rs` to include most of the data structures only when compiling against the `full` feature. This makes the compiles of `zellij-tile` lighter, as it doesn't include all of `utils`. As a side effect, due to the dependency notation for the optional dependencies (See https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies), we bump the rust toolchain version to 1.60.0. * tile: Import `data` from zellij-utils Add `zellij-utils` as a dependency to `zellij-tile` and allow us access to the `data` module defined there. Update the re-export in the `prelude` such that from all of the plugins points of view *absolutely nothing changes*. * utils: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. * client: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. Add the "full" feature flag to the `zellij-utils` dependency so it includes all the components we need. * server: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. Also unify the imports for the `data` module members: We import all of the through `data::` now, not through a mixture of `data::` and `prelude::`. Add the "full" feature flag to the `zellij-utils` dependency so it includes all the components we need. * tests: Fix `data` module dependency Since the `data` module has been migrated from `zellij-tile` to `zellij-utils`, we import it from `zellij-utils` directly now. * utils: Remove "full" feature in favor of conditional compilation using `target_family`. Replace the rust 1.60 method of specifying optional dependencies based on features and optionally include the dependencies only when not building for wasm instead. (I.e. `cfg(not(target_family = "wasm"))`) * cargo: Update module dependencies since `client`, `server` and `tile` now all depend on `utils` only. * docs(changelog): crate refactor * fix: typo (#1567) * feat(terminal): sixel support (#1557) * work * work * work * work * work * more work * work * work * work * hack around stdin repeater * refactor(sixel): rename sixel structs * feat(sixel): render text above images * fix(sixel): reap images once they're past the end of the scrollbuffer * fix(sixel): display images in the middle of the line * fix(sixel): render crash * fix(sixel): react to SIGWINCH * fix(sixel): behave properly in alternate screen mode * fix(sixel): reap images on terminal reset * feat(sixel): handle DECSDM * fix(terminal): properly respond to XTSMGRAPHICS and device attributes with Sixel * Add comment * fix(sixel): hack for unknown event overflow until we fix the api * feat(input): query terminal for all OSC 4 colors and respond to them in a buggy way * fix(sixel): do not render corrupted image * feat(input): improve STDIN queries * fix(client): mistake in clear terminal attributes string * fix(ansi): report correct number of supported color registers * fix(sixel): reap images that are completely covered * style(comment): fix name * test(sixel): infra * test(sixel): cases and fixes * fix(sixel): forward dcs bytes to sixel parser * refactor(client): ansi stdin parser * refactor(output): cleanup * some refactorings * fix test * refactor(grid): sixel-grid / sixel-image-store * refactor(grid): grid debug method * refactor(grid): move various logic to sixel.rs * refactor(grid): remove unused methods * fix(sixel): work with multiple users * refactor(pane): remove unused z_index * style(fmt): prepend unused variable * style(fmt): rustfmt * fix(tests): various apis * chore(dependencies): use published version of sixel crates * style(fmt): rustfmt * style(fmt): rustfmt * style(lint): make clippy happy * style(lint): make clippy happy... again * style(lint): make clippy happy... again (chapter 2) * style(comment): remove unused * fix(colors): export COLORTERM and respond to XTVERSION * fix(test): color register count * fix(stdin): adjust STDIN sleep times * docs(changelog): sixel support * flake.lock: Update (#1575) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(ci): add new rust toolchain location to action (#1576) * rust-toolchain: Update (#1578) Co-authored-by: a-kenji <[email protected]> * chore(nix): hide `nix` directory (#1579) * chore(gblame): add move to git-blame-ignore-revs This is not relevant for `git blame` messages. * chore(docs): add more matrix links (#1581) * fix: add usage comment to fish shell auto-start snippet (#1574) (#1583) * docs(changelog): add usage comment to fish script * Refactor match session name (#1582) * docs(changelog): refactor get session name (#1582) * fix(cli): let the exit message be different when detaching (#1573) * Let the exit message be different when detaching This patch changes the exit message printed to the user, so the user does not get the impression that they fat-fingered an "exit" instead of what was intended (a detach). For this, the InputHandler::exit() function was refactored, to get the reason as a parameter. As this function is not pub, this is considered okay. Signed-off-by: Matthias Beyer <[email protected]> * Change detach message This patch changes the detach message to be more in line with the other messages zellij displays to the user. Signed-off-by: Matthias Beyer <[email protected]> * docs(changelog): detach message * perf(terminal): better responsiveness (#1585) * performance(pty): only buffer terminal bytes when screen thread is backed up * style(fmt): rustfmt * docs(changelog): performance improvement * style(fmt): rustfmt * fix(search): adjust foreground color for better readability * style(fmt): rustfmt * test(e2e): update snapshots from SCROLL to SEARCH * Rename search directions to up/down * Rename search-functions in tests as well * Move all search-related functions out of grid.rs and into search.rs and reuse as much as possible * Fix bug where searches that fall on the line-ending are highlighting the whole line * Silence clippy on what I think is a false-positive * fix(terminal): persist cursor hide/show through alternate screen (#1586) * fix(terminal): persist cursor hide/show through alternate screen * style(fmt): rustfmt * style(clippy): make clippy happy * docs(changelog): cursor show/hide alternate screen fix * fix(editor): handle editor/visual/configured editor with arguments (#1587) * fix(editor): handle editor/visual/configured editor with arguments * style(fmt): rustfmt * docs(changelog): editor with arguments * fix(ci): quoting issues (#1589) * fix(mouse): avoid forwarding click events on pane border (#1584) * if left click is on pane border do not forward to application * properly handle frames * fix comment * fix another comment * add tests, fix edge case * docs(changelog): mouse click on pane frame fix * flake.lock: Update (#1592) Flake lock file updates: • Updated input 'crate2nix': 'github:kolloch/crate2nix/2d20dec4ae330f39b0bebeb8eb4a201b58d2b82c' (2022-07-09) → 'github:kolloch/crate2nix/45d97c7ce62c3d53954743057ceb32e483c31acd' (2022-07-12) • Updated input 'nixpkgs': 'github:nixos/nixpkgs/b39924fc7764c08ae3b51beef9a3518c414cdb7d' (2022-07-08) → 'github:nixos/nixpkgs/4a01ca36d6bfc133bc617e661916a81327c9bbc8' (2022-07-14) • Updated input 'rust-overlay': 'github:oxalica/rust-overlay/3dfc78e42a285caaf83633224a42e7fb7dde191b' (2022-07-10) → 'github:oxalica/rust-overlay/2cd36d4aef875867ee1d7963541ccb3ae50b358c' (2022-07-16) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(search): clear search when entering input in non-search-mode * fix(search): handle searching in updating viewport Co-authored-by: Martin Sirringhaus <> Co-authored-by: a-kenji <[email protected]> Co-authored-by: Thomas Linford <[email protected]> Co-authored-by: Aram Drevekenin <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: a-kenji <[email protected]> Co-authored-by: Tassilo Horn <[email protected]> Co-authored-by: Jae-Heon Ji <[email protected]> Co-authored-by: Matthias Beyer <[email protected]>
This PR incorporates a refactoring that will hopefully enable an easier implementation of #1242. Currently,
zellij-utilsis supposed to contain contents that are shared between theclientandservercrate, respectively. However, it also re-exportszellij-tile, in particular thedatamodule ofzellij-tile, which holds a lot of lower-level data structures used throughout the whole code.This has the implication that
zellij-tileitself cannot importzellij-utilsto access the shared data structures there, since that would cause a cyclic dependency. This is currently worked around by e.g. converting all keys and keybindings to strings before sending them to the plugins (which rely only inzellij-tile) sinceStringas a base type is understood by all plugins. However, handling strings isn't ergonomic and completely foregoes all advantages of Rusts type system.Since the only mode inside
zellij-tilethat is used by all other parts of the code is thedatamodule, this PR moves the data submodule tozellij-utils. This breaksutilsdependency ontileand allows us in turn to importutilsand use it inzellij-tile. In order not to break the plugins, we then re-export thedatamodules contents fromtile.This will significantly ease extensions to the code base (such as in #1242) that relate to the plugins since the plugins can now selectively be granted access to shared data structures from
zellij-utilsby re-exporting them throughzellij-tile.Note that since
zellij-utilsincludes some crates that cannot compile against thewasmtarget, a new feature "full" was added to aleviate this. As suchzellij-tilecurrently has only access tozelij_utils::data, but other modules that do not depend on crates incompatible withwasmcould be added, too.The new feature is currently still undocumented. I wanted to get feedback from you first before polishing it. Also I wasn't sure where/how to document this feature (Because this is the first time I work with features in cargo).
Looking forward to your feedback!