Add action to clear all buffers for a specific pane #2239

olekspickle · 2023-03-07T21:38:16Z

Fixes #2081 (?)

I want to start a bigger feature contribution but I needed some easy task to get familiar with the codebase. This seemed like a perfect candidate.

It's just a draft anyway if for some reason this naive implementation is somehow not favorable.
Also I guess for the sake of fixing this particular issue, it makes sense for me to clear only line_above.
But this is the more accurate semantic for this action IMO. Let me know what you guys think.

Tested it locallly , worked as expected.

test manually clear action (zellij action clear for a trashed pane)
write a dedicated test for it (not sure I did it clean here)

imsnif

Hey, looks cool - good job on this!

I left some comments in the code. Otherwise let's also add a bindable key for this, you can take a look at similar implementations in our kdl parser: https://github.com/zellij-org/zellij/blob/main/zellij-utils/src/kdl/mod.rs

imsnif · 2023-03-15T18:03:49Z

src/sessions.rs

@@ -207,7 +207,7 @@ pub(crate) fn assert_session_ne(name: &str) {
        process::exit(1);
    }
    if name.contains('/') {
-        eprintln!("Session name cannot contains '/'.");
+        eprintln!("Session name cannot contain '/'.");


imsnif · 2023-03-15T18:05:10Z

zellij-server/src/panes/grid.rs

+    pub fn clear_screen(&mut self) {
+        self.lines_above = Default::default();
+        self.lines_below = Default::default();
+        self.viewport = Default::default();


Let's add a conditional here so that we don't do this if alternate_screen is Some. This happens when eg. programs running in the terminal switch to full screen (eg. htop and friends - also Zellij itself). I think in such cases we really don't want to do this. We can add a log if that happens.

imsnif · 2023-03-15T18:06:31Z

zellij-server/src/tab/mod.rs

@@ -235,6 +235,9 @@ pub trait Pane {
    fn push_right(&mut self, count: usize);
    fn pull_left(&mut self, count: usize);
    fn pull_up(&mut self, count: usize);
+    fn clear_screen(&mut self) {
+        todo!();
+    }


Why not forgo the default implementation here? If you're concerned about plugins (our only other type of Pane) then I think we can leave it and remove the todo!() - I don't think this action is relevant for plugins...

Not really, when I was doing this specifically I was trying to understand the whole architecture of zellij, so, I'll gladly just remove it

imsnif · 2023-03-15T18:07:20Z

zellij-server/src/tab/unit/tab_integration_tests.rs

+        "",
+        "screen was cleared properly"
+    );
+}


olekspickle · 2023-03-15T19:58:40Z

Hmm, so zj action clear works as expected, but the binded action is glitchy, don't know why. Could it be overlapping with something?

I found this comment about escape sequences, but this is not my strongest side, so is there already some of this escape magic somewhere in zellij?
[Unhandled CSI] action='l', args=[2004], intermediates=[63] -> Reset bracketed paste

At a glance looks like a limitation of sorts

jurica · 2023-03-15T20:58:04Z

Hmm, so zj action clear works as expected, but the binded action is glitchy, don't know why. Could it be overlapping with something?

I found this comment about escape sequences, but this is not my strongest side, so is there already some og this escape magic somewhere in zellij? [Unhandled CSI] action='l', args=[2004], intermediates=[63] -> Reset bracketed paste

At a glance looks like a limitation of sorts

What a coincidence, I was testing your branch tonight, just while you did your latest changes.
I also noticed that triggering the action via CLI behaves different than when binding it to a key. But I just realized that in a release build it's less glitchy... I hope that hint helps.

Apart from that glitch on keybinding, this is really awesome! For me this is the feature I missed the most in zellij, thanks for the implementation!

olekspickle · 2023-03-21T10:17:43Z

Ok here are the logs I've localized near the buggy behavior:

logs


WARN   |zellij_server::panes::gri| 2023-03-21 11:14:42.109 [screen    ] [zellij-server/src/panes/grid.rs:2990]: Unhandled esc_dispatch: 62->[]
WARN   |zellij_server::panes::gri| 2023-03-21 11:14:42.110 [screen    ] [zellij-server/src/panes/grid.rs:2382]: Unhandled osc: [[49], [116, 114, 101, 101]]
WARN   |zellij_server::panes::gri| 2023-03-21 11:14:42.398 [screen    ] [zellij-server/src/panes/grid.rs:2382]: Unhandled osc: [[49], [46, 46, 111, 99, 117, 109, 101, 110, 116, 115, 47, 114, 117, 115, 116]]
WARN   |zellij_server::panes::gri| 2023-03-21 11:14:42.398 [screen    ] [zellij-server/src/panes/grid.rs:2990]: Unhandled esc_dispatch: 61->[]
WARN   |zellij_server::panes::gri| 2023-03-21 11:14:50.162 [screen    ] [zellij-server/src/panes/grid.rs:2990]: Unhandled esc_dispatch: 62->[]
WARN   |zellij_server::panes::gri| 2023-03-21 11:14:50.163 [screen    ] [zellij-server/src/panes/grid.rs:2382]: Unhandled osc: [[49], [46, 46, 111, 99, 117, 109, 101, 110, 116, 115, 47, 114, 117, 115, 116]]
WARN   |zellij_server::panes::gri| 2023-03-21 11:14:50.174 [screen    ] [zellij-server/src/panes/grid.rs:2990]: Unhandled esc_dispatch: 61->[]

So I need to handle 49 (set background color) in osc somehow
Tried doing fill_viewport and reset_viewport, it made things worse 😅

imsnif · 2023-03-28T14:15:05Z

Hey @alekspickle - sorry it took me some time to get to this. Thank you for your patience.

I have not managed to reproduce the glitch you mention, but on the other hand this as it is now doesn't really work for me locally with various results. I have had the most success with this change (in grid.rs):

    pub fn clear_screen(&mut self) {
        if self.alternate_screen_state.is_some() {
            log::warn!("Tried to clear pane with alternate_screen_state");
            return;
        }
        self.reset_terminal_state();
        self.mark_for_rerender();
//         self.lines_above = Default::default();
//         self.lines_below = Default::default();
//         self.viewport = Default::default();
    }

What do you think?

olekspickle · 2023-03-28T16:19:34Z

I was able to play with a several existing functions, but the best I did is that the new prompt line was at the bottom and after you hit enter (same behavior you have). This is better, and honestly - good enough. My idea was to clean up for a really trashed panes and I would not really override it by default.
I still don't know how 49 clear background byte messes up the binding option but action works as expected.

imsnif · 2023-03-29T08:43:32Z

Cool, glad you like this variant. Would you like to adjust the PR?

Otherwise, for the binding being messed up - I do not see this behaviour (neither with the change nor with the original PR). Do you still see it with the change?

olekspickle · 2023-03-29T09:45:28Z

I already did!
Yeah, it's kinda weird, but reasonable enough:

olekspickle · 2023-03-29T09:47:23Z

also I have new assets to commit after building release version for some reason. Is this expected? I guess it's because wasm binaries changed and only debug ones committed. Feels like something that should be ignored. Or does it gets used in packaging somehow?


Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   zellij-utils/assets/plugins/compact-bar.wasm
        modified:   zellij-utils/assets/plugins/status-bar.wasm
        modified:   zellij-utils/assets/plugins/strider.wasm
        modified:   zellij-utils/assets/plugins/tab-bar.wasm

imsnif · 2023-03-29T17:51:58Z

Hey @alekspickle - sorry, I hadn't noticed you already made the change!

This looks great. I did some minor cleanup: removing the commented out config (we'll update this on the doc website before the release) and removing the info log. I think the log is a bit noisy as is. Once the CI passes I'll merge.

The built assets are a bit of a release hack we have for... reasons. :) We only commit them through the CI when releasing a version.

Great work on this, hope to see more stuff from you!

olekspickle added 3 commits March 7, 2023 21:55

fix typo

ebb9308

Add clear screen action

06e3cc2

add proper test

6871ebe

olekspickle marked this pull request as ready for review March 8, 2023 14:37

olekspickle temporarily deployed to cachix March 15, 2023 17:48 — with GitHub Actions Inactive

imsnif requested changes Mar 15, 2023

View reviewed changes

olekspickle added 2 commits March 15, 2023 19:37

Merge branch 'main' into feat/2081-screen-clear

c8c0dd7

added bindable action; remove pointless default impl

1ecaf48

olekspickle temporarily deployed to cachix March 15, 2023 19:05 — with GitHub Actions Inactive

olekspickle requested a review from imsnif March 15, 2023 19:05

add default binding

fdb0644

olekspickle temporarily deployed to cachix March 15, 2023 20:05 — with GitHub Actions Inactive

use imsnif's variant

0bef6ef

olekspickle temporarily deployed to cachix March 28, 2023 20:10 — with GitHub Actions Inactive

imsnif added 2 commits March 29, 2023 19:47

remove commented example config

4cadfe8

remove log line

41704c1

imsnif temporarily deployed to cachix March 29, 2023 17:57 — with GitHub Actions Inactive

imsnif merged commit bdb39b1 into zellij-org:main Mar 29, 2023

imsnif mentioned this pull request Oct 9, 2023

Add ClearScrollback to replicate macOS Cmd+K behavior #1768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add action to clear all buffers for a specific pane #2239

Add action to clear all buffers for a specific pane #2239

olekspickle commented Mar 7, 2023 •

edited

Loading

imsnif left a comment

imsnif Mar 15, 2023

imsnif Mar 15, 2023

imsnif Mar 15, 2023

olekspickle Mar 15, 2023 •

edited

Loading

imsnif Mar 15, 2023

olekspickle commented Mar 15, 2023 •

edited

Loading

jurica commented Mar 15, 2023 •

edited

Loading

olekspickle commented Mar 21, 2023 •

edited

Loading

imsnif commented Mar 28, 2023

olekspickle commented Mar 28, 2023 •

edited

Loading

imsnif commented Mar 29, 2023

olekspickle commented Mar 29, 2023

olekspickle commented Mar 29, 2023 •

edited

Loading

imsnif commented Mar 29, 2023

Add action to clear all buffers for a specific pane #2239

Add action to clear all buffers for a specific pane #2239

Conversation

olekspickle commented Mar 7, 2023 • edited Loading

imsnif left a comment

Choose a reason for hiding this comment

imsnif Mar 15, 2023

Choose a reason for hiding this comment

imsnif Mar 15, 2023

Choose a reason for hiding this comment

imsnif Mar 15, 2023

Choose a reason for hiding this comment

olekspickle Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

imsnif Mar 15, 2023

Choose a reason for hiding this comment

olekspickle commented Mar 15, 2023 • edited Loading

jurica commented Mar 15, 2023 • edited Loading

olekspickle commented Mar 21, 2023 • edited Loading

imsnif commented Mar 28, 2023

olekspickle commented Mar 28, 2023 • edited Loading

imsnif commented Mar 29, 2023

olekspickle commented Mar 29, 2023

olekspickle commented Mar 29, 2023 • edited Loading

imsnif commented Mar 29, 2023

olekspickle commented Mar 7, 2023 •

edited

Loading

olekspickle Mar 15, 2023 •

edited

Loading

olekspickle commented Mar 15, 2023 •

edited

Loading

jurica commented Mar 15, 2023 •

edited

Loading

olekspickle commented Mar 21, 2023 •

edited

Loading

olekspickle commented Mar 28, 2023 •

edited

Loading

olekspickle commented Mar 29, 2023 •

edited

Loading