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

Improve oldfiles finder #657

Merged
merged 10 commits into from Mar 18, 2021

Conversation

jesseleite
Copy link
Contributor

@jesseleite jesseleite commented Mar 14, 2021

This adds options to the oldfiles finder to make it optionally more useful as a MRU (most recently used) finder.

  • include_current_session
    • Normally v:oldfiles only lists files from past vim sessions. This option will include recently accessed files in the current vim session using :buffers! t (the ! bang shows unlisted buffers after they are closed, and the t modifier sorts the list by MRU).
  • cwd_only
    • This option filters the results to only show files within the cwd. This makes for a nice project-specific-MRU finder.

At the moment both options don't change default oldfiles finder behaviour, as they are both opt-in. I might argue making include_current_session=true a default, having to restart vim to see new entries populated into v:oldfiles is kind of weird. Either way though, it'd just be nice to have the option!


PS. I know there was an issue/discussion at #433 around MRU pickers that was closed because of the (very cool) frecency extension, but I'd argue that MRU and frecency are different things:

  • Frecency = Frequency + Recency
  • MRU = Recency only

@kkharji
Copy link
Member

kkharji commented Mar 14, 2021

Instead of filereadable, vim.loop.fs_lstat is faster.

if not vim.loop.fs_lstat(file) then 
  print("file do 'not' exist")
end

Additionally, there is a vim.loop function to get current working directory, I believe it's vim.loop.cwd()

Other then that the pr LGTM, very smart and useful feature, well done 👍🏻

@jesseleite
Copy link
Contributor Author

jesseleite commented Mar 14, 2021

Haha, pardon my VimL, still learning the Neovim ropes!

I understand the difference between vim.fn and vim.api, but after reading the friendly manual, still a bit confused about the difference between vim.api and vim.loop.

Anyway, thank you for the kind words ❤️

@elianiva
Copy link
Member

still a bit confused about the difference between vim.api and vim.loop

vim.api is Neovim's specific APIs and vim.loop is basically libuv binding in lua using luv. If you want the docs for vim.loop, they're here, just substitute uv with vim.loop or create a local variable like local uv = vim.loop

@kkharji
Copy link
Member

kkharji commented Mar 15, 2021

I agree current session should br included by default but we should have an option to disable it if for some reason others would like to only browse old sessions files

@jesseleite
Copy link
Contributor Author

jesseleite commented Mar 15, 2021

Cool. One more thought...

As soon as you open a file, it gets added to the finder. I'm just realizing that fzf's :History finder doesn't inject the current/active file as the first result. This way you can open the history finder and just hit enter to go to the last opened file.

Maybe I should do the same here? I don't see how showing the current/active file in this finder is useful, since it's not 'old' if it's 'active'. If the user quits Vim, it'll still be tracked by v:oldfiles for the next session.

@kkharji
Copy link
Member

kkharji commented Mar 15, 2021

Yah it does not make sense to me as well.

One last thing before we merge, conni told that fs_lstat might break for symlinks so we need to use fs_stat instead

@kkharji
Copy link
Member

kkharji commented Mar 15, 2021

@jesseleite I'm testing it our right now and for some reason I see files I never opened with the same directory. Can you check please

@jesseleite
Copy link
Contributor Author

jesseleite commented Mar 15, 2021

@tami5 cwd_only=true is opt-in. By default, it's not scoped to cwd. The idea is oldfiles would be all old files, regardless of cwd:

  • :Telescope oldfiles
    • Show all oldfiles history, including oldfiles in current session.
  • :Telescope oldfiles cwd_only=true
    • Same as above, but scoped to cwd.

Or maybe I'm misunderstanding, which files are you seeing that you never opened?

@kkharji
Copy link
Member

kkharji commented Mar 15, 2021

Oh sorry my bad, I did open those files, and I forgot.

Okay, so the issue now is when I run :Telescope oldfiles in, say my nvim config, I don't get the recent files I opened at the top, instead the recent files in that directory gets promoted to the top and that defeats the point of mru, I could just use find_files. Thus, we need a way to; yes keep the most recent in current directory around, but not at the top, because the mess up the order.

Does it make sense?

@jesseleite
Copy link
Contributor Author

jesseleite commented Mar 15, 2021

Sorry, I don't understand.

