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.

Sign up for GitHub

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

Jump to bottom

selection_callback after going to parent dir #193

Closed
wants to merge 1 commit into from
Closed

selection_callback after going to parent dir #193

wants to merge 1 commit into from

Conversation

matthewgrossman
Copy link

No description provided.

@matthewgrossman matthewgrossman mentioned this pull request Nov 7, 2022
Open
fdschmidt93
fdschmidt93 reviewed Nov 7, 2022
View reviewed changes
lua/telescope/_extensions/file_browser/actions.lua
@@ -539,6 +540,7 @@ fb_actions.goto_parent_dir = function(prompt_bufnr, bypass)

finder.path = parent_dir
fb_utils.redraw_border_title(current_picker)
fb_utils.selection_callback(current_picker, current_dir_path:absolute())
Copy link
Member

Choose a reason for hiding this comment

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

Isn't finder.path already an absolute path? Is this actually required?

@fdschmidt93
Copy link
Member

I got something working locally in my own branch in this PR (3-liner, super simple):

That's the idea of the selection callback util :) glad it was that easy.

you mean such that we can let users opt in/out of this behavior?

I'm torn. I guess it's fine to have as the new default since I suppose the majority of users would appreciate it and we'd have an opt out just in case.

Also, is the first action_set.select:replace_if code snippet you posted the preferable place because it'll generically handle selecting the right entry, rather than my solution special-casing goto_parent_dir?

Wouldn't it apply to both? I guess I'd introduce one flag to opt out for both usages.

@fdschmidt93 fdschmidt93 mentioned this pull request Nov 10, 2022
Open
@fdschmidt93
Copy link
Member

This is now in master and #210 will also apply these sorts of things more universally.

@matthewgrossman
Copy link
Author

thanks! sorry I ghosted the thread, but glad we got a better-implemented solution into master 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fdschmidt93 fdschmidt93 fdschmidt93 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthewgrossman @fdschmidt93