Skip to content

fix: restore previously focused window when opening file in multiple-split layout with an open preview window #560

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

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

MomePP
Copy link
Contributor

@MomePP MomePP commented Jan 17, 2025

This PR restores the original window when opening file using the oil floating window. There is an implementation to restore the focused window but only works for the toggle action.

@github-actions github-actions bot requested a review from stevearc January 17, 2025 10:31
lua/oil/init.lua Outdated
vim.api.nvim_win_close(0, false)

-- Restore previous focused window
if original_winid and vim.api.nvim_win_is_valid(original_winid) then
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give an example where the new wnid != current window id?

I was playing around with your PR, after we close the current window, the win_id seems to be the same as the original winid.

Copy link
Contributor Author

@MomePP MomePP Jan 21, 2025

Choose a reason for hiding this comment

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

in my case, there is a usage where multiple splits are used.

for example, we have 3 splits : top-left, bottom-left and one right split.

|-------|-------|
|       |       |
|-------|       |
|       |       |
|-------|-------|

Before the PR, when we focusing on the right split and opening a file using Oil --float, it would always open in the top-left split.

After the PR, it will correctly open a file in the previously focused split which is the right split.

vim.w[winid].oil_original_win = original_winid

This behavior was already implemented for toggle_float to restore win_id of the previous focused window by saving it in vim.w.oil_original_win. So, i reused the saved win_id to restore previous window before opening the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

What version of neovim? I'm trying out your example, and on my neovim, it opens the file in the original focused split window. My build for reference

NVIM v0.10.2
Build type: Release
LuaJIT 2.1.1732813678

Copy link
Contributor Author

Choose a reason for hiding this comment

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

normally, i use the nightly, but i have tested with the latest release as well. the issue still occurs.

NVIM v0.10.3
Build type: Release
LuaJIT 2.1.1736781742

Copy link
Contributor Author

@MomePP MomePP Jan 22, 2025

Choose a reason for hiding this comment

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

Ah, it was my oil config. if i mapped my keybind to call only oil.toggle_float(). it behaves as expected, opening file in the right split.

however, the issue still remains with the preview usage.
in my case, i configured my keybind to open a preview window right after toggle_float as shown.

function()
    oil.toggle_float()
    oil_utils.run_after_load(0, function()
         if oil.get_cursor_entry() then
             oil.open_preview()
         end
    end)
end

or, if i open the float window first and then press to open the preview window, the issue occurs, open in a wrong split.

Copy link
Contributor

Choose a reason for hiding this comment

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

tough catch. Close out the PR if everything works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but that mean if we want to open a file in the correct split, we can't use the preview window ?
or do we need to close the preview window first before opening a file ?

i think this PR still needed to handle these edge cases of usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

To open in the correct split, I believe oil already opens the file in the last focused window pane

@MomePP MomePP force-pushed the restore-win-focus branch from c15118b to 24e53fd Compare January 22, 2025 07:23
@MomePP
Copy link
Contributor Author

MomePP commented Jan 22, 2025

further more, i found out that it needs to call M.close() instead of handling it separately in M.select() as it is redundant to close the floating window.

@MomePP MomePP changed the title feat: restore previously focused window when openning file fix: restore previously focused window when opening file in multiple-split layout with an open preview window Jan 22, 2025
@BozeBro
Copy link
Contributor

BozeBro commented Jan 23, 2025

further more, i found out that it needs to call M.close() instead of handling it separately in M.select() as it is redundant to close the floating window.

Ig I don't see how M.close handles the edge that nvim_win_close doesn't handle.
also for M.close, it seems the purpose of it is to close the buffer, and then restore the contents of the buffer before oil was opened.

In this new case, we want to close the buffer, and open a new file in the original window which is different

@MomePP
Copy link
Contributor Author

MomePP commented Jan 23, 2025

@BozeBro

Maybe I was wrong about the root cause of this issue. So, I created a new issue where we can discuss it further. Once we identify the proper solution, we can update this PR to fix the issue.

#566

In this issue, I was able to replicate the behavior using a bare minimal config. So, you could try reproducing the issue as well.

@stevearc
Copy link
Owner

Appreciate filing the issue as well, was able to repro myself and better understand the situation. Fix looks good, thanks!

@stevearc stevearc merged commit 83ac518 into stevearc:master Jan 24, 2025
8 of 9 checks passed
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.

3 participants