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 highlight open files #838

Merged
merged 7 commits into from
Apr 7, 2023

Conversation

nhat-vo
Copy link
Collaborator

@nhat-vo nhat-vo commented Mar 30, 2023

Added highlight open file feature as described in #743. I'm not sure if there is any better way to cache this so that we do not have to loop over all buffers every time. Do you think adding to state is a good idea?

@miversen33
Copy link
Collaborator

I love this idea :) That said, honestly I feel like (IMO) looping over the buffers each time isn't that big of a deal, especially if we only perform said change on a Buffer Change event (such as BufDelete/BufAdd event). At the end of the day, lua is fast and a loop operation (even over several thousand items) per event loop iteration is still effectively instant. Thats not to say that thinking about "caching" or reducing the work is a bad thing, just that its probably not going to have a massive performance impact. Even less of an impact if we tie it only to event changes.

One thing I would be curious about, this will highlight all open files with the same highlight right? Is there a way we can highlight the currently focused file different if its one of the files visible in Neotree?

@nhat-vo
Copy link
Collaborator Author

nhat-vo commented Mar 30, 2023

@miversen33 Thanks for your insights! I also think that caching might not result in a noticeable impact in performance, and adding the buffer list into state seems rather complicated so I'm not sure if I should implement this.

Regarding the focused file, are you talking about the follow_current_file option?

@miversen33
Copy link
Collaborator

Regarding the focused file, are you talking about the follow_current_file option?

Yes. Does the proposed highlighting overrule that option? I am at work so I cannot test this right now lol. My main concern is that while having all open files highlighted makes a ton of sense, I wouldn't want this to supersede other options (such as follow_current_file)

@nhat-vo
Copy link
Collaborator Author

nhat-vo commented Mar 30, 2023

@miversen33 it doesn't seem so. The focused file will both has the NeoTreeFileNameOpened and the highlight from follow_current_file like this:
image

@miversen33
Copy link
Collaborator

Reading code is hard lol. It looks like you are already using the event system for triggering the highlight logic. As long as the current focused file is distinct from the other open files, LGTM :)

lua/neo-tree/sources/common/components.lua Outdated Show resolved Hide resolved
lua/neo-tree/sources/filesystem/init.lua Outdated Show resolved Hide resolved
@nhat-vo nhat-vo requested a review from cseickel March 31, 2023 06:55
lua/neo-tree/utils.lua Outdated Show resolved Hide resolved
lua/neo-tree/sources/filesystem/init.lua Outdated Show resolved Hide resolved
@nhat-vo
Copy link
Collaborator Author

nhat-vo commented Apr 6, 2023

@cseickel I have refactored the modified_buffers list to opened_buffers and added the configs as you suggested. I find opened is less ambiguous than open in describing the status, as open also refers to the action.
To make the change more uniform, I also renamed the library function in buffers.items to opened instead of open, in case you don't mind 😅.

@nhat-vo nhat-vo requested a review from cseickel April 7, 2023 12:08
Copy link
Contributor

@cseickel cseickel left a comment

Choose a reason for hiding this comment

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

This looks great, there's just one more thing. Technically, the utils module is part of the public api and changing the signature or return type of a function in the utils module that does not start with a _ is a breaking change that is not allowed within a major version: https://github.com/nvim-neo-tree/neo-tree.nvim#what-is-a-breaking-change

To work around that, can you just add a function that mimics the old get_modified_buffers()? It can be marked as deprecated using a comment:

--- DEPRECATED: This will be removed in v3
---Gets a lookup of all open buffers keyed by path with the modifed flag as the value
---@return table
M.get_modified_buffers = function()

Internally, it should just call the new function and convert it to the simple table of booleans that was returned before.

@nhat-vo
Copy link
Collaborator Author

nhat-vo commented Apr 7, 2023

@cseickel Done!

@nhat-vo nhat-vo requested a review from cseickel April 7, 2023 15:34
Copy link
Contributor

@cseickel cseickel left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@cseickel cseickel merged commit 2621f53 into nvim-neo-tree:main Apr 7, 2023
2 checks passed
@cseickel cseickel linked an issue Apr 15, 2023 that may be closed by this pull request
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.

NeoTreeFileNameOpened hightlight option doesn't seem to work
3 participants