Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add args to new-tab action #2072

Merged
merged 21 commits into from
Feb 16, 2023
Merged

feat: add args to new-tab action #2072

merged 21 commits into from
Feb 16, 2023

Conversation

jaeheonji
Copy link
Member

fix #1967

I added args to NewTab action in keybind.
This can be used like this:

tab {
        ...
        bind "n" {
            NewTab {
                cwd "/tmp"
                layout "example2.kdl"
                name "Hello"
            };
            SwitchToMode "Normal"; 
        }
        ...
}

It can assign various layouts to key combinations.

@jaeheonji jaeheonji requested a review from imsnif January 8, 2023 16:46
@jaeheonji jaeheonji temporarily deployed to cachix January 8, 2023 16:46 — with GitHub Actions Inactive
@jaeheonji jaeheonji marked this pull request as ready for review January 10, 2023 15:51
@imsnif
Copy link
Member

imsnif commented Jan 13, 2023

@jaeheonji - this looks great!

Only issue I found is that cwd and name don't work unless a layout is also provided. That's okay - but if that's the case, maybe we should give an error in this case so the user knows about this?

@jaeheonji
Copy link
Member Author

@imsnif Thank you for reviewing!

I missed that point, I think it would be good to add it. For example, if only name is set, it will only change the tab name of the default layout.

I'll update that point and add test cases.

@jaeheonji jaeheonji temporarily deployed to cachix January 16, 2023 11:56 — with GitHub Actions Inactive
@jaeheonji
Copy link
Member Author

jaeheonji commented Jan 16, 2023

@imsnif

I updated that issue. but, it still have a issue about cwd.
The simplest way to apply cwd is to load the basic layout default.kdl file and apply cwd when there is no layout.

But, In this process, the default_layout set in config is not applied.
For example, If the user specified default_layout as compact in the config file, when NewTab is executed, compact should be imported by default. But, to apply cwd this doesn't work.

Becuase, Layout::stringified_from_path_or_default function basically returns the default layout.

So, cwd is applied only when layout is specified.

but if that's the case, maybe we should give an error in this case so the user knows about this?

Then, coming back to this issue, It looks good that it doesn't show any errors. at least for now.
If it prints an error, I don't know yet how to print it. If an error is displayed like CLIAction, zellij is terminated unintentionally.

And while writing e2e tests, I found one issue.
Currently, I have confirmed several times that this feature is working well. But, it does not work in RemoteRunner of e2e test.
I don't know the exact reason, so it's difficult to write a test. Do you have any ideas about this?

@zolrath
Copy link

zolrath commented Jan 16, 2023

As a user, I would expect to be able to provide any of these options independently and have them default to whatever zellij action new-tab with no options would produce when not supplied.

No Layout

No layout? Current behavior is the layout this instance of zellij was initially run with.
Also, as an aside the new-tab --layout command doesn't know the default layout folder and the user has to supply the full path.

No cwd

No cwd? Current directory.

No name

No name? Name is Tab #.

@jaeheonji
Copy link
Member Author

@zolrath Thank you for your comments.
This is what I'm trying to implement.

@imsnif

But, In this process, the default_layout set in config is not applied.
For example, If the user specified default_layout as compact in the config file, when NewTab is executed, compact should be imported by default. But, to apply cwd this doesn't work.

Becuase, Layout::stringified_from_path_or_default function basically returns the default layout.

To solve this problem, I parsed config first before parsing KDL of keybinds, and modified the code so that config options can be used in keybinds.

So, it now works exactly as intended for cwd and name.
Can you review it again?

@jaeheonji jaeheonji temporarily deployed to cachix January 24, 2023 14:17 — with GitHub Actions Inactive
Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jaeheonji - great work on this!

I added some comments that (I think) address your concerns. Let me know what you think.

@jaeheonji jaeheonji temporarily deployed to cachix February 1, 2023 10:37 — with GitHub Actions Inactive
@jaeheonji jaeheonji temporarily deployed to cachix February 16, 2023 08:18 — with GitHub Actions Inactive
@imsnif
Copy link
Member

imsnif commented Feb 16, 2023

@jaeheonji - I'm about to merge a pretty big PR today/tomorrow that will cause lots and lots of conflicts here. I don't know what the status here is, but if you're able to merge this before - it will save a lot of work :)

@jaeheonji
Copy link
Member Author

@imsnif Thanks for letting me know!

I'll be updating it today. then if you think my update needs more fixing, I'm fine with putting off this PR for a later time.

@imsnif
Copy link
Member

imsnif commented Feb 16, 2023

I trust your judgement, @jaeheonji ! If you think it's good, please merge.

@jaeheonji jaeheonji temporarily deployed to cachix February 16, 2023 11:56 — with GitHub Actions Inactive
@jaeheonji jaeheonji merged commit 00cd336 into zellij-org:main Feb 16, 2023
joshheyse pushed a commit to joshheyse/zellij that referenced this pull request Mar 11, 2023
* fix(themes): missing tokyo-night-dark theme

* feat: add args to new-tab action

* fix: name can be set without layout

* feat: pass config options to action parser

* chore: remove unnecessary default values

* chore: update snapshots

* fix(status-bar): add exception for NewTab

* fix: update code review

* feat: add shallow_eq for action
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add possibility to define a layout to use when opening a new tab with a keybinidng
3 participants