allow dump_screen() to only dump the viewport

[dannasman]

Hello!

First contribution here but to my understanding this small modification seemed to fix the issue #1520 .

[jaeheonji]

LGTM!

[imsnif]

Wait... wouldn't this mean that the scrollback editor now will no longer work with the entire scrollback?

[jaeheonji]

@imsnif

I think this PR is okay based on the intention of #1520. However, personally, it seems better to add options so that users can choose. (Default is current viewport, additionally all scrolls)

In my opinion, it seems good to merge this PR for now, and create a new PR. (Of course I'm going to make PR at the same time as Merge.)

What do you think about this?

[imsnif]

@jaeheonji - I didn't have time to check - but with this PR, when we do ctrl+s + e - do we see the whole scrollback in the editor, not just the viewport? If we do then we can merge this.

I agree that the intention of the original issue is very vague and could be worded better. :)

[jaeheonji]

@imsnif

For this PR, EditScrollbackAction can only edit the current viewport.
That is why additional option implementation will be needed immediately.

pub fn dump_active_terminal_screen(...) {
    ...
    let dump = active_pane.dump_screen(client_id) // must be able to forward additional option.
}

Perhaps, the original issue intention is to change the current viewport to output only, because dumping all scrolls is a lot of content.

Therefore, the default value should be the current viewport only, and the user should be able to select whether to output all additional scrolls.

[imsnif]

@jaeheonji - thanks for checking this!

The dump scroll back feature (ctrl+s + e) should dump the whole scrollback as well - always. It's a little useless otherwise (a lot of users use it to search, for example). I think this is pretty much the only thing this action is used for. It's very important that we keep it this way.

How we do this internally is IMO less important. So if we would like to add options to this method, it's great - but I think we should add user-facing implementations of these options as part of the change, otherwise we won't know what we need exactly (i.e. this is an implementation detail of a feature that does not exist yet). Makes sense?

[jaeheonji]

@imsnif

Yes, I totally agree with that. For example, if an option is added to this case, the user will be able to select as follows.

$ zellij action dump-screen -h
Dumps the pane to a file

USAGE:
    zellij action dump-screen <PATH>

ARGS:
    <PATH>

OPTIONS:
    -h, --help    Print help information
    -f, --full    Dump the pane with scrollback

[imsnif]

Yes, exactly @jaeheonji ! Thanks for understanding. Sorry for coming in like this - I just didn't want to break this feature :)

[imsnif]

So @jaeheonji - I think the best thing here would be to implement this as an option and keep the current default - while adding the CLI flag like you mentioned?

[jaeheonji]

@imsnif

Yes, exactly @jaeheonji ! Thanks for understanding. Sorry for coming in like this - I just didn't want to break this feature :)

You don't have to be sorry. I understand you. :)

I think the best thing here would be to implement this as an option and keep the current default - while adding the CLI flag like you mentioned?

Yes, that’s right.

[jaeheonji]

Hey, @dannasman! We need some additional implementations to go ahead with this PR.

Before I do this, I first ask you a question about whether you want to implement it. If you want to implement it, I will help you. What do you think?

[dannasman]

Hi!

I would be happy to implement it.

[jaeheonji]

Cool! Thank you :)
If you have any questions, please leave a comment!

[dannasman]

Hello!

Here is a proposed solution for the additional improvements.

[jaeheonji]

Thank you! Almost done!

I have one request. If possible, can the default output of the DumpScreen be only the viewport?

Of course, in this case, modifications would be necessary to keep the default value of other Actions referencing DumpScreen (eg EditScrollBack) to full scroll.

[jaeheonji]

Cool! It works well 👍