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

Fixing tabmove not working properly when there's a split in pane #3371

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

Neko-Box-Coder
Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder commented Jul 6, 2024

This fixes #3072 . Please see the issue for the problem.

It now uses the first pane ID inside the tab instead of the tab ID itself.

To test this,

  1. Create 3 tabs with the middle tab that has split panes.
  2. Either open files for each tab or type something in them to differentiate them.
  3. Do tabmove -1 or tabmove 1 when you are at the middle tab.
  • If you are in master, it will fail to remove the original tab and the tab is copied and the states are linked between the original and cloned tab, instead of "moving" the tab
  • If you are in the PR branch, it will be able to "move" the tab to the desired location by creating a copy and removing the original tab.

Copy link
Contributor

@niten94 niten94 left a comment

Choose a reason for hiding this comment

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

I have not looked at code related with tabs much before so I tried looking at other files but I think the fix is correct.

@@ -198,7 +198,10 @@ func (h *BufPane) TabMoveCmd(args []string) {
idxTo = util.Clamp(idxTo, 0, len(Tabs.List)-1)

activeTab := Tabs.List[idxFrom]
Tabs.RemoveTab(activeTab.ID())
if len(activeTab.Panes) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd remove this check. If there is a tab without panes, it is a bug, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I can remove it.
I saw this check in RemoveTab() so I thought it was needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In RemoveTab() it is probably not needed either... Also RemoveTab() looks quite weird overall: it checks specifically p.Panes[0], not any other pane of the tab (not to mention that it is inconsistent with its documentation, which says that it checks the id of the tab itself).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was a bit confused as well but decided not to change it given that other places are using the pane id seems like.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "Removing redundant check on pane ID" in the commit message is gonna be confusing to the readers.

@dmaluka dmaluka merged commit a10624c into zyedidia:master Jul 7, 2024
3 checks passed
schollz added a commit to schollz/aw that referenced this pull request Jul 7, 2024
Fixing tabmove not working properly when there's a split in pane (zyedidia#3371)
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.

Tabmove duplicates tab when tab contains splits
3 participants