Skip to content

feat: add BEFORE_FILE_MOVE BEFORE_FILE_RENAME events #625

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

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

sidlatau
Copy link
Contributor

This will enable to change of imports of the renamed file before the actual file rename.

@cseickel
Copy link
Contributor

@sidlatau Thanks for submitting the PR. One thing I would change is that we already have an event system that would suit this situation, and I wouldn't want to add a new hooks configuration.

Here is an example with something similar:

local event_result = events.fire_event(events.FILE_OPEN_REQUESTED, {
state = state,
path = path,
open_cmd = open_cmd,
}) or {}
if event_result.handled then

And an example of that event being consumed: https://github.com/nvim-neo-tree/neo-tree.nvim/wiki/Recipes#custom-window-chooser-for-file-open-commands

@sidlatau
Copy link
Contributor Author

Thanks for the provided example. But the problem is, that in my case I need an event handler to call async code and wait until a callback is called - only then I need to return control to neo-tree to continue with file rename/move. If I add code in event_handler, it does not wait until callback is called, apply_workspace_edit will be called after control is returned to neo-tree:

  event_handlers = {
    {
      event = "before_file_rename",
      handler = function(data)
        local params = ...
	lsp_client.request("workspace/willRenameFiles", params, function(err, result)
	  vim.lsp.util.apply_workspace_edit(result, dartls_client.offset_encoding)
	end)
      end,
    },

Do you have any ideas on how to make it work in blocking way using events system?

@cseickel
Copy link
Contributor

Do you have any ideas on how to make it work in blocking way using events system?

You don't actually need to do it in a blocking way. One of the keys passed in the data object can be a callback to actually perform the rename. Then you can return { handled = true } to indicate that the event is being handled elsewhere and to not continue on to do the rename in the function that called the event.

The FILE_OPEN_REQUESTED example uses the { handled = true } technique.

@sidlatau
Copy link
Contributor Author

The FILE_OPEN_REQUESTED example uses the { handled = true } technique.

Thanks for the explanation, It finally clicked to me, how to use the events system without blocking, nice solution :) Just had an issue with events results - they are actually not returned from the fire_event_internal function. Not sure about that queue - now it will return the last event handler result - is that ok?

@cseickel
Copy link
Contributor

Just had an issue with events results - they are actually not returned from the fire_event_internal function. Not sure about that queue - now it will return the last event handler result - is that ok?

I don't think that is true, maybe there was some other mistake that gave you that impression?

If you look at the actual code change you made, the only logical difference is that if there are multiple event handlers for the same event, then it will run all of them instead of stopping at the first one that reports { handled = true }. That change is definitely incorrect. In the case where there is a single handler registered, there should not be any difference in behavior at all.

@sidlatau sidlatau changed the title feat: add on_rename_file hook feat: add BEFORE_FILE_MOVE BEFORE_FILE_RENAME events Nov 28, 2022
@sidlatau sidlatau changed the title feat: add BEFORE_FILE_MOVE BEFORE_FILE_RENAME events feat: add BEFORE_FILE_MOVE BEFORE_FILE_RENAME events Nov 28, 2022
@sidlatau
Copy link
Contributor Author

I removed changes in queue.lua, I will investigate what is going on with event handler results separately. Are the remaining changes OK?

@cseickel
Copy link
Contributor

cseickel commented Dec 2, 2022

Sorry for the delay @sidlatau, these changes look like they will work perfectly. If they are not working for you, are you returning this from your handler?

    return { handled = true }

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #625 (96b84fa) into main (52a9efd) will increase coverage by 0.14%.
The diff coverage is 0.00%.

❗ Current head 96b84fa differs from pull request most recent head 3146f72. Consider uploading reports for the commit 3146f72 to get more accurate results

@@            Coverage Diff             @@
##             main     #625      +/-   ##
==========================================
+ Coverage   50.36%   50.50%   +0.14%     
==========================================
  Files          47       47              
  Lines        6217     6164      -53     
==========================================
- Hits         3131     3113      -18     
+ Misses       3086     3051      -35     
Impacted Files Coverage Δ
lua/neo-tree/events/init.lua 100.00% <ø> (ø)
lua/neo-tree/sources/filesystem/lib/fs_actions.lua 3.84% <0.00%> (-0.14%) ⬇️
lua/neo-tree/events/queue.lua 63.92% <0.00%> (-3.98%) ⬇️
lua/neo-tree/git/status.lua 55.49% <0.00%> (-0.92%) ⬇️
lua/neo-tree/ui/popups.lua 56.79% <0.00%> (-0.36%) ⬇️
lua/neo-tree/ui/highlights.lua 93.26% <0.00%> (-0.32%) ⬇️
lua/neo-tree/ui/selector.lua 5.10% <0.00%> (ø)
...eo-tree/sources/filesystem/lib/filter_external.lua 7.40% <0.00%> (ø)
lua/neo-tree/sources/common/preview.lua 10.22% <0.00%> (+0.17%) ⬆️
lua/neo-tree/sources/manager.lua 61.09% <0.00%> (+0.30%) ⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sidlatau
Copy link
Contributor Author

sidlatau commented Dec 5, 2022

Sorry for the delay @sidlatau, these changes look like they will work perfectly. If they are not working for you, are you returning this from your handler?

    return { handled = true }

Yes, I am returning {handled = true} - I registered separate issue for this #631 where I asked more specific question about implementation.

Are there any actions from me needed to move forward this PR?

@cseickel
Copy link
Contributor

cseickel commented Dec 5, 2022

Are there any actions from me needed to move forward this PR?

No, but if the behavior it depends on is broken, then we need to fix that first so you can confirm this works as expected.

@cseickel
Copy link
Contributor

@sidlatau If you rebase this on main, the event handler should work as expected. Give it a try and let me know if this PR does what you need it to now.

This will enable to change imports of renamed file
@sidlatau
Copy link
Contributor Author

@sidlatau If you rebase this on main, the event handler should work as expected. Give it a try and let me know if this PR does what you need it to now.

Rebased the PR on main. @cseickel I confirm, that it works now.

@cseickel cseickel merged commit c5a5a3d into nvim-neo-tree:main Dec 16, 2022
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.

2 participants