mikavilpas / yazi.nvim

A Neovim Plugin for the yazi terminal file manager
MIT License
465 stars 16 forks source link

fix: use input dir if needed when changing cwd #487

Closed chenxin-yan closed 1 day ago

chenxin-yan commented 1 week ago
  1. I found a bug when trying to change cwd on startup, in which since ya process have not logged anything yet, context.ya_process.cwd is nil, making the function ineffective. Therefore, I implement it so it uses input_path in this case.
  2. I modified the conditional to check if the last_directory equals cwd when the function is called, in which cwd does not need to be changed.
mikavilpas commented 1 week ago

I have hunch why that might be - when yazi is started, yazi.nvim also starts ya (the command line utility that listens for yazi's events). It is currently not possible to start ya if yazi is not running (https://github.com/sxyazi/yazi/issues/1314), so initial events cannot be read and will be lost.

I think this is a really good idea. I will check that the tests are still valid and then merge it 👍🏻

mikavilpas commented 1 week ago

Hmm, I made some changes and I'm no longer sure if it fits your use case. Could you give it a try before merging?

I noticed the general case works, but it looks like if I do the following operations, it's a bit weird:

In this case, it seems to change back to the original directory. I think it should maybe do nothing.

chenxin-yan commented 1 week ago

Yes, I tested it, and you are right. When opening Yazi with the current file, it works fine because input_path:parent() is the target directory. However, when open Yazi with cwd, input_path is already the target directory, when use input_path:parent() it would move to its parent. I think what we need to do is probably handle this case specifically.

I will try to test if it works but I think it would be fine to check when calling the function whether input_path is a directory or file. If it is a file, last_directory is its parent directory; If it is a directory, last_directory is that directory.

chenxin-yan commented 5 days ago

Could you take a look at the changes? It seems like it cannot pass two of the tests keybinding_helpers change_working_directory uses yazi's input_path if no cwd is available yet.

mikavilpas commented 4 days ago

Just tried it, and it seems the previous case still fails. Does it work for you?

chenxin-yan commented 4 days ago

It works perfectly for me. Which command did you use to start Yazi? Do you start it in cwd or the current file's directory?

chenxin-yan commented 4 days ago

I found another bug with yazi.toggle() that is not caused by this PR might be the issue. It seems like if previous_state.last_hovered is a directory and not a file when passing it in to yazi.setup(), yazi would start inside of that directory instead of a startup in its parent directory and select that directory.

mikavilpas commented 3 days ago

Yes, last_hovered can be either a file or a directory. That may be the cause of the bug.

Here's how I'm able to reproduce it:

  1. start yazi with a key that calls :Yazi
  2. in yazi, go to another directory
  3. input your keybinding for change_working_directory
  4. close yazi
  5. start yazi with :Yazi toggle. Yazi in opened in the new directory from step 2
  6. again input your keybinding for change_working_directory like before (step 3)
    • the cwd is changed to the first cwd from step 1
    • it should have been changed to the directory from step 3
chenxin-yan commented 3 days ago

I cannot reproduce the issue on my side. After pressing the keybinding in step 6, nothing happens. The only thing I notice when trying to debug is that if the last hover is a directory when trying to resume, yazi would start inside the directory hovered. I guess the expected behavior would be to start in the last opened directory with the hover on previously selected directory.

chenxin-yan commented 3 days ago

I cannot reproduce the issue on my side. After pressing the keybinding in step 6, nothing happens. The only thing I notice when trying to debug is that if the last hover is a directory when trying to resume, yazi would start inside the directory hovered. I guess the expected behavior would be to start in the last opened directory with the hover on previously selected directory.

Also, I tried to roll back to the commit before this feature was implemented. The problem still exists. I think it would be valuable to take at the implementation of yazi.toggle() and maybe a find a better way to implement it.

mikavilpas commented 2 days ago

Hmm, I see. I think the bug I described is not serious and realistically almost nobody will ever hit it.

I cannot reproduce the issue on my side. After pressing the keybinding in step 6, nothing happens. The only thing I notice when trying to debug is that if the last hover is a directory when trying to resume, yazi would start inside the directory hovered. I guess the expected behavior would be to start in the last opened directory with the hover on previously selected directory.

This sounds like it could be due to https://github.com/sxyazi/yazi/issues/1314 like I described in a previous message. Starting yazi and hovering a specified directory is being discussed can be tracked here https://github.com/sxyazi/yazi/issues/1610

I added some changes to make sure the feature is tested with its current behaviour. Can you make sure it still works for you? I can merge it in after that.

chenxin-yan commented 2 days ago

I added code to handle the case when input_path points to a file. In that case, last_directory should be its parent. It works for me now. Everything looks good. Thanks!

mikavilpas commented 1 day ago

Thanks!