The whole finder orders the exact same way as v:oldfiles does. It doesn't matter which directory those files come from, it just sorts by most recently accessed. This is how v:oldfiles works, and it's how fzf's :History finder works as well. If you open it and hit enter on the first result, it'll open the last file you accessed.

If you use the cwd_only=true option, the whole list is just filtered down to only show files that exist within that cwd, but the order does not change at all. If you open it and hit enter on the first result while using the cwd_only=true option, it'll open the last file you accessed within that cwd.

Maybe we can do a screen call or something for me to better understand your issue? I'm on the Telescope Gitter as well if you want to DM me there!

@Conni2461
Copy link
Member

Whats missing here? Does anyone need something from me? Code looks good to me (first glance) :)

@kkharji
Copy link
Member

kkharji commented Mar 17, 2021

Whats missing here? Does anyone need something from me? Code looks good to me (first glance) :)

Sorry, couldn't test today and I'm about to sleep.

It looks good but I'm quite unsure if it will introduce undesirable experience.

Last time I tested it, I had an issue with the order in which files get sorted. I noticed it doesn't get sorted based on recently opened files, rather somehow prompting current directory files.

Conni can you please test, and check is this still the case?

Basically, checkout the pr -> open few files in specific order , close them (maybe restart neovim) and launch oldfiles picker.

Thanks

@jesseleite
Copy link
Contributor Author

Last time I tested it, I had an issue with the order in which files get sorted. I noticed it doesn't get sorted based on recently opened files, rather somehow prompting current directory files.

@tami5 I'm still not sure what you mean here, but I'm all for another set of eyes and thorough testing 🙂

@Conni2461 I'd be glad to answer any questions, screenshare, DM on gitter, whatever is helpful 👍

@kkharji kkharji merged commit d4bf118 into nvim-telescope:master Mar 18, 2021
@kkharji
Copy link
Member

kkharji commented Mar 18, 2021

Well done @jesseleite

@jesseleite jesseleite deleted the improve-oldfiles-finder branch March 18, 2021 12:38
@jesseleite
Copy link
Contributor Author

Thank you for help and encouragement 🙌

@kristijanhusak
Copy link

@jesseleite I was testing this today, and I still don't like the idea of showing buffers that are not listed. For example I'm using https://github.com/tpope/vim-dadbod and it creates some temporary files that prints out the results, or some scratch buffers that I don't really need.

Could we add an option to hide the buffers that are not listed?

@jesseleite
Copy link
Contributor Author

@kristijanhusak I do have a vim.loop.fs_stat() (essentially filereadable()) check around each unlisted buffer though, which means dadbod should be creating actual files on your filesystem.

Try this...

  • Open a vim-dadbod buffer
  • Close nvim
  • Re-open nvim
  • echo v:oldfiles
  • You should see that dadbod file at the top of your v:oldfiles list, right?

If we hide unlisted files, then it presents a new bug, which is the same bug that fzf's :History finder has. If you open a file and then immediately close it, you won't see it in your :Telescope oldfiles or fzf :History UNTIL you restart nvim and get the new v:oldfiles list. That's awkward, because I know I just had that project file open, and I know it'll be in my v:oldfiles for the next vim session.

That said, I'd rather show dadbod files, than hide the user's actual project files that SHOULD be in the list when the user has closed those buffers. Nawmean?

@kristijanhusak
Copy link

@jesseleite it does create files, in tmp folder, but buffers created for those files are scratch buffers. Once neovim is restarted, those files are gone, since they were created in another neovim session, and tmp folders are created per session. v:oldfiles returns them, but they are no longer readable so they are not shown.
I'm ok to have this as an opt-in, and leave the default functionality as it is now.

@jesseleite
Copy link
Contributor Author

@kristijanhusak I think the fix may then be to ignore scratch files in the tmp folder, rather than ignore all unlisted buffers (which again includes valid user project files that they recently edited and closed). Looking into this! 👍

Maybe we ignore scratch/tmp files by default, and add an option like include_tmp_files=true if people actually want access to all that in the finder.

@kristijanhusak
Copy link

I think that setting is too specific. I believe most of the people who comes from FZF will expect this to work as it worked there. I never had a problem from missing buffers after I delete them, probably because I got used to it in FZF.
I don't see why there wouldn't be an option like only_buflisted that would be disabled, and if someone wants same functionality like FZF, they just set it to true.

