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: toggle view of multi selections #48

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

fdschmidt93
Copy link
Member

@fdschmidt93 fdschmidt93 commented Jan 7, 2022

E: as future notice, this PR probably will take some more time, but is left in its current state sort of as a recipe for users that require that feature rather soon

cc @jamestrew is this close to what you'd want? You're happy to give this a whirl and give feedback :)

toggle_sel.mp4

@Conni2461 this is like a 10mins implementation. Is a clean version of this something you think is good for telescope-core? For telescope-file-browser (more gimmicky anyways), it makes a lot of sense because multi selections are cached across file/folder browser and various levels of folders etc. Not sure about telescope-core where you only have a single view (ie single finder) anyways.

@Conni2461
Copy link
Member

Conni2461 commented Jan 7, 2022

I dont know if this is a good for core (at least right now) but i think its cool and i know how we could improve it.

So its similar to this thing nvim-telescope/telescope.nvim#1590 but rather than piping in all results we pipe in all multi_selection (basically just a subset).

So what we could do, to reduce code duplication and to make this more awesome 😆 Is something like this

require("telescope.actions.generate").pipe(prompt_bufnr, {
  prompt_title = "Stuff", -- optional
  results_title = "More stuff",
  sorter = conf.generic_sorter({}), -- optional
  finder = finders.new_iterator_finder { 
    results = ipairs(current_picker:get_multi_selection()),
    entry_maker = ... 
  },
  -- or. i think entry manager doesn't have a ipairs iterator (yet)
  finder = finders.new_iterator_finder { 
    results = current_picker.manager.ipairs(), 
    entry_maker = ... 
  },
  -- slowly support more and more options
})

So why am I thinking iterator, because right now you need to first iterate over all items put them into a table so the finder can iterate over them again to put them into the next table.

So having this more high level interface (of refresh) would clean up this PR and it should still be possible to do the toggling thing. Also it would be easier to maintain for the future.

Thoughts

@jamestrew
Copy link
Collaborator

It looks good but like you mentioned, it does cause all sorts of strange behaviors with a good chunk of the actions.

Documenting the action behaviors here:

  • toggle_all: seems fine (occasionally glitch-y graphically which we already briefly discussed)
  • remove_file: functioning as intended
  • create_file: error - finder.path is nil
  • toggle_browser: no error but still broken - again probably related to finder.path is nil
  • goto_parent_dir: nothing happens
  • toggle_hidden: nothing happens
  • open_file: with nothing selected, gives helpful message. With things selected, closes Telescope
  • rename_file: functioning as intended
  • goto_cwd: changes results_title to ./
  • goto_home_dir: changes results_title to nil
  • change_cwd: error with no selection
  • copy_file: pretends to work "... has been copied!" but obviously not the intended usage
  • toggle_selections: functioning as intended

I think many of these don't really have to be functional during the Multi Selection view. I think the actions which obviously should work are toggle_all, remove_file, toggle_browser, rename_file and toggle_selections. Maybe a couple others could be included. The rest, we can maybe get away with a message like [telescope] Return to file/folder browser to perform desired action and suppress/exit the action?

@fdschmidt93
Copy link
Member Author

@Conni2461 fully agree. Sounds like that would amount to quite the PR though 😅 I'll try to have a look into this more next week

@jamestrew thanks for the bunch of tests! I'd say this would be a rather tedious way to deal with this. Such use cases kind of make me believe maybe mappings should be associated with finders as opposed to prompt buffers and then you'd just deactivate all of the fb_actions. I'll see whether there is a holistic way to deal with these when I get around to doing the telescope PR I suppose.

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