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

[RFC] Sort buffers by time used #164

Closed
wants to merge 4 commits into from
Closed

Conversation

stevearc
Copy link
Contributor

@stevearc stevearc commented Aug 3, 2021

This is an attempt to add functionality similar to what was requested in #91. The main difference is that instead of scoring each buffer based on the number of times it was entered, we are scoring the buffer based on how long we spent inside of it.

  • We use CursorMoved and CursorMovedI to determine if the user is still active inside of a buffer
  • There's an idle timeout so we stop scoring a buffer if the user tabs out of vim, walks away from their keyboard, or falls asleep
  • The scores decay over time, so buffers that are used more recently will naturally bubble up to the top
  • Re-ordering is handled by :BufferOrderByTime to be consistent with the other ordering methods

Looking for feedback around

  • Thoughts about this time method vs the frequency method mentioned in the original issue
  • The default values for idle_timeout and time_decay_rate. They've been fine with my initial testing, but want to know if it feels too jumpy to other people.

@romgrk
Copy link
Owner

romgrk commented Aug 3, 2021

This feature is interesting, but I wonder how useful it can really be? Have you tried it? Is it pleasant to use? I wouldn't want to introduce complexity & runtime costs that won't actually be used by people.

Oh and I think I've been using single-quote convention for strings in lua, keep them consistent.

@stevearc
Copy link
Contributor Author

stevearc commented Aug 3, 2021

Fair questions. When using vim, I tend to end up with roughly two types of buffers open. There's the main 1-5 buffers that I need for my immediate task, and there's all the other buffers that I open incidentally from doing a goto-definition, navigating through the quickfix, or some other method. I like having the main buffers stay near the low position because I use to jump between them (via BufferGoto). This sorting lets me easily place the buffers that I'm using the most in those low slots. If my task changes and I end up working primarily with a different set of buffers, the exponential decay means that it doesn't take long for those new buffers to sort themselves to the left.

While it's useful for me, I also don't know if anyone else would like this functionality. If you want we can leave this up for a while and see if other people comment or leave a thumbs up. Presumably @skt041959 would find it useful, perhaps others would as well.

From a performance standpoint, unless you call :BufferOrderByTime the only overhead is the CursorMoved & BufEnter autocmds. For CursorMoved, we're just doing a couple math operations. For BufEnter, we fetch and set a couple buffer-local variables and do a bit of math. I ran a profile of switching buffers, and the logic in timing.lua was around 0.025ms
Screenshot from 2021-08-03 11-26-41
(it's the tiny blue slice)

on_leave_buffer()
end

if not is_listed(bufnr) or bufnr == active_buffer then
Copy link
Owner

@romgrk romgrk Aug 3, 2021

Choose a reason for hiding this comment

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

Here you're using is_listed to determine if the buffer is present in the tabline, but I added an exclude_ft and exclude_name options to barbar recently, you'll need to create an is_displayed function to check for presence in the tabline.

This whole logic should be extracted in that function:

local exclude_ft = opts.exclude_ft
local exclude_name = opts.exclude_name
for i, buffer in ipairs(buffers) do
if not nvim.buf_get_option(buffer, 'buflisted') then
goto continue
end
if exclude_ft ~= vim.NIL then
local ft = nvim.buf_get_option(buffer, 'filetype')
if utils.has(exclude_ft, ft) then
goto continue
end
end
if exclude_name ~= vim.NIL then
local fullname = nvim.nvim_buf_get_name(buffer)
local name = utils.basename(fullname)
if utils.has(exclude_name, name) then
goto continue
end
end

active_buffer = bufnr
end

local function on_enter_buf()
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer spelling things out long, buffer instead of buf.

@romgrk
Copy link
Owner

romgrk commented Aug 3, 2021

So if I understand correctly, this doesn't reorder anything until you call the order-by command, right?

Also, I'm really picky with adding CursorMoved* autocommands because those accumulate with every plugin. Is there a way to avoid them, or to add them only for users interested in this behavior?

Also note that I'm not at ease with the level of complexity that this brings in the codebase; if you personally find it very useful and are a strong proponent of this feature, I'm fine including it; but I'd rather not merge if it's not going to be used.

@stevearc
Copy link
Contributor Author

stevearc commented Aug 3, 2021

Correct, reordering only happens with :BufferOrderByTime

I can think of two ways to get rid of the CursorMoved:

  1. Use an option in the g:bufferline dict and only create the autocmds if it's set. The drawback is that there's a race condition: if the user sets the value after barbar is loaded, the value won't be respected.
  2. Require the user to call a function to enable the time sorting. Avoids the race condition issue, but now configuration isn't purely done by setting a variable.

FWIW, I ran a test with 10,000 individual CursorMoved autocmds calling report_activity(), and the worst case I saw was 100ms, which equates to about 10 microseconds per call. Even piling up with other plugins, that's going to take a long time before it's noticeable.

Totally understand the concerns about added complexity. The only thing I'll say is that while this PR does add more logic, it's very loosely coupled. The only entrypoints to it are via the sort function and the autocmds, and they don't call into any of the other barbar internals (save the new 'is_displayed' util function). And again, I'm really fine with leaving this for a while to see if anyone else is interested! That's part of why I labeled this as a RFC. I'm just using my own fork locally, so it doesn't affect me at all.

@romgrk
Copy link
Owner

romgrk commented Aug 4, 2021

Alright, let's leave this around for now, we'll see if there is more interest. I'll comment on the code if/when we pick this up.

@stevearc
Copy link
Contributor Author

stevearc commented Oct 2, 2021

Given the lack of comments/reacts, I think it's safe to say there isn't much interest in this feature. Happy to bring it back if that changes, but there's no need to leave it dangling any longer.

@stevearc stevearc closed this Oct 2, 2021
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