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: expand file browser functionality #1257

Closed

Conversation

fdschmidt93
Copy link
Member

@fdschmidt93 fdschmidt93 commented Sep 18, 2021

Supersedes #613

Closes #584 #1337

Large share of the creds of course to @elianiva and everyone helping on the original PR

The builtin.file_browser now

  • can toggle between a {file, folder}_browser (where file_browser shows the currently picked folder) which keep multi selections in sync via caching entries by absolute path
  • Enables copying(+), deleting*, moving(+), renaming, creating, and opening (with system default app) files (+: via multi selections, *: supports multi selections)
  • Actions are now fully remappable and combinable with other built-in actions

Note on usage:

  • Copying/Moving files: multi select and then <C-y>/y or m (moving only normal mode for now) in target directory

Demo: Moving around files across folders into another folder

fb.mp4

@fdschmidt93
Copy link
Member Author

@Conni2461 what do you think about builtin.files.{__browse_files, __browse_folders}? Something like it would be required to implement the demo and I guess broader actions (bc of refreshing), but I'm not crazy about it. Would you prefer a separate file for internal finders or something else? thanks in advance :) Then I can clean this up.

@Conni2461
Copy link
Member

I dont know. I need to think about this but (for a change) i kinda wanna do fun telescope things this week, so i dont feel like even thinking about it. I am also not that interested about these fs operations anymore. So if you wanna finish this sooner rather than later, you are on your own :)

Originally we planed a simple file_browser and an extension for the fs operations. We talked about that on gitter but you cant really search for on messages (that was idk last year beginning this year). I have one mention here on github #290 (comment)

If you can wait, we can talk about this next week, because right now i dont know.

@fdschmidt93
Copy link
Member Author

Yeah, no worries -- more than earned :) thanks for the pointer. I'll try and think about something I'd happily get merged.

@fdschmidt93
Copy link
Member Author

Ok, I think I found a satisfactory solution for the finders switch. I'm pretty happy with this now :)

@tami5 may I please ask you to review / test drive this PR whenever you have too much time this week before Conni takes a look (he's understandably too busy with more important things :))? :) Ideally in conjunction with nvim-lua/plenary.nvim#241 I'll probably keep cleaning up the plenary PR a bit further as mentioned over there.

Copy link
Member

@kkharji kkharji left a comment

Choose a reason for hiding this comment

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

Great work @fdschmidt93, I was going to test more but my dinner is ready 😭

lua/telescope/actions/init.lua Outdated Show resolved Hide resolved
lua/telescope/actions/init.lua Outdated Show resolved Hide resolved
map("n", "g", actions.goto_prev_dir)
map("i", "<C-f>", actions.toggle_browser)
map("n", "<f>", actions.toggle_browser)
map("i", "<C-w>", actions.goto_cwd)
Copy link
Member

Choose a reason for hiding this comment

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

<C-w> is so commonly used for deleting previous word. same goes for <C-f> which is char forward.

lua/telescope/actions/init.lua Show resolved Hide resolved
lua/telescope/actions/init.lua Outdated Show resolved Hide resolved
lua/telescope/builtin/files.lua Show resolved Hide resolved
map("i", "<C-h>", actions.toggle_hidden)
map("n", "<h>", actions.toggle_hidden)
map("i", "<C-g>", actions.goto_prev_dir)
map("n", "g", actions.goto_prev_dir)
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit slow when the user has g.. mappings in normal mode, it would be cool if somehow <nowait> is given (not sure that would fix it though)

lua/telescope/actions/init.lua Outdated Show resolved Hide resolved
Comment on lines +396 to +432
map("i", "<C-h>", actions.toggle_hidden)
map("n", "<h>", actions.toggle_hidden)
map("i", "<C-g>", actions.goto_prev_dir)
Copy link
Member

Choose a reason for hiding this comment

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

<C-h> make more sense then <C-g> for goto_prev_dir, no?

lua/telescope/actions/init.lua Show resolved Hide resolved
@fdschmidt93
Copy link
Member Author

Thanks already for the comments so far @tami5 :)

I'll try to incorporate them some time tomorrow (evening, probably). How do you like the UX otherwise with toggling between the file and folders picker? Something like that for navigation was what I always am missing in nvim-tree or the like, so I've built it in :) Happy to get more ideas if you have some here.

@kkharji
Copy link
Member

kkharji commented Sep 21, 2021

Thanks already for the comments so far @tami5 :)

I'll try to incorporate them some time tomorrow (evening, probably). How do you like the UX otherwise with toggling between the file and folders picker? Something like that for navigation was what I always am missing in nvim-tree or the like, so I've built it in :) Happy to get more ideas if you have some here.

I don't use nvim-tree so I'm not sure.
so far I really loved the filename<c-e> it really makes sense as well toggling between folders/files. One ux advantage might be to make folders auto toggle when the user types for example in telescope.nvim dir lua/te this shows 0 results and has / maybe here should auto-toggle folder picker. Don't you think?

@fdschmidt93
Copy link
Member Author

Update for anyone subscribed to the PR as file browser expansion seems to be a rather anticipated feature. As a beacon of hope nvim-lua/plenary.nvim#241 now only needs to undergo (most likely) Conni's review. Then I can clean up #1257 which should hopefully be more straightforward given that the branch is rather advanced.

@ikerurda
Copy link

ikerurda commented Nov 5, 2021

Great work @fdschmidt93!
I've noticed that the cwd flag when running :Telescope file_browser cwd=... or require("telescope.builtin").file_browser({ cwd = ... }) doesn't work. Could you check it out?

