Feature request: How to dump the terminal output #1353

cosminadrianpopescu · 2022-04-27T15:37:04Z

Hello,

I'm a long time tmux user, I'm in the process of switching to zellij (due to the floating panels) and I'm missing the search in buffer.

I've seen in a post on reddit that you intend to implement a command to dump the terminal output (with all the scroll) into a file. I think this is an amazing idea which would leave tmux so far behind.

I would be interested in this.

Can you please explain what it involves, from a development point of view? I'm a developer, I've developed in my life in many languages, but not in Rust. However, I would be willing to see what I can come up with.

imsnif · 2022-04-28T14:39:00Z

Hey @cosminadrianpopescu - I'm happy to read you're excited about Zellij!

I'd be happy to guide you through implementing this feature, but maybe as a first step - since you mention you're not very proficient with Rust - let's see if this is something that would make sense for you to implement? It could be a pretty big feature (though not super complex if you're willing to devote some time to it).

We keep the viewport of each TerminalPane in its Grid, specifically here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/panes/grid.rs#L277

Each Grid is made up of Rows, which can be canonical (meaning we got an "official" line break) or non-canonical, meaning we wrapped that row because there was no room to display it fully on screen. Each Row consists of zero or more TerminalCharacters, which hold the char in that cell of the terminal.

Would you like to take a look through that area of the code and let me know if it makes sense to you? If so I'd be happy to discuss steps on how to implement this.

cosminadrianpopescu · 2022-04-29T08:40:48Z

I would imagine is something on these lines?

        if let Ok(mut f) = File::create("/tmp/dump.screen") {                                                                                                                            
            for line in &self.lines_above {                                                                                                                                      
                for char in &line.columns { // Here I'm sure there is a smarter way to get the columns as a string.                                                              
                    write!(f, "{}", char.character);                                                                                                                             
                }                                                                                                                                                                
                writeln!(f);                                                                                                                                                     
            }                                                                                                                                                                    
        }

What about the widht from TerminalCharacter. Do I need to keep count of it? Or the char type of line::columns::character will include also all the characters that form a multi-byte char? Also, regarding the canonical and not canonical form. Do we care when dumping to file? I don't think that we should try to wrap the line when dumping to a file. Will let the editor opening the dumped file to do the wrapping, right?

cosminadrianpopescu · 2022-04-29T09:18:07Z

Actually, I think I see the point of canonical. I think I was looking at it the other way around. But probably I need to wrap the writeline in an if depending on canonical, something like this:

if line.is_canonical {
    writeln!(f);
}

imsnif · 2022-04-29T09:37:04Z

I would imagine is something on these lines?

Yes, that should do the trick - except we would of course need to adjust it a little to the codebase (eg. not use write! directly but rather use our os_input_output interface) - we can totally worry about those things later once we have a basic implementation. I'll guide you through it.

What about the widht from TerminalCharacter. Do I need to keep count of it?

No need, you can safely ignore it and write the characters to disk. Handling the width will be the responsibility of whoever opens the file.

Actually, I think I see the point of canonical. I think I was looking at it the other way around. But probably I need to wrap the writeline in an if depending on canonical, something like this:
if line.is_canonical {
    writeln!(f);
}

Yes! This is a good and simple solution.

cosminadrianpopescu · 2022-04-29T09:52:20Z

Here is a more robust way of the code above:

        if let Ok(mut f) = File::create("/tmp/log") {                                                                                                                            
            for line in &self.lines_above {                                                                                                                                      
                let s: String = (&line.columns).into_iter().map(|x| x.character).collect();                                                                                      
                write!(f, "{}", s);                                                                                                                                              
                if line.is_canonical {                                                                                                                                           
                    writeln!(f);                                                                                                                                                 
                }                                                                                                                                                                
            }                                                                                                                                                                    
        }

Let me know what you think. I would really be interested in developping this feature. With this feature added, I can completely forget about tmux. The search is the last piece missing and this would also allow me to stay inside of the tty, since after dumping the buffer I can easily also copy / paste from vim to the xclip without the help of the mouse (which is missing in the tty).

cosminadrianpopescu · 2022-04-29T09:54:49Z

I really never thought of it (searching and copy pasting from the buffer) in this way.

I think your idea (of dumping the buffer in a file rather than trying to implement a selection and a search command and so on) is brilliant.

imsnif · 2022-04-29T09:57:47Z

I'm not 100% sure, but I think your first solution is a tiny bit more performant (less heap allocations), but it doesn't really matter in this case :)

