Half-page scroll actions #813
Conversation
* fix(performance): do not hang when resizing large line wraps * style(fmt): make rustfmt happy * style(clippy): make clippy happy
* fix(compatibility): handle home/end keys properly from terminfo * style(fmt): make rustfmt happy * style(fmt): remove unused import
zellij-server/src/tab.rs
Outdated
| .get_mut(&PaneId::Terminal(active_terminal_id)) | ||
| .unwrap(); | ||
| // prevent overflow when row == 0 | ||
| let scroll_columns = (active_terminal.rows().max(1) - 1)/2; |
There was a problem hiding this comment.
Yes! But to be fair this is also a mistake in the other methods this was copied from. If you'd like to fix this @oromate that would be cool.
There was a problem hiding this comment.
I can fix this. Do you think it makes sense for me to also correct the variable name in the other method?
There was a problem hiding this comment.
That would be great, thanks! There are a few of those.
* specifies the minimum version the package can be compiled with, may be ignored with `--ignore-rust-version` option ref: https://doc.rust-lang.org/nightly/cargo/reference/manifest.html#the-rust-version-field
* fix(unix): forkpty => openpty * style(fmt): make rustfmt happy
* `colors_transform` is deprecated and superceded by `colorsys` ref: https://crates.io/crates/colors-transform
…into oromate-half-page-scroll
There was a problem hiding this comment.
Hey @oromate - thank you for your patience and apologies it took me a few days to get to this.
All in all this looks great. There's the issue with the variable name mistake in the code comments if you feel like fixing it there and in the other methods, but no biggie all in all.
Aside from that, the CI tells us this needs to be formatted. You can do that with rustfmt (I run cargo fmt you can see more details in our CONTRIBUTING doc), and the e2e tests.
The e2e tests are failing because the controls line changed for Scroll mode and they're comparing it against the previous snapshot and finding a difference. We use cargo insta to compare snapshots (https://insta.rs/docs/cli/). You can update the snapshots with their CLI tool once you've run the tests and then commit them to this branch.
Once that is done, I'd be happy to merge this. Please let me know if anything is unclear! I'll try to be quicker on the uptake with the changes.
zellij-server/src/tab.rs
Outdated
| .get_mut(&PaneId::Terminal(active_terminal_id)) | ||
| .unwrap(); | ||
| // prevent overflow when row == 0 | ||
| let scroll_columns = (active_terminal.rows().max(1) - 1)/2; |
There was a problem hiding this comment.
Yes! But to be fair this is also a mistake in the other methods this was copied from. If you'd like to fix this @oromate that would be cool.
|
I believe I managed to resolve the reviews. I hope everything works out. Taking the opportunity to share a little of the journey:
|
…into oromate-half-page-scroll
|
Hey @oromate - thanks for the fixes! I went ahead and ran Thanks for your observations with docker-compose! Would you like to maybe format them a little and add them to CONTRIBUTING.md? They have the best chance of being seen there. Thanks for your contribution! |
I managed to make it this far. I think there is still a lack of documentation and tests, what do you think? I basically replicated the page scroll code and did the division by two, so maybe it would be nice to avoid code duplication. #794