Also I've seen that mapping actions.move_file to fails.
And actions.toggle_browser freezes nvim and doesn't open anything (in nvim 0.5.1 MacOS).

@fdschmidt93
Copy link
Member Author

fdschmidt93 commented Nov 5, 2021

I've noticed that the cwd flag when running :Telescope file_browser cwd=... or require("telescope.builtin").file_browser({ cwd = ... }) doesn't work. Could you check it out?

Ah, right now that's path. It gets a bit funky to think about the current directory of the file browser versus the directory of the folder browser (which is nvim cwd). I have to revisit that and clean that up. (I have to check that, it's been some time since I worked on this 😅)

And actions.toggle_browser freezes nvim and doesn't open anything (in nvim 0.5.1 MacOS).

Works fine for me on nvim master. Is there any sort of error log? Maybe plenary's scandir relies on some lua version your nvim is not linked against or something? Should be easily tested with

local pscan = require "plenary.scandir"

print(vim.inspect(pscan.scan_dir(vim.loop.cwd(), {
  hidden = true,
  add_dirs = true,
  depth = 1,
})))

Does this work for you and give you files / folders of nvim cwd?

E: I've added actions.open_file and it seems to work very nicely on my system already :) very excited for this PR.

@fdschmidt93 fdschmidt93 force-pushed the feat/fb-improvements branch 4 times, most recently from aa910ee to 111c883 Compare November 5, 2021 23:02
@ikerurda
Copy link

ikerurda commented Nov 5, 2021

It works now, thanks! :)
But.. why does it fail when mapping actions.move_file to C-m?

Also, I think that previously, the mapping C-d was used to scroll down the preview window. You are overwriting it now.

@fdschmidt93
Copy link
Member Author

fdschmidt93 commented Nov 6, 2021

But.. why does it fail when mapping actions.move_file to C-m?

<C-m> sends the same key codes as <CR>, that's why it wasn't mapped by default.

Also, I think that previously, the mapping C-d was used to scroll down the preview window. You are overwriting it now.

For the time being the mappings are mostly for me to test the file browser. However, the file picker is going to be a very "active" picker and therefore I'd personally lean to have all these actions as defaults because I'd say that is what users want from the file browser (otherwise you can also use find files). In any case, you can always remap that mapping again.

local tbl = { line }
tbl.ordinal = Path:new(line):make_relative(opts.cwd)
return setmetatable(tbl, mt)
local fb_finder = function(opts)
Copy link
Member Author

@fdschmidt93 fdschmidt93 Nov 7, 2021

Choose a reason for hiding this comment

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

There's an annoying issue I'll have to look into and is probably not so easily fixed.

  1. Multi select
  2. Search
  3. Find multi selection not highlighted again

This currently happens, because this PR creates new finders eagerly in _call. This affects master in a different form.

  1. Launch file_browser
  2. Multi select
  3. Go to different folder
  4. Return & find multi selections gone

Ultimately, the multi selection is inadequately preserved since the entry is created anew from a new finder resulting in a new metatable for the entry, such that picker:is_multi_selected doesn't result in true for the same entry. I'll have to think about this some more to come up with an hopefully easy fix (maybe add selection as a table w/o metatable, if easily possible?).

E: this of course also affects live grep

Copy link
Member Author

Choose a reason for hiding this comment

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

Now the entries are cached by their absolute path to transparently remain multi selections across file and folder browsers.

@fdschmidt93 fdschmidt93 force-pushed the feat/fb-improvements branch 2 times, most recently from 4c03042 to 481d026 Compare November 9, 2021 15:46
Copy link

@ikerurda ikerurda left a comment

Choose a reason for hiding this comment

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

I think there's an error on the lines 1176 and 1177:
Wouldn't it be something like this?

set_bkm(buf, "n", "<CR>", "<cmd>lua _G.__TelescopeBatchRename()<CR>", opts)
set_bkm(buf, "i", "<CR>", "<cmd>lua _G.__TelescopeBatchRename()<CR>", opts)

@fdschmidt93
Copy link
Member Author

No, it should work all the same with or without _G. prefix. I've also checked it by hand just now, the function is properly cleared.

@ikerurda
Copy link

ikerurda commented Nov 9, 2021

I was refering also to the missing <cmd>lua and the __TelescopeBatchRename (before __TelescopeBulkRename)

Otherwise from your last commit I'm getting this error:
E492: Not an editor command: __TelescopeBulkRename()

@fdschmidt93
Copy link
Member Author

fdschmidt93 commented Nov 9, 2021

Ah sorry, too sleepy. Forget entirely what I said. I read something totally different from your comment than what you wrote. Of course you're right, must have gotten it wrong when I renamed it.

e: function works again now.

@fdschmidt93 fdschmidt93 force-pushed the feat/fb-improvements branch 2 times, most recently from dc10e1a to 027ae65 Compare November 9, 2021 22:10
@Conni2461
Copy link
Member

Yeah I think it shouldnt be here. Always should have been an extension. This was my mistake.

@fdschmidt93
Copy link
Member Author

Closing the PR accordingly and will release a beta version over at the soon-to-be made public nvim-telescope/telescope-file-browser.nvim.

Once the extension is released, we'll probably do a file-browser-ectomy of core I suppose.

@fdschmidt93
Copy link
Member Author

The extension has now made been made accessible: https://github.com/nvim-telescope/telescope-file-browser.nvim

Please consider it a beta for the time being and see the associated roadmap.

@benfrain
Copy link

Woohoo! Can’t wait to try this. Thanks for all the hard work 👍👍👍👍

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.

Renaming files and folder on Telescope file_browser
6 participants