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

Register finder #275

Merged
merged 11 commits into from
Nov 23, 2020
Merged

Register finder #275

merged 11 commits into from
Nov 23, 2020

Conversation

sunjon
Copy link
Contributor

@sunjon sunjon commented Nov 22, 2020

Not super happy with the user experience so far.
Need nicer ways for editing, and more control over how you paste.

Putting the finder specific actions in actions/init.lua also feels strange, when everything else there is relatively generic.

Suggestions welcome

@sunjon
Copy link
Contributor Author

sunjon commented Nov 22, 2020

screenshot

attach_mappings = function(_, map)
-- TODO: Find a way to insert the text... it seems hard.
-- map('i', '<C-i>', actions.insert_value, { expr = true })
map('i', '<CR>', actions.insert_value)
Copy link
Member

Choose a reason for hiding this comment

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

You should use actions.goto_file_selection_edit:replace(actions.insert_value). That is a cool new api done by tj. In that way we respect user mappings. Take a look at what other builtins do with replace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into this and update tomrrow.

Copy link
Contributor Author

@sunjon sunjon Nov 23, 2020

Choose a reason for hiding this comment

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

To be honest, that mapping wasn't used and I'm not sure how :replace fits in with the other mappings. If it's still applicable, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

The thing about actions.goto_file_selection_edit:replace is that we basically replacing the users main action.
So if a user doesn't like <CR> as mapping and likes to use idk <space> he will hit <space> and we would call actions.goto_.... With that replace we basically just follow what the user expects for all individual pickers, even though they do something differently like checking out a branch or stuff. Does that makes sense.

@Conni2461
Copy link
Member

@sunjon Great to see you working on another PR 👍. Looks really good. I just left some nitpicking comments but besides that i am exited for that picker 😄

@sunjon
Copy link
Contributor Author

sunjon commented Nov 23, 2020

Perhaps not for this itteration, but I think a new sorter would suit these results better.
Currently, if my query is A, the fixed order of the list remains, but the "A" entry jumps to the top. What I think would be better, is if the results "rotated" to the match, ie. If "3" is the top entry, "4", "5","6", etc would follow.

@sunjon sunjon changed the title WIP: Register finder Register finder Nov 23, 2020
@sunjon
Copy link
Contributor Author

sunjon commented Nov 23, 2020

Remaining issues (not necessarily blocking merge):

  1. Default action. Should the default action be put()?

  2. Hide registers with no content?
    I'd have done this, but if this issue is resolved, then a user might want to use the edit functionality to put something into an empty register.

  3. Custom ordering.
    It may be that a user is only interested in the [0-9] registers. Perhaps configration to apply a "hidden_registers = { ...}` filter.

@Conni2461
Copy link
Member

1. Default action. Should the default action be put()?

Yes i think the default should be pasting the content inside the file at the current cursor pos.

3\. It may be that a user is only interested in the `[0-9]` registers.  Perhaps configration to apply a "hidden_registers = { ...}` filter.

We can also just say, that a user can define a table of registers in which he is interested.

Besides my remaining comments. I think this LGTM and when you done i think you can squash and merge.
Oh and could you also add the new builtin to the readme builtin table. That everyone sees that we have that cool new picker. Thanks

@sunjon sunjon merged commit 1246556 into nvim-telescope:master Nov 23, 2020
@sunjon sunjon mentioned this pull request Nov 23, 2020
@kkharji
Copy link
Member

kkharji commented Nov 23, 2020

well done @sunjon awesome work

kkharji pushed a commit that referenced this pull request Nov 27, 2020
…o builtin_runner_insert_issue

* 'master' of github.com:nvim-telescope/telescope.nvim:
  Refactor builtin (#287)
  fix: Use noremap when mapping. (#286)
  Fix the bug report template to suggest -u
  Add gitter tag
  feat: highlighter only
  Filter the completion of the command (#279)
  feat: Buffers rework (indicators and sorting) (#208)
  feat: v0.1 of extensions (#278)
  Register finder (#275)
  Various previewer fixes (#260)
  docs: fix builtin table formatting (#272)
  feat: Add highlights builtin (#267)
  Fixed minor typos (#271)
  Feat: Add filetypes builtin (#263)
  Fix: cwd detection of builtin.git_ (#264)
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.

3 participants