@jesseleite
Copy link
Contributor Author

I don't see why it would ever be desirable to open Telescope oldfiles and NOT see a file in there that I knew that I just edited, only to have it show after restarting vim 🤷‍♂️

@kristijanhusak
Copy link

Habit. I'm used to see only things that I'm working with. You could say I'm using this as a buffer switcher, but I need MRU sort.

@jesseleite
Copy link
Contributor Author

jesseleite commented Mar 18, 2021

You could say I'm using this as a buffer switcher, but I need MRU sort.

Why not then use :Telescope buffers sort_lastused=true? ❤️

@kristijanhusak
Copy link

Because it's not really sorting the whole list by MRU. It just preselects alternate buffer differently.

@jesseleite
Copy link
Contributor Author

What you are describing then sounds like a possible PR to the buffers finder, to make sort_lastused actually sort by MRU. I could get behind that!

  • :Telescope buffers - lists open buffers, with whatever sorting options you set on it
  • :Telescope oldfiles - lists recently opened and/or closed files, which does as advertised

@kristijanhusak
Copy link

Yeah, i guess it needs to be something like that. It's up to telescope collaborators to decide.

@kkharji
Copy link
Member

kkharji commented Mar 29, 2021

I really don't like this new changes, I'm trying to open recent file in the current directory and I can't see it. I just see ALL files in current directory.

Is it possible that files get add oldfiles when previewing them. Is it possible that files is merged into oldfiles?

@jesseleite
Copy link
Contributor Author

@tami5 What do you mean you see ALL files in current directory?

@kkharji
Copy link
Member

kkharji commented Mar 29, 2021

I mean like what you would expect find_files to return.

Okay it's seems to be an issue with my confing. :oldfiles sometimes output files I never opened + ALL of the directory files.

hmmmmmmmmm wired issue. maybe I should delete shada file

@jesseleite
Copy link
Contributor Author

@tami5 I did mention to you in gitter DM the other day about the LSP related issue I found, which lists files I never opened when certain LSPs crawl my files. Not sure if this is a Neovim bug. That said, I think I have a solution I'm going to PR shortly.

Could you try something for me though? If you disable all your LSP stuff and delete your shada file, does oldfiles still list files you never intentionally opened?

@kkharji
Copy link
Member

kkharji commented Mar 29, 2021

@jesseleite oh yes indeed, I wasn't expecting that bug to be related to this issue. Yah it's a lsp thing for sure, lua lsp seems to causing this from time to time.

That would be awesome thanks @jesseleite

@YodaEmbedding
Copy link

YodaEmbedding commented Apr 10, 2021

Is there a way to also include other entries from other file pickers, e.g. builtin.git_files?

:Telescope oldfiles cwd_only=true alternate=git_files

This would be a way to inject MRU behavior into other file pickers.

@jesseleite
Copy link
Contributor Author

@YodaEmbedding That might get weird IMO, as many finders aren't file based (ie. command finder, etc). This oldfiles finder is pretty coupled to the concept of v:oldfiles. That said, there's a prompt history PR here which you might be interested in: #521

@thraizz
Copy link
Contributor

thraizz commented Sep 6, 2021

@tami5 I did mention to you in gitter DM the other day about the LSP related issue I found, which lists files I never opened when certain LSPs crawl my files. Not sure if this is a Neovim bug. That said, I think I have a solution I'm going to PR shortly.

Could you try something for me though? If you disable all your LSP stuff and delete your shada file, does oldfiles still list files you never intentionally opened?

Did you manage to PR your solution for the LSP crawling issue? I suffer from this as well as pyright indexes all 200+ py files and completely overwrites my dear oldfiles picker :(

@jesseleite
Copy link
Contributor Author

@thraizz, I keep meaning to PR a bugfix for this. Curious for you though, which LSP(s) is/are doing it for you?

@thraizz
Copy link
Contributor

thraizz commented Sep 6, 2021

@thraizz, I keep meaning to PR a bugfix for this. Curious for you though, which LSP(s) is/are doing it for you?

pyright, i fixed it by changing :buffers! t in line 347 of internal.lua to :buffers t so unlisted-buffers (the buffers with a number + 'u' shown in :buffers! t) are ignored.

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.

None yet

7 participants