And thanks! My ideal solution would be to temporarily replace the current pane with your default editor to make it even more seamless, but that'll for sure come later in addition to this feature. Also, classic search is being worked on as well. So a solution for every taste!

Anyway - this all sounds great, I'd be very happy if you implement this. How can I help? Shall we break it down to steps, do you have a different idea?

cosminadrianpopescu · 2022-04-29T10:50:14Z

My ideal solution would be to temporarily replace the current pane with your default editor to make it even more seamless, but that'll for sure come later in addition to this feature.

That indeed would be a perfect solution.

Also, classic search is being worked on as well. So a solution for every taste!

Yes, also classic search is not bad (I've used it in tmux for years). But your solution (with dumping the screen in a file) is perfect.

Anyways, my idea is that then I will set up a shortcut which will dump the screen and open the file automatically in a popup pane. So it would be as close as possible of your first idea, with 0 effort.

What I'm thinking is that this code (let me know if it needs some more polishing, as I told you I'm not expert in rust) can be called by a new Action. And the next step is to properly implement a new Action (following as a model one of the existing actions).

Does this sounds ok? Do you have any advice for me? And this code should reside in the grid.rs file.

a-kenji · 2022-04-29T15:00:46Z

Anyways, my idea is that then I will set up a shortcut which will dump the screen and open the file automatically in a popup pane. So it would be as close as possible of your first idea, with 0 effort.

I would encourage you to make this action as program agnostic as possible.

cosminadrianpopescu · 2022-04-29T18:10:13Z

I would encourage you to make this action as program agnostic as possible.

I think you misunderstood. What this feature would implement would be only an Action like DumpScree.

Then, all the rest is done via config, like for example:

- action: [DumpScreen, Run: {cmd: nvim, args: [/tmp/log.sh, +normal G]}, SwitchToMode: Locked, ]                                                       
  key: [Char: '/']

The opening of the editor and so on would not be part of the code.

imsnif · 2022-04-29T19:36:38Z

Hey @cosminadrianpopescu ,

I think your solution is pretty cool. Let's get it implemented!

Does this sounds ok? Do you have any advice for me? And this code should reside in the grid.rs file.

It sounds good. Either one of the functions is fine (at first glance, I'll give it another go over once you make a PR ofc). A few notes on structure:
The part that writes to the filesystem needs to be here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/os_input_output.rs#L236
It would probably best to create a function there called something like write_to_file* that gets a String or &str, whatever's more convenient for you, and does the writing itself. This is so that we can more easily mock it in our tests.
To access this os_api you have two options:

The closest place we have it right now is in Tab: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/tab/mod.rs#L76 (the hierarchy is: Screen => Tab => TerminalPane => Grid). So the Tab can get a String buffer from TerminalPane when it receives the appropriate action and then write it to file.
We've been wanting to give TerminalPane a copy of the os_api as well as mentioned in this comment: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/panes/terminal_pane.rs#L37 - if you feel brave, you can do that and then have the file writing done one layer below... as you wish

Then there's a bit of threading you need to do to create the action and pass it from zellij-client (it's essentially a thin client that is mostly in charge of STDIN/STDOUT, and forwarding actions to the server) to zellij-server where it'll get to the Tab eventually.

Makes sense? I can expand on whichever part you want or explain things differently.

*If it makes more sense to you, you can also call it write_to_temp_file and have it pick and return the temporary file name, that might save you some effort

a-kenji · 2022-04-29T20:33:13Z

I would encourage you to make this action as program agnostic as possible.

I think you misunderstood. What this feature would implement would be only an Action like DumpScree.

Then, all the rest is done via config, like for example:
- action: [DumpScreen, Run: {cmd: nvim, args: [/tmp/log.sh, +normal G]}, SwitchToMode: Locked, ]                                                       
  key: [Char: '/'] 
The opening of the editor and so on would not be part of the code.

Yes, I think I did misunderstand you. It sounded like there was much more hard coding going on in the beginning. What you propose is exactly what I had in mind.

cosminadrianpopescu · 2022-04-30T07:17:45Z

*If it makes more sense to you, you can also call it write_to_temp_file and have it pick and return the temporary file name, that might save you some effort

I was thinking that the file name can be set via config or hard coded. The folder it will be the system temp folder and the name something deterministic. The idea is that after the screen is dumped I want to have an action to be able to automatically open that file, and since the commands don't return an output to the next command in chain, I would have to manually open the file, if the name is not deterministic. What do you think?

