Feature/git previewer extensions

[weilbith]

Closes #307

This is my first attempt with the suggestion of @Conni2461 to just start with the new previewer functions. I extended this with some minor additional features. Still missing is the implementation of changing the previewer without restarting the picker. More detailed information in the commit messages.

I'm not too happy about the integration of the relative file path for the bcommit related preview cases. I'm open for better alternatives. E.g. I wasn't sure if the current design is meant to always only have the opts as single parameter or if the pickers can simply pass additional parameters. 🤷

[weilbith]

@Conni2461 if you might wanna have a first look? Especially regarding the treatment of the relative path. I'm still thinking about ideas how to make the preview switchable. But if you already have something in mind..

[Conni2461]

First of all thanks for your contribution. I was about to go to bed so i will have a look at your code tomorrow.

I wasn't sure if the current design is meant to always only have the opts as single parameter or if the pickers can simply pass additional parameters.

I am not sure if i understand this correctly. opts is a table and can basically hold an unlimited amount of parameters.

I'm still thinking about ideas how to make the preview switchable.

I have some ideas how we could make this possible but i let you take the shot. My tip would be take a look at lua/telescope/pickers.lua and what previewer actually does there.
Also the current picker should be available inside lua/telescope/actions/init.lua.

[weilbith]

I am not sure if i understand this correctly. opts is a table and can basically hold an unlimited amount of parameters.

Sure. The point is if all of these function always should be like foo(opts) and bar(opts) and I want to pass any new value to just foo do I need to add it as a field oft opts or would it be fine to just do foo(opts, value). I'm not too sure about the opts approach. It looks like it has a sensitive default for each picker and user can (partially) overwrite it.
I'm not too sue if that explanation was better... 🙈

---

I also thinking about improving the diff preview for bcommit to center/highlight the line that is the current active in the related buffer. Could be quite helpful. When you then search for something else you can scroll the preview. But I would say the likelihood that the current line area is of interest is higher than the top of the file. I just don't know yet how to determine this position in the preview. 🤔

[Conni2461]

Depends on if the user should be able to override or not. You could just do it and we can talk about it later, if tj or i think that could be improved. :)

I just don't know yet how to determine this position in the preview.

I can feel you i am frustrated with the previewers too 🤣. I think that is not possible with how git previews currently work. Right now they are basically just termopens and use less as a pager. I think that is fine for now but i was sketching something really cool but that would require #298 and i am not sure if it will be fast enough. So basically we could get the git --no-pager diff with a plenary job and write that to a buffer and maybe also apply treesitter hightlighting with this diff shows only one file. Then we will have a real buffer and could do something like vim.fn.search or just nvim_win_set_cursor. But i think this is currently out of scope and will require some tricks to make it fast.

[Conni2461]

Looks pretty good overall :) Good job

[rockerBOO]

Thanks for this pull request :).

I am not sure if i understand this correctly. opts is a table and can basically hold an unlimited amount of parameters.

Sure. The point is if all of these function always should be like foo(opts) and bar(opts) and I want to pass any new value to just foo do I need to add it as a field oft opts or would it be fine to just do foo(opts, value). I'm not too sure about the opts approach. It looks like it has a sensitive default for each picker and user can (partially) overwrite it.
I'm not too sue if that explanation was better... see_no_evil

The opts allows a certain amount of functional adaptations through having only 1 parameter and not a variable number.

In your example it would certainly be easier to add another parameters but in terms of like hoisting, mocking, testing it can be easier to have a common interface in the function. I think the idea is opts can be {} and it should be valid in every example. In this case they are all options and not requirements.

foo()
foo({my_option = 'red'})

If the scope of calling them opts, and they are hard requirements makes them required, then it might make sense to prefix functions with the requirements.

foo(value, opts)
foo('value', {}) 
foo('value', { my_option = 'red' })

In the case of the pickers, there are sane defaults can be applied and overwritten though optional arguments in the table. This allows composing through options.

function foo(user_opts) 
   local opts = {} or user_opts
   return function picker(opts) 
      -- now we have composing of custom functions through option passing, 
      -- the user can extend and customize the whole picker through a simple function signature. 
      -- options can be transitive and not change the function implementation that users apply
  end
end

Then you can do

opts = { picker = function(opts)  require'telescope.pickers'.something_picker(opts) end }

And customize what opts get sent to the picker, or change the picker completely, or extend the current built in ones.

Hopefully I cleared up the opts perspective. Maybe we can make the usage side easier. Merging tables is not built in so check out vim.tbl_extend, vim.extend, vim.list_extend, vim.deep_tbl_extend.

[weilbith]

@rockerBOO thanks for this exhaustive explanation. This helps me to get better into the code base and follow the design approaches.

---

I'm currently pretty busy at work with a lot of research in my free time etc. I'll continue to work on the feedback and start the preview cycling soonish.

[Conni2461]

@weilbith Are you still working on this. Do you need any more assistance?

You probably will have a bad time rebasing to current master with all the refactoring i did. I am very sorry about that.

Also i will try out new git diff previewer ideas here #354. Those will not make your previewers obsolete. It just changes the underlining API from termopen to async plenary jobs and vim buffer as pager. That should fix some issues we currently have and give us more control. To even realize some of the stuff you were talking about. Feel free to ask any questions if you are not sure were we are heading and how you should proceed.

[weilbith]

Hey
Thanks for your always kind words.
Although that is a weak excuse I'm in a down due to too much coding stress at work. So I did absolutely nothing in regards to this PR. I'm very sorry.
If this feature is more requested/awaited for feel free to just take control over it. I think my changes where to trivial and need refactor due to the review anyway, that it could be just re-done. Eventually less work than re-basing.
If you still like the current setup of "just" guiding, I would love to hear your design approach for switching the previewer. As this is the major missing functionality.

PS: I'm still watching the repository and saw your work (as you mentioned). I'm highly appreciating your contribution to a tool that helps me each day. :🙏

[Conni2461]

Although that is a weak excuse I'm in a down due to too much coding stress at work. So I did absolutely nothing in regards to this PR. I'm very sorry.

Don't be sorry thats totally fine. Its almost christmas anyway, so we should just enjoy that this miserable year is almost over 🤣 .

I just tried this new previewers idea, thought about this PR and was just checking in. I would recommend we just keep this current way of guiding, i don't wanna take away your PR and its not that urgent for me.

My design approach would be to write a Picker:set_previewer(previewer) function which will be called inside a action function where we set our new previewer. We can then map this action function to a key. And set_previewer just sets the new previewer and calls it. Setting a new previewer rather then just running it would keep the current previewer when scroll though our list and not just go back to the default one. I think that should work but i haven't tried it 😆

Edit: Also thanks for the kind works. We all just try to make it a better tool for ourselves.

[tjdevries]

Hi, I'm just going through and checking on all the PRs. Do you need anything from me here?

[Conni2461]

@weilbith I gonna pick this up in the next 2-3 days. It kinda want this to be done because its a really cool idea. :) I have not yet decided if i finish it in this PR or if i gonna open a new PR.

Let me know if you still want to do it, then I will not do it :)

[Conni2461]

Closing in favor of #528