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

Cycle previewers with git commit and bcommit already using it #528

Merged
merged 2 commits into from Jun 14, 2021

Conversation

Conni2461
Copy link
Member

@Conni2461 Conni2461 commented Feb 12, 2021

Closes #307
Closes #627

Look at it here: https://streamable.com/sxi8fz

Continuation of #314

Edit: would be cool if we could somehow show the context of the previewer. Idk how we would do this.

@Conni2461 Conni2461 changed the title Add new previewers Cycle previewers with git commit and bcommit already using it Feb 12, 2021
@elianiva
Copy link
Member

I'm a bit confused, what do you mean by the context of the previewer ?

@Conni2461
Copy link
Member Author

Conni2461 commented Feb 12, 2021

Like in which previewer i am currently in like in which of the 3 are currently displayed:

  • previewers.git_commit_diff_to_parent.new(opts),
  • previewers.git_commit_diff_to_head.new(opts),
  • previewers.git_commit_diff_as_was.new(opts),

Like the name. I have no idea why i wrote context 🤣

@elianiva
Copy link
Member

ah, ok I see. I think "context" makes sense but calling it a "name" is easier for me to understand 😆

@Conni2461
Copy link
Member Author

We could put it where we currently have Preview but i don't think we can update this on the fly

@Conni2461
Copy link
Member Author

Half working version: https://gist.github.com/Conni2461/d02a0216e0436cd436ca5991e2c239bb :| Way to much duplication shouldn't happen in telescope and i don't know how to proceed because of the upcoming ui module.

@Conni2461
Copy link
Member Author

Conni2461 commented Feb 21, 2021

Okay i have the changing title. Here is a new video: https://streamable.com/n3b8aq

It requires a new PR for plenary. I will update this post after i submitted that PR.

Edit: requires nvim-lua/plenary.nvim#82 thats why tests are failing
Edit 2: Requires manual mappings now and probably when its merged as well :)

require('telescope').setup {
  defaults = {
    mappings = {
      i = {
        ["<C-s>"] = actions.cycle_previewers_next,
        ["<C-a>"] = actions.cycle_previewers_prev,
      },
    },
  }
}

@Conni2461
Copy link
Member Author

Rebased and added something new. diff to parent will now also add the full commit message on top. git diff as was does this also but only for git_commits not git_bcommits. So i am not sure if i like this.

Thoughts @elianiva @weilbith ?

Besides that it should be done. :)

@elianiva
Copy link
Member

GJ, works like a charm! Is it possible to switch to a custom previewer instead of predefined one? e.g file previewer to media previewer from media_files extension

@Conni2461
Copy link
Member Author

Do you want it as second previewer so you can cycle or do you just want to replace the previewer? Because yes you should just be able to do that. Like with everything 😆 Something like this:

local my_find_files = function(opts)
  opts = opts or {} -- just in case you wanna do opts. Then you have to merge tables or so
  require'telescope.builtin'.find_files {
    -- function to new previewer. Just for example bat one
    previewer = require'telescope.previewers'.cat.new(opts),
    -- or if you want to cycle
    alt_previewers = {
      -- Array of functions
      require'telescope.previewers'.cat.new(opts),
    }
  }

I tested this. Its really weird to jump between buffer and term previewers. But i love how its possible 🤣

