-
-
Notifications
You must be signed in to change notification settings - Fork 639
refactor(#2988): migrate change_dir functionality to Explorer class #3233
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
base: master
Are you sure you want to change the base?
Conversation
…all places using change_dir to use it from explorer
|
@alex-courtis this one was really interesting to refactor, I was able to really strengthen my knowledge in lua classes even though the problems weren't that hard. I will get into the next ones. |
You underestimate yourself. This was not an easy change. FYI you now have permissions to push branches directly to this repo, so you don't need to use your fork next time. |
Just a questionwhat is the profiling for? |
Base Test Case FailureSetupcd /tmp
git clone git@github.com:rvaiya/keyd.gitchange_root_to_node - failOpen nvim in
Expected: data is new root After Analysis
We can debug this by adding lines that note the explorer's unique id: -- in constructor
self.uid_explorer = vim.loop.hrtime()
log.line("dev", "Explorer:new uid_explorer=%d", self.uid_explorer)
-- destructor
function Explorer:destroy()
log.line("dev", "Explorer:destroy uid_explorer=%d", self.uid_explorer)
-- change
function Explorer:force_dirchange(foldername, should_open_view)
log.line("dev", "Explorer:force_dirchange uid_explorer=%d foldername=%s", self.uid_explorer, foldername)
---
else
log.line("dev", "Explorer:change_dir uid_explorer=%d calling renderer:draw()")
self.renderer:draw()
---You can see that the old explorer instance is destroyed and a new on created, but draw is called on the old one: RecommendationRevert to old behaviour of getting the new explorer before drawing: if should_open_view then
require("nvim-tree.lib").open()
else
-- TODO #2255
-- The call to core.init destroyed this Explorer instance hence we need to fetch the new instance.
local explorer = core.get_explorer()
if explorer then
explorer.renderer:draw()
end
endThis is not ideal at all, as an Explorer method from a destroyed instance is running. |
It's used to report performance of costly operations. We ask users to enable it when reporting Performance Issues |
Base Test Case PlanChange To Directory NodeOpen nvim in /tmp/keyd
Expected: tree root is data Change To File NodeOpen nvim in /tmp/keyd Navigate to
Expected: tree root changes to data Change Up Root NodeOpen nvim in /tmp/keyd
Expected: tree root changes /tmp is_window_eventWe must create a test cases that results in Option Test Case Plansync_root_with_cwd = trueOpen nvim in
Expected: tree root should change to data nvim-tree.lua change_rootOpen nvim in home
Expected: tree root change to /tmp/keyd nvim-tree.lua open_on_directorySet Open nvim in home Expected: tree with root /tmp is opened config.actions.change_dir.enable = trueOpen nvim in /tmp/keyd
config.actions.change_dir.enable = falseOpen nvim in /tmp/keyd
config.actions.change_dir.restrict_above_cwd = trueSet Open nvim in /tmp/keyd
config.actions.change_dir.global = trueSet Open nvim in /tmp/keyd
Switch to editor window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has turned out to be much harder than anticipated... there were a lot of landmines. We are close, please:
- Test!
- Update the remaining config paths #3233 (comment) #3233 (comment)
- Apply the sad hack to
force_dirchange#3233 (comment) - Add param annotation #3233 (comment)
| ---@private | ||
| ---@return boolean | ||
| function Explorer:should_change_dir() | ||
| return config.enable and vim.tbl_isempty(vim.v.event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return config.enable and vim.tbl_isempty(vim.v.event) | |
| return config.actions.change_dir.enable and vim.tbl_isempty(vim.v.event) |
| local valid_dir = vim.fn.isdirectory(foldername) == 1 -- prevent problems on non existing dirs | ||
| if valid_dir then | ||
| if self:should_change_dir() then | ||
| self:cd(config.global, foldername) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self:cd(config.global, foldername) | |
| self:cd(config.actions.change_dir.global, foldername) |
| function Explorer:change_dir_to_node(node) | ||
| if node.name == ".." or node:is(RootNode) then | ||
| self:change_dir("..") | ||
| elseif node:is(FileNode) and node.parent ~= nil then | ||
| self:change_dir(node.parent:last_group_node().absolute_path) | ||
| else | ||
| node = node:as(DirectoryNode) | ||
| if node then | ||
| self:change_dir(node:last_group_node().absolute_path) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for going the extra mile here - this looks so much better as this functionality should be owned by the Explorer instance.
Please do remember to add parameter annotations.
|
I will get back asap |
|
Hi @alex-courtis, I am just commenting to say that I am alive :). As I state before I've been practicing on some few small things so that I can better contribute. Currently I can contribute, I can understand the things you ask me to change and so on (sometimes need to do some researches but is ok) But there some things that I feel like "I should know this from the beginning", and I believe by improving just little bit I can better contribute to this project and start fixing bugs, not losing obvious things when doing the tasks and so on. I will be making the changes shortly. Btw I created a repo to help any people now or in the future that I pretty sure will enhance their knowledge base on playing around with lua + neovim, because lua is not neovim, so there is a need to learn lua for neovim (basically learn the apis and some real world things that can be done). The aim of the repo is to guide the user through a series of challenges (leveling up the difficulty on each) so that they easily make progress on their paths, I did this in the past with Maybe you can check and reference for any new coming contributor that enters on a situation similar to mine. the repo is Neovim Plugin Challenges. |
That is incredible! You've looked into a great variety of apis - properties, commands, uvlib etc - AND patterns like events. I've put it on the wiki, if that's OK with you: https://github.com/nvim-tree/nvim-tree.lua/wiki/Development
There is no shame at all - you acknowledge it (thus own it) and will benefit from it. |
This refactor moves directory change operations from the actions.root module into the Explorer class, improving code organization and encapsulation.
Changes:
add_profiling_to, should_change_dir, and cd
Resolves #2988