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

draft rename #197

Merged
merged 9 commits into from
Jul 20, 2019
Merged

draft rename #197

merged 9 commits into from
Jul 20, 2019

Conversation

JurisMajors
Copy link
Contributor

@JurisMajors JurisMajors commented Jul 2, 2019

Done:

It should add the current filename to the execution line with the cursor at the end

It should work with filenames with spaces without the need for quotes

It should use the underlying operating system call with os.Rename

  • Create parent path when renamed to a path

We should ideally check the destination file existence before and prompt user for an overwrite for safety, kind of like the same prompt mechanism as delete.

TODO:

It should move the current file selection to the same file if its position is changed

Prompt for creating parent directories


Prompting for creating the parent directories could get messy with current structure, let me know if we want to do that at all or if you have any nice ideas on how to do it.

Moving the file selection to the renamed file with app.nav.sel, I could not get it working, if you have any ideas let me know.

@JurisMajors
Copy link
Contributor Author

So I got the prompt for replacing files working, however, currently it does not change the selection. Therefore, to see the file info (after renaming) you must manually Ctrl+R. After we get file selection working it should be fine.

The prompt works by caching the rename oldpath and newpath and using that after getting an answer for prompt.

@JurisMajors
Copy link
Contributor Author

Now also a prompt for making a new directory works. All is left to do is move the selection to the newly renamed file.

@gokcehan
Copy link
Owner

gokcehan commented Jul 5, 2019

@JurisMajors I tried this patch and it seems to be working fine. I think we should also allow overwriting rename command much like delete and paste commands. To move the selection, I think we first need to reload the directory as the previous content is changed after rename. Currently it does not seem to show the new file name at all if you don't reload (it is updated on the next tick if you set period option).

@JurisMajors
Copy link
Contributor Author

@gokcehan Hey, I did put some time in to this now, the renewing works (without set period, however, I cannot get the selection working consistently. I assume I am misunderstanding how the select commands are supposed to work or what preconditions it must satisfy. Currently, for some reason, it only works when you rename to a file including a new directory (because it simply cd's in that directory). I believe the changed directories are not loaded in the cache and it still processes the old ones. If you have time, I'd appreciate if you could look at it and afterwards I could add the documentation for everything.

@gokcehan
Copy link
Owner

@JurisMajors Sorry, I have been quite busy and I couldn't get back to this. This is likely due to directory reloading being an asynchronous operation. There is an ugly hack in nav.sel to manually make a stat call and insert a dummy &file{FileInfo: lstat} to the directory. I tried this and it kind of works but somehow it is broken when the file is moved to different directories. Solution to this problem is likely along this line but there are still some things that I don't understand. I will try to make this work when I have some time though I'm not sure when I can do that. Feel free to work on this if you want. If you can come up with a somewhat working solution we can merge it and leave the cleaning for later.

@JurisMajors
Copy link
Contributor Author

@gokcehan
I gave it another shot, however, even with the clue that the loading being asynchronous (which certainly is the issue atm) I could not get the fileselection change working. If you could please commit the half-working solution, I could possibly use it as a starting off point to make it proper, because its not really clear for me what you did.

I tried to do some steps manually, like cd'ing in the directory, removing directory from cache, then calling nav.loadDir on it. However, dir.loading never goes to false in that scenario (which is weird way of implementing it) and then after wards call dir.sel(). This way attempting to wait for the directory to be loaded and only then do the select call. Thing I have not tried yet is loading the directory myself and updating the file list, but that could get messy if new directories are created.

Anyway, this seems like a weird issue to have, I'll be postponing working on this until you or somebody else either does a half-working solution or a working solution on which I can then continue work on top and possibly improve it.

@gokcehan
Copy link
Owner

@JurisMajors Sorry for dragging this too long. I think we can merge this as it is and we can work on file selection later on. I had tried something along the line:

diff --git a/nav.go b/nav.go
index 603745a..16e3c0d 100644
--- a/nav.go
+++ b/nav.go
@@ -752,11 +752,20 @@ func (nav *nav) rename(ui *ui) error {
        if err := os.Rename(oldPath, newPath); err != nil {
                return err
        }
-       nav.renew()
-       // TODO: change selection
-       if err := nav.sel(newPath); err != nil {
+
+       lstat, err := os.Stat(newPath)
+       if err != nil {
                return err
        }
+
+       base := filepath.Base(newPath)
+
+       last := nav.dirs[len(nav.dirs)-1]
+       last.files = append(last.files, &file{FileInfo: lstat})
+       last.sel(base, nav.height)
+
+       nav.renew()
+
        return nil
 }

The thing is, the way I described it, it should already be working with nav.sel in your patch and I don't know why it doesn't work. I don't think it is a good idea to add this without understanding the code.

It seems that the documentation update is missing in your patch. We can merge this and I can update the documentation if you want. Also do you think we should change the default keybinding to r which is the current binding in the example configuration? I try to keep the defaults lowercase with a few exceptions so users can bind uppercase letters for custom commands.

@JurisMajors JurisMajors marked this pull request as ready for review July 20, 2019 14:44
@JurisMajors
Copy link
Contributor Author

@gokcehan I had the R just for testing purposes since my old rename was r. I did go generate and updated the documentation. Feel free to merge it or make the necessary adjustments to it.

@gokcehan gokcehan merged commit bfd01e5 into gokcehan:master Jul 20, 2019
gokcehan added a commit that referenced this pull request Jul 16, 2020
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