Also if a previewer has no specific window title it will just print "Preview". So this feature works with older versions (and term previewers haven't added titles for them) but pwntester or windwp can just add them later if they want

@elianiva
Copy link
Member

Perfect, exactly what I wanted. Thank you!

@kkharji
Copy link
Member

kkharji commented Feb 26, 2021

what about auto-deduction? for example images. So that file browser and others can switch to image preview

@weilbith
Copy link
Contributor

Pretty sick. Great work! 👌

@Conni2461
Copy link
Member Author

Conni2461 commented Feb 26, 2021

I know i kinda like it :D what about message at top of diff to parent should we keep it?

Edit: thanks again for suggestion :D

@weilbith
Copy link
Contributor

weilbith commented Feb 26, 2021

what about message at top of diff to parent should we keep it?

I think it is quite useful. But if it could be independent of the diff itself would be great. But I assume this is waaaay to heavy.
Because what I would like is to focus the diff on the line that the user has currently active. So you immediately see the diff related to your current buffer location before you opened Telescope. Or rather what's closest to it. What do you think?

Edit: maybe make the commit message just another previewer that can be cycled. I guess a git commit message source would be nice in general. And if you have a a diff open and wonder about some information in the related commit message, you could cycle and read it.

@weilbith
Copy link
Contributor

weilbith commented Feb 26, 2021

And while I'm already brainstorming and telling wishes: it would be great to have another action which will not checkout this commit, but put the file into a diff view. The scrollable preview is nice, but sumtimes you need a proper split diff view and also be able to pull old hunks into your buffer.
But that would be another PR. 😬

@Conni2461
Copy link
Member Author

Don't say more. You're wishes will be granted. Today or tomorrow dont know but this week 99%. Who need different PRs. 1000+ changes PR incoming. 😆

  • Showing current line is possible. With vim.fn.search.
  • We can make another previewer just for message. Everything is possible. Telescope api is just insanely nice 😆 I just need to define it and put it in alt_previewers table.
  • Diff view should also be possible. :D

I like having so clear suggestions.

@Conni2461
Copy link
Member Author

Do we wanna highlight the message? I guess so we can highlight hash, author and date

@Conni2461
Copy link
Member Author

Conni2461 commented Feb 26, 2021

2021-02-26_19-07
Example of the new view and highlighting i just did. Open for suggestions. We can easily also highlight just email, just name. First column anything in that header basically.

Message on the other hand not because i don't know its content

@weilbith
Copy link
Contributor

weilbith commented Feb 26, 2021

Don't say more. You're wishes will be granted.

🤗


Could we have proper buffer highlighting for the "as was " file and some diff highlights for the, well yeah, diff view? In your screenshot it looks so pale. Makes it kinda hard to "see" anything. But I like the header highlights. 🙃

@Conni2461
Copy link
Member Author

That screenshot shows only git message in a new preview. I just picked one where i actually described the commit 🤣
All views should have highlighting but i am only testing lua right now so it might be broken. I am testing c later.

I am close to vimdiff on split, vsplit and tabedit mappings. Enter will still be checkout. In my opinion. Also this will only be implemented for bcommits so i think you can test and break it tomorrow (if you have the time) 🤣 .

@Conni2461
Copy link
Member Author

I think i could have made the clip better but i am too lazy to record again. How did you do that like 100 times @elianiva 😆

Here is showcase of all new features: https://streamable.com/5xu5r7

Commit follows in a sec

@Conni2461
Copy link
Member Author

Okay i am like 95% done with this here, if anyone wanna try it. I still need to implement titles for old previewers and i am going to run this branch for 1,2 days after that i kinda wanna merge and move on 😆

@Conni2461 Conni2461 force-pushed the cycle_previewers branch 3 times, most recently from 03a4642 to 705940a Compare March 10, 2021 16:21
@Conni2461
Copy link
Member Author

Everything in telescope land is done, but dynamic title breaks everything if title len > window width because there is no truncating in plenary. So i am thinking about moving all our truncate stuff in telescope to plenary. Will talk about it with tj

And i wanted to move on 😭

@ecosse3
Copy link

ecosse3 commented Apr 20, 2021

Any news? I can not wait for that 😄

@Conni2461
Copy link
Member Author

:) Something is missing in plenary to make it work. Otherwise its done here. Just a rebase. I will check in on the plenary work tomorrow maybe i can help out :)

@Conni2461
Copy link
Member Author

@VVKot I rebased this pr, so you can technically use it. Only dynamic_window_titles (or whatever i named it) might break your telescope till that plenary pr is merged. I need to go thought the whole diff again tho. I am not sure if i made a mistake while rebasing

@VVKot
Copy link
Contributor

VVKot commented May 26, 2021

Thanks @Conni2461 , I can check this out!

@Conni2461
Copy link
Member Author

I'll try to finish + merge it this evening (mostly just docs missing).

@VVKot
Copy link
Contributor

VVKot commented Jun 14, 2021

@Conni2461 sorry, I haven't circled back here - but I did check out your branch and this looks great!

@Conni2461 Conni2461 force-pushed the cycle_previewers branch 3 times, most recently from 2b51d6c to dd5a4f2 Compare June 14, 2021 19:26
- new git previewers
- jump to line in bcommit previewer
- vimdiff for bcommits
- dynamic preview window titles
- more previewers documentation

Co-authored-by: Thore Strassburg <thore@weilbier.net>
@Conni2461
Copy link
Member Author

Lets give this a try. Might not be perfect but its only implemented for commits and bcommits, so we can iterate on it in the next few weeks. Maybe more people can come up with really cool things when using the api in extensions.

Its not mapped on default so you need to setup something like this:

require('telescope').setup {
  defaults = {
    mappings = {
      i = {
        ["<C-s>"] = actions.cycle_previewers_next,
        ["<C-a>"] = actions.cycle_previewers_prev,
      },
    },
  }
}

@Conni2461 Conni2461 merged commit 6ac5ee0 into nvim-telescope:master Jun 14, 2021
@Conni2461 Conni2461 deleted the cycle_previewers branch June 14, 2021 19:50
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.

Feature: Expand shortened result paths in the preview title feat: Builtin Bcommit switch preview content
7 participants