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

feat: add gg and G to default mappings #1325

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

goolord
Copy link
Contributor

@goolord goolord commented Oct 9, 2021

since we have scrolling now, I think these are sensible default mappings :)

Copy link
Member

@fdschmidt93 fdschmidt93 left a comment

Choose a reason for hiding this comment

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

Generally LGTM thanks :)

Could you please make one change prior to merging though?

function actions.move_to_top(prompt_bufnr)
local current_picker = actions.get_current_picker(prompt_bufnr)
current_picker:set_selection(
p_scroller.top(current_picker.sorting_strategy, current_picker.max_results, current_picker.manager:num_results())
)
end
--- Move to the middle of the picker
---@param prompt_bufnr number: The prompt bufnr
function actions.move_to_middle(prompt_bufnr)
local current_picker = actions.get_current_picker(prompt_bufnr)
current_picker:set_selection(
p_scroller.middle(current_picker.sorting_strategy, current_picker.max_results, current_picker.manager:num_results())
)
end
--- Move to the bottom of the picker
---@param prompt_bufnr number: The prompt bufnr
function actions.move_to_bottom(prompt_bufnr)
local current_picker = actions.get_current_picker(prompt_bufnr)
current_picker:set_selection(
p_scroller.bottom(current_picker.sorting_strategy, current_picker.max_results, current_picker.manager:num_results())
)
end

For these three actions, we would need to replace

  local current_picker = actions.get_current_picker(prompt_bufnr)

with

  local current_picker = action_state.get_current_picker(prompt_bufnr)

as the former by now is deprecated.

We often abstain from adding new default mappings; however, I'd vote in favor to include this one. Once you've made the changes and no other member makes a comment, I'd merge some time tomorrow or the day after to give them sufficient time to chime in, if that's ok :)

@Conni2461
Copy link
Member

The functions are currently mapped to H/M/L because that what they previously did. I am fine with these mappings but we need new functions that allow us to jump to high, middle and low of the current screen.

Also we need a mention of them here https://github.com/nvim-telescope/telescope.nvim#default-mappings

@goolord
Copy link
Contributor Author

goolord commented Oct 11, 2021

ah, I didn't realize that the behavior of those keybindings even changed with the scrolling PR. I can try to fix them, though this is getting mildly out of the scope of this pr imo

@Conni2461
Copy link
Member

This is out of scope of this PR. Just wanted to mention it. Updating readme is enough. After that i would merge it. Thanks for the PR :)

If you wanna write these new functions that would be awesome :)

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