From af6537bc66e2ea506b9640c34720ef85d4704be6 Mon Sep 17 00:00:00 2001 From: vE5li Date: Mon, 29 Jan 2024 04:08:51 +0100 Subject: [PATCH] fix(jumplist): Ctrl+o, Ctrl+i weird behavior when deleting buffers #25461 Problem: - Navigation is not always symmetric: pressing Ctrl+o n times followed by Ctrl+i n times does not always gets me back to where I started. - Invalid buffers are not skipped by Ctrl+i/o, I have to press Ctrl+i/o multiple times to get to the next/previous buffer. Solution: - Remove all entries of a buffer from the jump list when deleting it. - Don't add a new entry to the jump list if the next buffer to be displayed is already in the jump list. Closes #25365 --- src/nvim/buffer.c | 117 +++++++++++++++++-------- src/nvim/ex_cmds2.c | 2 +- test/functional/editor/jump_spec.lua | 126 +++++++++++++++++++++++++++ 3 files changed, 209 insertions(+), 36 deletions(-) diff --git a/src/nvim/buffer.c b/src/nvim/buffer.c index d597d82ee2..00cb7272c0 100644 --- a/src/nvim/buffer.c +++ b/src/nvim/buffer.c @@ -1183,6 +1183,32 @@ static int empty_curbuf(bool close_others, int forceit, int action) return retval; } +/// Remove every jump list entry referring to a given buffer. +/// This function will also adjust the current jump list index. +void buf_remove_from_jumplist(buf_T *deleted_buf) +{ + // Remove all jump list entries that match the deleted buffer. + for (int i = curwin->w_jumplistlen - 1; i >= 0; i--) { + buf_T *buf = buflist_findnr(curwin->w_jumplist[i].fmark.fnum); + + if (buf == deleted_buf) { + // Found an entry that we want to delete. + curwin->w_jumplistlen -= 1; + + // If the current jump list index behind the entry we want to + // delete, move it back by one. + if (curwin->w_jumplistidx > i && curwin->w_jumplistidx > 0) { + curwin->w_jumplistidx -= 1; + } + + // Actually remove the entry from the jump list. + for (int d = i; d < curwin->w_jumplistlen; d++) { + curwin->w_jumplist[d] = curwin->w_jumplist[d + 1]; + } + } + } +} + /// Implementation of the commands for the buffer list. /// /// action == DOBUF_GOTO go to specified buffer @@ -1205,6 +1231,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit) { buf_T *buf; buf_T *bp; + bool update_jumplist = true; bool unload = (action == DOBUF_UNLOAD || action == DOBUF_DEL || action == DOBUF_WIPE); @@ -1362,7 +1389,11 @@ int do_buffer(int action, int start, int dir, int count, int forceit) // If the buffer to be deleted is not the current one, delete it here. if (buf != curbuf) { + // Remove the buffer to be deleted from the jump list. + buf_remove_from_jumplist(buf); + close_windows(buf, false); + if (buf != curbuf && bufref_valid(&bufref) && buf->b_nwindows <= 0) { close_buffer(NULL, buf, action, false, false); } @@ -1382,40 +1413,53 @@ int do_buffer(int action, int start, int dir, int count, int forceit) if (au_new_curbuf.br_buf != NULL && bufref_valid(&au_new_curbuf)) { buf = au_new_curbuf.br_buf; } else if (curwin->w_jumplistlen > 0) { - int jumpidx = curwin->w_jumplistidx - 1; - if (jumpidx < 0) { - jumpidx = curwin->w_jumplistlen - 1; - } + // Remove the current buffer from the jump list. + buf_remove_from_jumplist(curbuf); - forward = jumpidx; - while (jumpidx != curwin->w_jumplistidx) { - buf = buflist_findnr(curwin->w_jumplist[jumpidx].fmark.fnum); - if (buf != NULL) { - // Skip current and unlisted bufs. Also skip a quickfix - // buffer, it might be deleted soon. - if (buf == curbuf || !buf->b_p_bl || bt_quickfix(buf)) { - buf = NULL; - } else if (buf->b_ml.ml_mfp == NULL) { - // skip unloaded buf, but may keep it for later - if (bp == NULL) { - bp = buf; + // It's possible that we removed all jump list entries, in that case we need to try another + // approach + if (curwin->w_jumplistlen > 0) { + // If the index is the same as the length, the current position was not yet added to the jump + // list. So we can safely go back to the last entry and search from there. + if (curwin->w_jumplistidx == curwin->w_jumplistlen) { + curwin->w_jumplistidx = curwin->w_jumplistlen - 1; + } + + int jumpidx = curwin->w_jumplistidx; + + forward = jumpidx; + do { + buf = buflist_findnr(curwin->w_jumplist[jumpidx].fmark.fnum); + + if (buf != NULL) { + // Skip unlisted bufs. Also skip a quickfix + // buffer, it might be deleted soon. + if (!buf->b_p_bl || bt_quickfix(buf)) { + buf = NULL; + } else if (buf->b_ml.ml_mfp == NULL) { + // skip unloaded buf, but may keep it for later + if (bp == NULL) { + bp = buf; + } + buf = NULL; } - buf = NULL; } - } - if (buf != NULL) { // found a valid buffer: stop searching - break; - } - // advance to older entry in jump list - if (!jumpidx && curwin->w_jumplistidx == curwin->w_jumplistlen) { - break; - } - if (--jumpidx < 0) { - jumpidx = curwin->w_jumplistlen - 1; - } - if (jumpidx == forward) { // List exhausted for sure - break; - } + if (buf != NULL) { // found a valid buffer: stop searching + curwin->w_jumplistidx = jumpidx; + update_jumplist = false; + break; + } + // advance to older entry in jump list + if (!jumpidx && curwin->w_jumplistidx == curwin->w_jumplistlen) { + break; + } + if (--jumpidx < 0) { + jumpidx = curwin->w_jumplistlen - 1; + } + if (jumpidx == forward) { // List exhausted for sure + break; + } + } while (jumpidx != curwin->w_jumplistidx); } } @@ -1511,7 +1555,7 @@ int do_buffer(int action, int start, int dir, int count, int forceit) } // Go to the other buffer. - set_curbuf(buf, action); + set_curbuf(buf, action, update_jumplist); if (action == DOBUF_SPLIT) { RESET_BINDING(curwin); // reset 'scrollbind' and 'cursorbind' @@ -1533,14 +1577,17 @@ int do_buffer(int action, int start, int dir, int count, int forceit) /// DOBUF_UNLOAD unload it /// DOBUF_DEL delete it /// DOBUF_WIPE wipe it out -void set_curbuf(buf_T *buf, int action) +void set_curbuf(buf_T *buf, int action, bool update_jumplist) { buf_T *prevbuf; int unload = (action == DOBUF_UNLOAD || action == DOBUF_DEL || action == DOBUF_WIPE); OptInt old_tw = curbuf->b_p_tw; - setpcmark(); + if (update_jumplist) { + setpcmark(); + } + if ((cmdmod.cmod_flags & CMOD_KEEPALT) == 0) { curwin->w_alt_fnum = curbuf->b_fnum; // remember alternate file } @@ -3675,7 +3722,7 @@ void ex_buffer_all(exarg_T *eap) // Open the buffer in this window. swap_exists_action = SEA_DIALOG; - set_curbuf(buf, DOBUF_GOTO); + set_curbuf(buf, DOBUF_GOTO, false); if (!bufref_valid(&bufref)) { // Autocommands deleted the buffer. swap_exists_action = SEA_NONE; diff --git a/src/nvim/ex_cmds2.c b/src/nvim/ex_cmds2.c index 842f8a4297..484b3572ab 100644 --- a/src/nvim/ex_cmds2.c +++ b/src/nvim/ex_cmds2.c @@ -405,7 +405,7 @@ buf_found: // Open the changed buffer in the current window. if (buf != curbuf) { - set_curbuf(buf, unload ? DOBUF_UNLOAD : DOBUF_GOTO); + set_curbuf(buf, unload ? DOBUF_UNLOAD : DOBUF_GOTO, true); } theend: diff --git a/test/functional/editor/jump_spec.lua b/test/functional/editor/jump_spec.lua index 717284b7d1..fe03d82164 100644 --- a/test/functional/editor/jump_spec.lua +++ b/test/functional/editor/jump_spec.lua @@ -3,6 +3,7 @@ local Screen = require('test.functional.ui.screen') local clear = helpers.clear local command = helpers.command +local dedent = helpers.dedent local eq = helpers.eq local fn = helpers.fn local feed = helpers.feed @@ -192,6 +193,131 @@ describe("jumpoptions=stack behaves like 'tagstack'", function() end) end) +describe('buffer deletion', function() + local base_file = 'Xtest-functional-buffer-deletion' + local file1 = base_file .. '1' + local file2 = base_file .. '2' + local file3 = base_file .. '3' + local base_content = 'text' + local content1 = base_content .. '1' + local content2 = base_content .. '2' + local content3 = base_content .. '3' + + local function format_jumplist(input) + return dedent(input) + :gsub('%{file1%}', file1) + :gsub('%{file2%}', file2) + :gsub('%{file3%}', file3) + :gsub('%{content1%}', content1) + :gsub('%{content2%}', content2) + :gsub('%{content3%}', content3) + end + + before_each(function() + clear() + command('clearjumps') + + write_file(file1, content1, false, false) + write_file(file2, content2, false, false) + write_file(file3, content3, false, false) + + command('edit ' .. file1) + command('edit ' .. file2) + command('edit ' .. file3) + end) + + it('deletes jump list entries when the current buffer is deleted', function() + command('edit ' .. file1) + + eq( + format_jumplist([[ + jump line col file/text + 3 1 0 {content1} + 2 1 0 {file2} + 1 1 0 {file3} + >]]), + exec_capture('jumps') + ) + + command('bwipeout') + + eq( + format_jumplist([[ + jump line col file/text + 1 1 0 {file2} + > 0 1 0 {content3}]]), + exec_capture('jumps') + ) + end) + + it('deletes jump list entries when another buffer is deleted', function() + eq( + format_jumplist([[ + jump line col file/text + 2 1 0 {file1} + 1 1 0 {file2} + >]]), + exec_capture('jumps') + ) + + command('bwipeout ' .. file2) + + eq( + format_jumplist([[ + jump line col file/text + 1 1 0 {file1} + >]]), + exec_capture('jumps') + ) + end) + + it('sets the correct jump index when the current buffer is deleted', function() + feed('') + + eq( + format_jumplist([[ + jump line col file/text + 1 1 0 {file1} + > 0 1 0 {content2} + 1 1 0 {file3}]]), + exec_capture('jumps') + ) + + command('bw') + + eq( + format_jumplist([[ + jump line col file/text + 1 1 0 {file1} + > 0 1 0 {content3}]]), + exec_capture('jumps') + ) + end) + + it('sets the correct jump index when the another buffer is deleted', function() + feed('') + + eq( + format_jumplist([[ + jump line col file/text + 1 1 0 {file1} + > 0 1 0 {content2} + 1 1 0 {file3}]]), + exec_capture('jumps') + ) + + command('bwipeout ' .. file1) + + eq( + format_jumplist([[ + jump line col file/text + > 0 1 0 {content2} + 1 1 0 {file3}]]), + exec_capture('jumps') + ) + end) +end) + describe('jumpoptions=view', function() local file1 = 'Xtestfile-functional-editor-jumps' local file2 = 'Xtestfile-functional-editor-jumps-2'