- action: [DumpScreen, Run: {cmd: nvim, args: [/tmp/log.sh, +normal G]}, SwitchToMode: Locked, ]                                                       
  key: [Char: '/']

I will start develop and let you know when I have a PR. Also, I will add here any questions I might have.

cosminadrianpopescu · 2022-04-30T07:19:19Z

I was thinking that the file name can be set via config or hard coded. The folder it will be the system temp folder and the name something deterministic. The idea is that after the screen is dumped I want to have an action to be able to automatically open that file, and since the commands don't return an output to the next command in chain, I would have to manually open the file, if the name is not deterministic. What do you think?

Or maybe the DumpScreen command can take one parameter, which is the file? I think this would also be good, and then I would do something like this:

- action: [DumpScreen: /tmp/log.sh, Run: {cmd: nvim, args: [/tmp/log.sh, +normal G]}, SwitchToMode: Locked, ]                                                       
  key: [Char: '/']

imsnif · 2022-04-30T08:09:30Z

Or maybe the DumpScreen command can take one parameter, which is the file? I think this would also be good, and then I would do something like this:
- action: [DumpScreen: /tmp/log.sh, Run: {cmd: nvim, args: [/tmp/log.sh, +normal G]}, SwitchToMode: Locked, ]                                                       
  key: [Char: '/']

Hum... what happens though if you trigger the action on another pane before you closed the first one? I guess since this happens "in user space" this is up to you, but it would be nice to also have an option to dump it to a random temporary file. I'm not sure what the story right now is to have optional parameters to ~~options~~ actions... @a-kenji - what do you think about all of this?

(don't let this stop you though @cosminadrianpopescu, you can start developing for your use case and we'll sort it out as we go)

cosminadrianpopescu · 2022-05-01T06:51:08Z

