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

Handle per-picker history in built-in history handler #2290

Open
stsewd opened this issue Dec 29, 2022 · 8 comments
Open

Handle per-picker history in built-in history handler #2290

stsewd opened this issue Dec 29, 2022 · 8 comments
Labels
enhancement Enhancement to performance, inner workings or existent features

Comments

@stsewd
Copy link

stsewd commented Dec 29, 2022

Is your feature request related to a problem? Please describe.

Currently, the default built-in history handler merges the history of all pickers, this is a problem when using different pickers (like one for files, another for symbols, git, etc), all those histories are very different.

Describe the solution you'd like

Save history per-picker, this can be done by having a file per picker, e.g ~/.local/share/nvim/telescope_history/{picker} (fzf does this).
Doesn't look like telescope gives a unique name to each picker, so the picker ID could be its prompt title to start.
There could also be a setting to have the old behavior or a setting to enable the new one.

Already wrote a custom handler that saves the history per-picker, so if you consider this improvement I can open a PR :)

Describe alternatives you've considered

I know that there is https://github.com/nvim-telescope/telescope-smart-history.nvim, but that plugin feels like a little over-kill (it requires sqlite instead of using files) for a feature that could be added in the core itself, and it also does per-cwd history, which should be fine for its own plugin (but could also be added to core if needed).

Additional context

I'm trying to migrate from fzf, where history is per "picker". But feel free to close this issue if you don't think this should be in core.

@stsewd stsewd added the enhancement Enhancement to performance, inner workings or existent features label Dec 29, 2022
@fdschmidt93
Copy link
Member

I know that there is https://github.com/nvim-telescope/telescope-smart-history.nvim, but that plugin feels like a little over-kill (it requires sqlite instead of using files) for a feature that could be added in the core itself, and it also does per-cwd history, which should be fine for its own plugin (but could also be added to core if needed).

Personally, I fully agree. At some point I started re-writing telescope-smart-history.nvim with vim.json because I got very annoyed that the sqlite dependency that was rather tedious to setup on remote instances which didn't have the needed libraries. At the same time, I've ended up finally managing and never saw re-writing it through ;) In fairness, I think vim.json wasn't a thing at the time of writing telescope-smart-history.nvim.

Already wrote a custom handler that saves the history per-picker, so if you consider this improvement I can open a PR :)

Something along these lines (as per motivation above) would be very greatly appreciated from my end :) My hunch is a good way to go about this would be migrating telescope-smart-history.nvim to vim.json? There's not necessarily a need to port it in core directly.

However, I'd let @Conni2461 be the judge as the original author of the functionality.

@Conni2461
Copy link
Member

Sorry for the late response.

Yeah i would love to have a more advanced history in builtin. The whole idea behind the history interface was that i wasn't sure if my actual history implementation was good enough "forever" and the current version of "smart-history" only uses sql.nvim / sqlite because it simplifies a lot of things ^^ but i can definitely understand the "over-kill" and server argument so please you're welcome to provide a PR with a new implementation (then we can archive smart-history).

Right now i think one file with vim.json makes more sense but i wouldn't make it a "legit" json file, i would do every line a new json string because then we can just append a new line async to the file and dont have to rewrite the whole file after every history addition, similar to simple-history right now. If that makes sense. Because with a json solution, i think its easier to also have the cwd argument.

Doesn't look like telescope gives a unique name to each picker, so the picker ID could be its prompt title to start.

Yep we are also already doing that for smart-history but i am not against adding a ID field or actually extracting an ID from the prompt title before we merge with the opts the user passes in, so we have actual uniqueness.

@cosminadrianpopescu
Copy link

Starting from here, I've developed this extension, which is doing exactly this. Is offering a context aware extension using vim.json for json processing. So, basically it has no dependency other than a current version of neovim.

It also offers a parameter to configure if you want to take into consideration also the current directory or not (by default, the current directory is not considered).

Please let me know what you think, and also if you think I should change the code.

@Conni2461
Copy link
Member

Conni2461 commented Feb 19, 2023

it looks pretty solid. I again wouldn't rewrite the whole file on every new prompt input, i think its a waste of resources and we can just have each line in that file a valid json object, rather than having a valid json file, that way we can just append and forget.

Do you mind opening a PR, because i think it makes sense inside core. thanks :)

@gegoune

This comment was marked as off-topic.

@Conni2461
Copy link
Member

please open an issue in the frecency extension repo

@cosminadrianpopescu
Copy link

I again wouldn't rewrite the whole file on every new prompt input, i think its a waste of resources

I am not exactly familiar with the telescope history api. I just noticed that there are some callbacks exposed in the config. Where should I move the writing? In which callback, or in which place.

we can just have each line in that file a valid json object, rather than having a valid json file, that way we can just append and forget

If I understand it correctly (append and forget), then we would have something like this:

{"Oldfiles": ["file1"]}
{"Oldfiles": ["file2"]}
{"Find files": ["file1"]}
{"Oldfiles": ["file3"]}
{"Find files": ["file2"]}

If this is the case, then it means that the limit parameter it is applied globally, rather than by context. Is this ok? I don't see an issue with it, but let me know also what do you think.

If though you are referring to this model:

{"Oldfiles": ["file1", "file2", "file3", ...]}
{"Find files": ["file1", "file2", "file3", ...]}

then I don't think that we can just append and forget. I assume you are referring to the first model, right?

Do you mind opening a PR, because i think it makes sense inside core. thanks :)

Against which repository should I open the PR?

@Conni2461
Copy link
Member

I was thinking about this structure, it creates the least amount of file operations in my opinion because we can do append only.

{ "content": "tes", "picker": "Find Files", "cwd": "/home/conni/repos/telescope.nvim" }
{ "content": "main.rs", "picker": "Find Files", "cwd": "/home/conni/repos/rust/proj" }
{ "content": "picke", "picker": "Find Files", "cwd": "/home/conni/repos/telescope.nvim" }
...

then a new history entry would be just appending this line, which makes it as fast as the already existing history, but reading it would be slower because we would need to decode every single line, but i would do that on first telescope open and after that we can cache it (like we already do) and work on two at the same time. like update cache and write to file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to performance, inner workings or existent features
Projects
None yet
Development

No branches or pull requests

5 participants