Trigger update after copy/move

[valoq]

This adds the 'updated' member to the dir struct to trigger a manual update of the current directory after copy/move operations (when the period option is used).

Previously the directory information were only updated when the directory mtime was changed, which happens only during file creation but not after the file has finished copying.

Fixes #1417 , Fixes #1226 , Fixes #776

There are probably a few other bugs that can be fixed like this, but I would like some feedback on this approach first.

[valoq]

Also added a manual refresh to trigger the update after copy/move without requiring the period option.

[joelim-work]

Thanks for your contribution.

The only concern I have is the current change might not be thread-safe, since copyAsync/moveAsync are run in separate threads:

lf/nav.go

Lines 1501 to 1504 in d2136df

	if cp {
	go nav.copyAsync(app, srcs, dstDir)
	} else {
	go nav.moveAsync(app, srcs, dstDir)

On the other hand checkDir is generally (perhaps exclusively, but I haven't confirmed) called in the main thread.

The problem is that it's hard to prove any issues (as is the case with multithreading in general), and everything worked fine when I tested it myself. Perhaps a safer alternative is to send a command to the event loop using either app.ui.exprChan or remote (the latter notifies all instances).

I think it's worth leaving this PR open for a while to see if anyone else has comments.

[joelim-work]

Also I think you can add #1516 to the Fixes section, I suspect there might be other issues that may be fixed as well.

[valoq]

Thanks for your contribution.

The only concern I have is the current change might not be thread-safe, since copyAsync/moveAsync are run in separate threads

I have been thinking about this for a while now and maybe there is something I missed but is there actually any issue with this?
Like what is the worst case? If loadDir is executed on the wrong data because it has changed in another thread, there should be no harm done, right?
Since nav.currDir().updated = true is immediately followed by nav.loadDir(dstDir) the chance of that to happen is already very limited.

We could add a mutex, but I am not familiar enough with go to tell what that entails and if it is worth it if we do not actually solve any theoretical issue with this. Maybe someone with more experience in go/multi-threading can provide some input on this.

I think it's worth leaving this PR open for a while to see if anyone else has comments.

Agreed, I will try to add a few more use cases to this manual refresh, like after delete operation. That should cover a few more bugs and provide more chances to catch bugs and side effects.

[joelim-work]

Like what is the worst case?

For a start, nav.loadDir has write access to nav.dirCache which is a map, and without synchronization this kind of data structure could potentially get into an invalid state and cause the program to crash. Even if you add a mutex to protect nav.dirCache, it is possible that someone in the future will add unsafe code to nav.loadDir (or a function that is called by it) without realizing that it is called by multiple threads.

It is important to take thread synchronization seriously in multithreaded programs, even if it is somewhat theoretical and negative effects are rarely observed. For instance there was some issue that was causing lf to crash occasionally which I fixed in #1354, and the root cause turned out to be unsafe code that was added to the previewer thread to enhance previews.

Agreed, I will try to add a few more use cases to this manual refresh, like after delete operation. That should cover a few more bugs and provide more chances to catch bugs and side effects.

Thanks, I think it will be good if you can compile a list of scenarios that are fixed and add them to the PR description, so anyone else interested in the PR can test the changes.

[valoq]

It seems like the manual refresh trigger with nav.loadDir(dstDir) was not actually necessary since loadDir is called in the main thread anyway after copy/move operations.
I reverted the manual refresh and even without the period option the directory information are still updated as expected.

Since nav.currDir().updated = true only triggers a refresh when loadDir is executed in the main thread, there should not be any remaining issue with thread safety, correct?

[valoq]

082418c should also fix #1226

I think we could also fix #776 but I am not quite sure what the best approach would be since there does not seem to be any separate function to update the dircount in other panes. Any idea for this fix?

[joelim-work]

It seems like the manual refresh trigger with nav.loadDir(dstDir) was not actually necessary since loadDir is called in the main thread anyway after copy/move operations. I reverted the manual refresh and even without the period option the directory information are still updated as expected.

Since nav.currDir().updated = true only triggers a refresh when loadDir is executed in the main thread, there should not be any remaining issue with thread safety, correct?

I didn't realise that a manual refresh wasn't required either. In this case, I think it should work OK. Thanks for catching that.

082418c should also fix #1226

Thanks, I think it is fine to also update after running a synchronous shell command too. Please ensure these issues are added to the PR description so that they will be automatically closed when the PR is merged.

I think we could also fix #776 but I am not quite sure what the best approach would be since there does not seem to be any separate function to update the dircount in other panes. Any idea for this fix?

I did a brief test of the scenario described in #776 (comment) and it worked fine with your changes. This is essentially just an extension of the problem where load checks the mtime of the current directory but not the mtime of its contents. Of course, this change won't pick up file modifications that are made from external sources, but that would require changing load to check files in addition to directories, or use some library like fsnotify to watch files, neither of which is trivial to implement.

[valoq]

I did a brief test of the scenario described in #776 (comment) and it worked fine with your changes.

How does it solve the problem yet?
I am still trying to find the right place in the code to trigger the dircount update.
Right now getDirs is only executed on reload and to solve #776 we would need to find an appropriate method to trigger this update through renew() as well. Or is there already a more obvious solution I do not see?

[joelim-work]

How does it solve the problem yet? I am still trying to find the right place in the code to trigger the dircount update. Right now getDirs is only executed on reload and to solve #776 we would need to find an appropriate method to trigger this update through renew() as well. Or is there already a more obvious solution I do not see?

So this was the test case I was using:

1. Use the following config file:

   set dircounts true
   set info size
   

2. Create a new directory dir in the current directory
3. Open lf, which should show dir with the number 0 indicating that it's empty
4. Run $touch dir/file1
5. A synchronous shell command will call app.nav.renew at the end, which should reload the current directory if it's changed.
6. In this case while dir has changed, the current directory has not, so the dircount doesn't update. But with your changes you force the current directory to be reloaded anyway by setting updated to true.

I'm not sure why you mention getDirs here, this function is used to reinitialize the directory data structure from a given path, which is called in the following scenarios:

• Starting lf for the first time
• Calling reload, which reinitializes the internal state
• Using cd to change to a new location, which requires loading that directory and its parents

[joelim-work]

BTW one other (potentially crazy) idea I had was to use the printDir function to maintain a list of files visible in the UI, and use that to check for modifications.

This will save the need to insert 'update' statements at various points in the code, and should even work if files are changed from an external source (provided period is enabled). I have no idea how well that will work out in practice though.

[valoq]

I'm not sure why you mention getDirs here...

I misunderstood what the issue #776 was actually about.
There is another dircounts bug that may not have been reported yet.

When you change the number of files in a directory, the file number of the previous directory is not updated either and I was looking for a function where I could add update variable.

The number of files in directory a never changes unless I use manual refresh.

[joelim-work]

Oh that will be because you set the update flag on only app.nav.currDir(), but actually parent directories can be changed too. You can use something like below, but then this pretty much reloads everything and is similar to calling reload.

for _, d := range app.nav.dirs {
	d.updated = true
}

[valoq]

Actually, it seems that the dir size information is already loaded correctly but what is missing is a refresh of the dir cache in the previous pane.

checkDir is executed on nav.currDir which also updates the dir size information correctly, so after checkDir the len of currDir in the last image of the above example is already 2 but this information is never updated in the left pane.

I did not yet comprehend how the caching works in lf but there is probably an appropriate place in the code that could trigger a simple refresh of the selected directory of the left pane. A full redraw that iterates through all files isn't needed since only the current directory base will change in size (through actions in lf)

[joelim-work]

Actually, it seems that the dir size information is already loaded correctly but what is missing is a refresh of the dir cache in the previous pane.

checkDir is executed on nav.currDir which also updates the dir size information correctly, so after checkDir the len of currDir in the last image of the above example is already 2 but this information is never updated in the left pane.

I did not yet comprehend how the caching works in lf but there is probably an appropriate place in the code that could trigger a simple refresh of the selected directory of the left pane. A full redraw that iterates through all files isn't needed since only the current directory base will change in size (through actions in lf)

@valoq I'm not sure if you have found it yet, but you should take a look at the readdir function. This performs the actual loading of each directory by calling standard library function Readdirnames to get a list of files in that directory. Any files that are also directories will call Readdirnames to get the dirCount.

What this means is that a dir data structure contains a list of files, but the length of that list is acutally not tied to the dircount shown in the previous pane (parent directory). Also regarding the previous pane, I don't think there is currently a way to update a single entry - the entire directory has to be reloaded from scratch.

[valoq]

Also regarding the previous pane, I don't think there is currently a way to update a single entry - the entire directory has to be reloaded from scratch.

I opened a separate issue to address the dircounts in the previous pane. #1657

For now I think this MR is ready to merge since there is no longer any threading issue remaining.

[joelim-work]

Very well, it may not cover 100% of cases, but it is a small patch and does solve some outstanding problems. I think it is fine to merge this.