I began devleoping the feature. I have one question. I see that in order to define a new Action, I need to include in the Pane trait (https://github.com/zellij-org/zellij/blob/main/zellij-server/src/tab/mod.rs#L108) a new method to process the action. Then, I see that I need to define the implementation in the terminal_pane.rs file, which is clear (this is where the magic will happen). But what do I need to define in the plugin_pane.rs implementation? If I don't define the method also there, I see that I get a compilation error.

62  | impl Pane for PluginPane {
    | ^^^^^^^^^^^^^^^^^^^^^^^^ missing `dump_screen` in implementation

cosminadrianpopescu · 2022-05-01T07:26:03Z

Hum... what happens though if you trigger the action on another pane before you closed the first one? I guess since this happens "in user space" this is up to you, but it would be nice to also have an option to dump it to a random temporary file. I'm not sure what the story right now is to have optional parameters to options actions

I'm not sure I follow. I would say that the parameter to the DumpScreen action should be optional. If it's present, then we dump to the indicated file, if not we dump to a random temp file. Regarding "what happens if you trigger the action on another pane before you closed the first one?", I don't understand. I was thinking of using the Sync file writter, so, there is no way to change the pane before the dump is finished. Anyway, even with async file writer, once I trigger the dumping of the file, assuming that it's not finished until the panel is changed, I have a reference to the pane that is being dumped, right? So, even though the pane is changed, this should not impact the dumping (Am I missing something)? And even I'm missing and there could be an issue there, I still am missing the connection with the parameters.

cosminadrianpopescu · 2022-05-01T07:44:15Z

Another question is how do I run the test cases?

imsnif · 2022-05-01T08:11:56Z

Some quick answers because I'm on the road: For running the tests you can take a look at contributing.md... The method can be a noop for plugin pane for now, we'll clean it up later

…

On Sun, May 1, 2022, 9:44 AM Cosmin Popescu ***@***.***> wrote: Another question is how do I run the test cases? — Reply to this email directly, view it on GitHub <#1353 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCHTSC2OJRP5R523X6OJTVHYY5VANCNFSM5UPRLKAQ> . You are receiving this because you commented.Message ID: ***@***.***>

cosminadrianpopescu · 2022-05-01T09:59:03Z

The method can be a noop for plugin pane for now, we'll clean it up later.

That's fine, because that's what I implemented so far. Just a return "":

    fn dump_screen(&mut self, _client_id: ClientId) -> String {
        return "".to_owned();
    }

cosminadrianpopescu · 2022-05-02T17:56:09Z

Do you have any idea what this message means:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Message("data did not match any variant of untagged enum KeyActionUnbind", Some(Pos { marker: Marker { index: 231, line: 9, col: 10 }, path: "keybinds" }))', zellij-utils/src/input/plugins.rs:21:67

Basically I did the work with a hard coded name and it works fine. Now I'm trying to add the String parameter to the DumpScreen command, everything compiles fine, but I get this run time error when zellij starts.

Thank you.

imsnif · 2022-05-02T18:00:57Z

Do you have any idea what this message means:
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Message("data did not match any variant of untagged enum KeyActionUnbind", Some(Pos { marker: Marker { index: 231, line: 9, col: 10 }, path: "keybinds" }))', zellij-utils/src/input/plugins.rs:21:67
Basically I did the work with a hard coded name and it works fine. Now I'm trying to add the String parameter to the DumpScreen command, everything compiles fine, but I get this run time error when zellij starts.

Thank you.

It's this line: https://github.com/zellij-org/zellij/blob/main/zellij-utils/src/input/plugins.rs#L21
My guess is that Zellij failed to parse the config file. I guess it's a matter of redefining your new DumpScreen action to accept an optional parameter. @a-kenji - any input on how we should go about this?

imsnif · 2022-05-02T18:22:29Z

Btw @cosminadrianpopescu - the more I think about it the more I feel it would be better to at least start with the file path parameter being mandatory. We don't have a good and easy way to display messages to the user yet, so if we create a random temp file name it'll be hard to let the user know about it. We might be escaping this problem for now by making it mandatory. wdyt?

a-kenji · 2022-05-02T18:33:45Z

Do you have any idea what this message means:
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Message("data did not match any variant of untagged enum KeyActionUnbind", Some(Pos { marker: Marker { index: 231, line: 9, col: 10 }, path: "keybinds" }))', zellij-utils/src/input/plugins.rs:21:67
Basically I did the work with a hard coded name and it works fine. Now I'm trying to add the String parameter to the DumpScreen command, everything compiles fine, but I get this run time error when zellij starts.

Thank you.

Does it work when starting zellij with: zellij setup --clean?

a-kenji · 2022-05-02T18:37:18Z

Or maybe the DumpScreen command can take one parameter, which is the file? I think this would also be good, and then I would do something like this:
- action: [DumpScreen: /tmp/log.sh, Run: {cmd: nvim, args: [/tmp/log.sh, +normal G]}, SwitchToMode: Locked, ]                                                       
  key: [Char: '/']
Hum... what happens though if you trigger the action on another pane before you closed the first one? I guess since this happens "in user space" this is up to you, but it would be nice to also have an option to dump it to a random temporary file. I'm not sure what the story right now is to have optional parameters to ~~options~~ actions... @a-kenji - what do you think about all of this?

Optional parameters to actions work quite well, the NewPane, or NewTab action's are good examples for that.

I'm fine with going ahead an getting some sort of workable solution here.

The initial reason I did not open a pr for this: I imagined this working together with a formatting system with some sort of interpolation. I really need to write the formatting issue up soon.

cosminadrianpopescu · 2022-05-02T19:59:16Z

Does it work when starting zellij with: zellij setup --clean?

No.

cosminadrianpopescu · 2022-05-02T20:00:30Z

Optional parameters to actions work quite well, the NewPane, or NewTab action's are good examples for that.

This is what I tried to emulate. I wonder if being an Option<String> and not of some enum has something to do with it?

cosminadrianpopescu · 2022-05-02T20:06:38Z

We might be escaping this problem for now by making it mandatory. wdyt?

Without Option it works fine. With mandatory param is fine.

cosminadrianpopescu · 2022-05-02T20:08:01Z

I will have an initial PR as soon as I clear out some warnings (it should be preety soon).

a-kenji · 2022-05-02T20:15:01Z

Does it work when starting zellij with: zellij setup --clean?

No.

That is quite weird. And it also doesn't work when starting with just the default configuration?

cosminadrianpopescu · 2022-05-03T13:47:35Z

I've opened an initial PR. For the test cases, I'm trying to implement some, but I see that the function write_to_tty_stdin in the test cases is not implemented. So, how can I generate some tty output for the test cases? Is there something you recommend me? Maybe another test case doing this?

    fn write_to_tty_stdin(&self, _fd: RawFd, _buf: &[u8]) -> Result<usize, nix::Error> {
        unimplemented!()
    }

imsnif · 2022-05-03T18:16:37Z

Hey @cosminadrianpopescu - very cool to see you open an initial PR so quickly!

About testing: while Zellij has e2e tests, we only use them to kind of make sure "things are connected" and try not to add many tests there. The reason is that (like most e2e tests) they are quite fragile and tend towards flakiness. So we use them only when absolutely necessary.

For this use case, I'd check out the tab integration tests here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/tab/unit/tab_integration_tests.rs

In these tests we call the methods on tab directly and then snapshot the output. In this case snapshotting the output won't make a whole lot of sense, so instead what I'd go for is to create a mock method on FakeInputOutput (I guess you already had to do that for the tests to compile) and have it write to something on FakeInputOutput that we can assert in the test later.

I plan on taking a look at the draft PR tomorrow and maybe then I'll have more specific ideas once I see how you implemented this.

imsnif · 2022-05-04T17:35:58Z

A thousand pardons for not getting to this yet @cosminadrianpopescu ! 🙏
Had more of a busy day than I thought and I want to give this a good look. Will do my best to get to it tomorrow.

cosminadrianpopescu · 2022-05-04T18:11:02Z

Don't worry. There is no rush. I am using the feature already. I will open another ticket for another idea, to discuss. I've seen the discussion page, so I'll put it there.

* Initial commit for fixing #1353 * adding a new line between the lines_above and the viewport * changes following code review * implementing a test case for the dump screen * implemented test case for dump_screen * better regexp replace * fixes following code review * style(api): remove extraneous method in plugin pane * style(fmt): rustfmt * style(tests): fix method name Co-authored-by: Aram Drevekenin <aram@poor.dev>

a-kenji · 2022-06-15T10:49:50Z

@cosminadrianpopescu,
The cli actions are now implemented in main.
It would be great if you could try them out and give us feedback.
There is still much to be polished there!

But you can already dump the pane there with:

zellij action "DumpScreen: ./dump"

a-kenji added the action Issues related to actions label Apr 27, 2022

cosminadrianpopescu added a commit to cosminadrianpopescu/zellij that referenced this issue May 3, 2022

Initial commit for fixing zellij-org#1353

9e32246

raphCode mentioned this issue Jun 1, 2022

Feature: Make current zellij scrollback accessible #1448

Open

a-kenji closed this as completed Jun 15, 2022

Feature request: How to dump the terminal output #1353

Feature request: How to dump the terminal output #1353

Comments

cosminadrianpopescu commented Apr 27, 2022 • edited Loading

imsnif commented Apr 28, 2022

cosminadrianpopescu commented Apr 29, 2022 • edited Loading

cosminadrianpopescu commented Apr 29, 2022

imsnif commented Apr 29, 2022

cosminadrianpopescu commented Apr 29, 2022

cosminadrianpopescu commented Apr 29, 2022

imsnif commented Apr 29, 2022

cosminadrianpopescu commented Apr 29, 2022

a-kenji commented Apr 29, 2022

cosminadrianpopescu commented Apr 29, 2022

imsnif commented Apr 29, 2022 • edited Loading

a-kenji commented Apr 29, 2022

cosminadrianpopescu commented Apr 30, 2022

cosminadrianpopescu commented Apr 30, 2022

imsnif commented Apr 30, 2022 • edited Loading

cosminadrianpopescu commented May 1, 2022

cosminadrianpopescu commented May 1, 2022 • edited Loading

cosminadrianpopescu commented May 1, 2022

imsnif commented May 1, 2022 via email

cosminadrianpopescu commented May 1, 2022

cosminadrianpopescu commented May 2, 2022

imsnif commented May 2, 2022

imsnif commented May 2, 2022

a-kenji commented May 2, 2022

a-kenji commented May 2, 2022

cosminadrianpopescu commented May 2, 2022

cosminadrianpopescu commented May 2, 2022

cosminadrianpopescu commented May 2, 2022

cosminadrianpopescu commented May 2, 2022

a-kenji commented May 2, 2022

cosminadrianpopescu commented May 3, 2022

imsnif commented May 3, 2022

imsnif commented May 4, 2022

cosminadrianpopescu commented May 4, 2022

a-kenji commented Jun 15, 2022

cosminadrianpopescu commented Apr 27, 2022 •

edited

Loading

cosminadrianpopescu commented Apr 29, 2022 •

edited

Loading

imsnif commented Apr 29, 2022 •

edited

Loading

imsnif commented Apr 30, 2022 •

edited

Loading

cosminadrianpopescu commented May 1, 2022 •

edited

Loading