neo-tree.nvim icon indicating copy to clipboard operation
neo-tree.nvim copied to clipboard

Added ability to custom define sources for source selector

Open miversen33 opened this issue 2 years ago • 11 comments

This adds the ability to define which sources will be displayed in the source_selector.

As neo-tree's source ecosystem grows, it probably doesn't make sense to have the source_selector display every source that is registered for Neotree to use. This allows a user to enable Neotree for things (like diagnostics) without having to also have that in their source_selector.

I set the defaults to be the sensible ones that were already present (so there is no change to users unless they want the changes). Lemme know your thoughts, this is mostly a POC

miversen33 avatar Aug 11 '22 02:08 miversen33

NOTE: I changed the target to main for you, please target that for future PRs.

cseickel avatar Aug 11 '22 11:08 cseickel

I think that a better way to handle this is to configure the tab label from the source itself. It's a little more complicated for us but I think it's better because each external source can establish their own default tab_label. In this case, not defining a tab_label or setting it to nil could mean don't show it in the source selector.

So it would be like this instead:

require("neo-tree").setup({
  filesystem = { 
    tab_label = "  Files "
  },
  buffers = {
    tab_label = "  Buffers "
  },
  git_status = {
    tab_label = "  Git "
  },
  diagnostics = {
    tab_label = " 裂Diagnostics "
  },
})

We could then merge that into root level tables like you have here during the setup() call to make it easier to work with.

What do you think @miversen33 @mrbjarksen @prncss-xyz?

cseickel avatar Aug 11 '22 11:08 cseickel

I toyed with this idea instead but opted to not do it because it would be more complex (to code) and I was feeling lazy lol. I think that works just fine :)

I'll update the pr tonight and let you know once I do for further review.

miversen33 avatar Aug 11 '22 11:08 miversen33

I like your idea @cseickel. I think it's still worth falling back to the source name if tab_label is nil and only disabling the selector on false or an empty string.

mrbjarksen avatar Aug 11 '22 13:08 mrbjarksen

@mrbjarksen I feel it would make more sense to have anything being displayed on the UI to be an inclusion list as opposed to exclusion list.

By specifically including certain items (as this PR initially requests), we can ensure that the UI does not become "unwieldy" with future updates to available sources (or user configured third party sources, etc). Making it so a user must actively decided what sources they want to have displayed in this makes sense to me (IMO), from a future planning/long term maintenance perspective.

Just figured I would put my 2 cents here, if you / @cseickel still want to opt for a exclusion list as opposed to an inclusion list, I will update the PR accordingly

miversen33 avatar Aug 11 '22 13:08 miversen33

I see your point. I feel like the decision could go both ways. I think a only minority of external sources would be awkward to have in the source selector (like diagnostics) and since the selector is already an optional feature it wouldn't be too much trouble disabling those.

The more I think about it, though, the more I'm leaning towards an inclusion list, as you suggest.

mrbjarksen avatar Aug 11 '22 13:08 mrbjarksen

It's not a huge deal either way, but I personally like the inclusion list idea more. The primary reason is that I don't like to fallback to the source name in place of a thoughtful label with an icon. I would remove the fallback concept entirely and rely on the existence of a tab_label.

cseickel avatar Aug 11 '22 17:08 cseickel

Hi, sorry for the interruption. Author of #427 here.

I found out that with the current change, it (kind of) fails when the active tab is not in the sources list, because it cannot detect the active index along the way. These four variables have to be updated during the loop. https://github.com/nvim-neo-tree/neo-tree.nvim/blob/3c70bb4a8a6cf52b534673a3e64c1213a086797f/lua/neo-tree/ui/selector.lua#L267-L268

IMO, I agree with @cseickel to ditch the nil -> fallback feature and make it so that tab is not rendered when value is nil in tabs_label. But this route will make the separators kinda awkward when config.source_selector.separator.override = "active" and active tab not rendered cuz it will look like, / a / b /\ d \...

Maybe rendering the tab if is_active or config.source_selector.tabs_label[source_name] ~= nil would be a viable option?

Anyways, we have to update sources.common.commands.prev_source / next_source (i.e. keybinds <, >) and the documentation as well. I am happy to do any of the works :)

pysan3 avatar Aug 12 '22 14:08 pysan3

These are really good points:

IMO, I agree with @cseickel to ditch the nil -> fallback feature and make it so that tab is not rendered when value is nil in tabs_label. But this route will make the separators kinda awkward when config.source_selector.separator.override = "active" and active tab not rendered cuz it will look like, / a / b /\ d ...

Maybe rendering the tab if is_active or config.source_selector.tabs_label[source_name] ~= nil would be a viable option?

I think that if a source should not have a tab normally, then the entire source selector should be disabled when that source is shown.

Anyways, we have to update sources.common.commands.prev_source / next_source (i.e. keybinds <, >) and the documentation as well. I am happy to do any of the works :)

In this case, I'd say those commands should skip over what is not shown as a tab, so it should work off of the tab_labels list. That probably needs to be a part of this PR or at least be applied after this PR is complete.

cseickel avatar Aug 12 '22 14:08 cseickel

I apologize for the delay on this, life has been hectic. I plan on revising this this weekend (it is not dead) :)

miversen33 avatar Sep 06 '22 23:09 miversen33

I apologize for the delay on this, life has been hectic. I plan on revising this this weekend (it is not dead) :)

NP at all, I completely understand! Thanks for the update.

cseickel avatar Sep 07 '22 01:09 cseickel

Is there any update on this PR? This is a feature I'm very interested in with stuff like netman coming up that adds sources that I don't necessarily want to show in the source selector. If this has been orphaned I could definitely take a look at finishing up what needs to be done! Thanks to all the hard work from everyone on this already!

mehalter avatar Apr 04 '23 14:04 mehalter

@mehalter I wouldn't call it orphaned, just that I have been quite preoccupied with other stuff and haven't had the motivation to come back to this lol.

I actually want to get this completed and accepted soon as I am looking to move on to the next bit of netman which focuses on other things and not neo-tree (and thus I would like to have anything related to Neo-tree, including selfish contributions to it to make netman's life easier completed).

That said, I have absolutely no idea when I will get back around to this so if you are motivated, feel free to take a shot at this :)

miversen33 avatar Apr 04 '23 15:04 miversen33

This will be addressed after #876 has been finished out.

miversen33 avatar Apr 16 '23 18:04 miversen33

Because of how out of date this branch is, reviewing it is going to be a hot mess (and I ran into some fun issues with pulling it forward). I will close this PR out and create a new one. I apologize :(

miversen33 avatar Apr 16 '23 18:04 miversen33

New Branch can be found here: https://github.com/miversen33/neo-tree.nvim/tree/configurable-source-selector-entries

I will also tag the PR when I get it up

miversen33 avatar Apr 16 '23 18:04 miversen33