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

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

Merged
merged 5 commits into from
Oct 2, 2024

Conversation

chenxin-yan
Copy link
Contributor

  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.

@chenxin-yan chenxin-yan force-pushed the fix-change-directory branch 2 times, most recently from 178ba98 to 8bbfecd Compare September 26, 2024 08:07
@mikavilpas
Copy link
Owner

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 (sxyazi/yazi#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
Copy link
Owner

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:

  • start yazi in directory A, move to directory B
  • change the directory with the keymapping
  • close yazi
  • start yazi again (automatically in directory B), but don't move to any directory
  • yazi opens in directory B
  • change the directory with the keymapping
    • the directory is changed to A

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

@chenxin-yan
Copy link
Contributor Author

chenxin-yan commented Sep 26, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

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

@chenxin-yan
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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 sxyazi/yazi#1314 like I described in a previous message. Starting yazi and hovering a specified directory is being discussed can be tracked here sxyazi/yazi#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
Copy link
Contributor Author

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 mikavilpas merged commit 33857bf into mikavilpas:main Oct 2, 2024
10 checks passed
@mikavilpas
Copy link
Owner

Thanks!

@chenxin-yan chenxin-yan deleted the fix-change-directory branch October 2, 2024 18:14
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