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 moving tab to other position #3047

Merged

Conversation

devzbysiu
Copy link
Contributor

@devzbysiu devzbysiu commented Dec 31, 2023

This PR adds a possibility to move tab to other position (#1656)

Details:

  • when we want to move the first tab to the left or the last tab to the right, the implementation wraps the tabs around (i.e.: moving first tab left will make this tab the last one, moving last tab right will make it the first one), so it switches the tab with the tab on the left/right as if the tabs are in circular buffer. This implementation allows changing tabs without the need to move all of the other tabs to different position. See the demo below.
  • at this moment, the key bindings are set (in normal mode) to Alt + i for moving tab left and Alt + o for moving tab right (no particular reason for those bindings, I just picked something available and close to "hjkl row")
  • major changes are:
    • changelog - changed separately after merge
    • added tip for this feature, here I also did small refactor of the tip files (moved the repeated strings! macro to the main module file and reused it in all tip files)
    • e2e tests, here I moved the steps definition to separate file so as not to clutter the cases file + snapshots
    • the feature itself (protobuf changes for new action and direction of move, handling the action) + unit tests

Demo:

  • first I'm changing the tab name
  • then showing the contents of all tabs
  • then moving the tab around
  • again showing contents of all tabs
  • again some moving around
zellij-tabs-showcase.webm

Edit:
Demo of more complex scenarios:

zellij-tabs-complex-showcase.webm

@Andrew15-5
Copy link

I think a much visually impressive demo would be if each tab had multiple panes, this also would serve as a test/reassurance that this feature does work with multi-pane tabs. Or maybe some tabs have 1 pane, and some tabs have multiple panes.

@devzbysiu
Copy link
Contributor Author

I think a much visually impressive demo would be if each tab had multiple panes, this also would serve as a test/reassurance that this feature does work with multi-pane tabs. Or maybe some tabs have 1 pane, and some tabs have multiple panes.

That's a good point. Added demo with more complex scenarios (multi-pane, single-pane, sync tabs).

@jaeheonji
Copy link
Member

jaeheonji commented Jan 7, 2024

@devzbysiu

It really looks cool! I'll review it soon :)

Before that, I have one request. Please remove the CHANGELOG.md in changes. I will apply this after merging the PR seperatly.

@devzbysiu
Copy link
Contributor Author

Hi @jaeheonji, thank you :)

I'll revert CHANGELOG.md asap. I'll also fix the tests (my bad, I focused on e2e and missed cargo xtask test 🤦‍♂️ )

@mmirus
Copy link

mmirus commented Jan 7, 2024

Thanks for working on this feature! I'm a new user and was missing it.

Copy link
Member

@jaeheonji jaeheonji left a comment

Choose a reason for hiding this comment

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

I'm sorry. The review is late due to a personal issue. I haven't run the test yet, but I left a few comments. 😃

zellij-utils/src/data.rs Outdated Show resolved Hide resolved
zellij-utils/assets/prost/api.action.rs Outdated Show resolved Hide resolved
zellij-server/src/screen.rs Outdated Show resolved Hide resolved
zellij-server/src/screen.rs Outdated Show resolved Hide resolved
default-plugins/status-bar/src/tip/data/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@jaeheonji jaeheonji left a comment

Choose a reason for hiding this comment

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

Great! I think we are almost done.

Functionally I think I'm ready to merge now. But before that, I suggest some minor refactoring. Please take a look.

zellij-utils/src/kdl/mod.rs Outdated Show resolved Hide resolved
zellij-server/src/screen.rs Outdated Show resolved Hide resolved
zellij-server/src/screen.rs Outdated Show resolved Hide resolved
@devzbysiu
Copy link
Contributor Author

@jaeheonji perfect. Thank you for the review 👍 . I'll take a look asap. Probably this weekend

@jaeheonji
Copy link
Member

@devzbysiu Great :)

Actually, We plan to make a new release this weekend or next week. And we're hoping to include this feature in the next version. So, if you need any help, please let me know! then, I will update the refactoring part.

@jaeheonji
Copy link
Member

LGTM. Really Good!

After CI is passed, I will merge as soon as possible.

@jaeheonji jaeheonji merged commit dd5ea26 into zellij-org:main Feb 18, 2024
6 checks passed
@janos-r
Copy link

janos-r commented Mar 19, 2024

Omg after years finally!
Now I assume
#3080 and
#1656
can be closed if the feature gets released and works

@DanielAndreasen
Copy link

Speaking of, when can we expect this feature in a new release?

@giyany
Copy link

giyany commented Mar 21, 2024

It's highly likely this release happens in April, but please don't come after me if it takes longer.

@youcefs21
Copy link

Hi, I know you said to not come after you if it takes longer, but it is almost end of may now, and I'd love an update on the progress

@DanielAndreasen
Copy link

It's in the latest release here: https://github.com/zellij-org/zellij/releases/tag/v0.40.1
And it works wonderfully! 🎉

@boblehest
Copy link

It's in the latest release here: https://github.com/zellij-org/zellij/releases/tag/v0.40.1 And it works wonderfully! 🎉

Is it described in the release notes? I can't see any mention of it.

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.

None yet

9 participants