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

[WIP][RFC] Open tmp window: handle case where tmp window is prev window #488

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Maltimore
Copy link
Contributor

@Maltimore Maltimore commented Jan 16, 2023

This is my first ever contribution to an nvim plugin, so take everything with a grain of salt.

This PR fixes an issue when the win_split_mode is set such that no new window is opened, but instead the agenda/capture buffer are opened in the current window. See the following excerpt of my org config:

  win_split_mode = function(bufname) -- open agenda buffer in same window
    local bufnr = vim.api.nvim_create_buf(false, true)
    -- Setting buffer name is required
    vim.api.nvim_buf_set_name(bufnr, bufname)
    local current_window_nr = vim.api.nvim_tabpage_get_win(0)
    vim.api.nvim_win_set_buf(current_window_nr, bufnr)
  end,

There is a problem when "finalizing" the capture (with <C-c> by default), namely that it tries to close the temporary window. But a temporary window had never been opened.

I wonder if this PR is at all going in the right direction. I haven't run the test suite yet as I haven't yet learned how that works. I'll hopefully get to it in the next days unless you tell me now that this PR doesn't make sense.

@Maltimore Maltimore marked this pull request as draft January 16, 2023 20:43
Copy link
Contributor

@jgollenz jgollenz left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @Maltimore 👍

end
else
local prev_bufnr = vim.api.nvim_buf_get_var(0, 'org_prev_buffer')
vim.api.nvim_set_current_buf(prev_bufnr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best to also run the checks on bufnr.

if prev_bufnr and vim.api.nvim_buf_is_valid(prev_bufnr) ...

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, what should happen if the bufnr is invalid? With winnr the window gets closed in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point(s). I suppose if the prev_bufnr is invalid, we should simply unload the capture buffer and vim will automatically switch to some other buffer?

@jgollenz
Copy link
Contributor

You can run the tests with

make test

if it's the first time you run them, be sure to get the dependencies before that

git clone https://github.com/nvim-treesitter/nvim-treesitter
git clone https://github.com/nvim-lua/plenary.nvim

All those are ran from the root of the repository

@Maltimore Maltimore force-pushed the capture_no_new_window branch from b05d0a3 to 87476e3 Compare January 26, 2023 20:05
@Maltimore Maltimore force-pushed the capture_no_new_window branch from 87476e3 to 421f6a2 Compare January 26, 2023 20:11
@Maltimore
Copy link
Contributor Author

I pushed a change incorporating your suggestion, thanks again.
I thought about what to do in case prev_bufnr is invalid, and I concluded not to do anything. My first impulse was to then delete the buffer for which the tmp window was opened, but since this is such a general function (not specific to capture), we don't know what the purpose of the tmp window might've been, and we shouldn't just go around deleting buffers.

@jgollenz
Copy link
Contributor

jgollenz commented Mar 2, 2023

Hey @Maltimore, sorry it took me so long to respond. When the bufnr is invalid, the capture is written to the destination and we are left with an empty file. I'm not exactly sure yet, the trail led me to capture/init.luas _refile_to. Perhaps it's because we are closing the actual capture buffer without saving. But that's beside the point of this MR. This looks good to me 👍 Please, just add a warning that tells the user that no match was found both fore the prev_winnr and the prev_bufnr. I'm not even sure what would cause such a situation, but just in case.

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.

2 participants