Bug in FsRenMovFileW?

[Kelimion]

Hi,

I'm writing an example filesystem plugin in Odin to help people use our programming language to write Total Commander plugins, and also to exercise our upcoming improved `core:os` replacement using a nice UI.

What it does so far:
- It reports its name (using `GetDefRootNameW`) as "Example Odin FS Plugin"
- `FsInitW` remembers the plugin number and callbacks
- `FsFindFirstW`, `FsFindNextW` and `FsFindClose` translate the WString <> utf8 and call our filesystem iterator, then translate back to the WString-based FindDataW.
- `FsGetFileW` and `FsGetFileW` work, updating the progress bar, supporting Overwrite if the destination exists

But now I'm running into what seems like a bug in `FsRenMovFileW`.
I use Shift-F6 to rename a file on the plugin FS, `FsRenMovFileW` is called with the old and new name, move and overwrite flags (and remote info).

So far, so good.
If the destination doesn't exist, e.g. LICENSE -> LICENSE.txt, then it renames no problem. `FsRenMovFileW` is called with `move` set to true.
Renaming back is also fine.

If however I have both LICENSE and LICENSE.txt and want to rename/move to overwrite the destination, `FsRenMovFileW` is still called with `move` set to true, however if I return `FS_FILE_EXISTS` as per the documentation, the user receives the following message:

Code: Select all

Error: Cannot write \\\Example Odin FS Plugin\LICENSE!

Please remove the write protection!

I'm logging the calls from Total Commander into the plugin, and I would've expected the user to receive the Overwrite dialog, same as it does with `FsGetFileW` and `FsPutFileW`, and for the procedure to be called again with the overwrite boolean set. It doesn't.

API is called, paths are translated to local path, and you can see I want to move LICENSE.txt to LICENSE

Code: Select all

[INFO ] --- [2025-05-03 16:01:01] [example_fs.odin:118:FsRenMovFileW()] 
old_path: W:\Odin\LICENSE.txt
new_path: W:\Odin\LICENSE
move: true
overwrite: false

This calls my helper procedure also used to upload (putfile) and download (getfile):

Code: Select all

[INFO ] --- [2025-05-03 16:01:01] [example_fs.odin:147:copy_or_move()] 
src: W:\Odin\LICENSE.txt
dest: W:\Odin\LICENSE
flags: bit_set[Copy_Flag; i32]{Move}
mode: Internal

And it notices the destination exists and returns `FS_FILE_EXISTS` to Total Commander.

Code: Select all

[INFO ] --- [2025-05-03 16:01:01] [example_fs.odin:156:copy_or_move()] W:\Odin\LICENSE -> .Exists

It's at this point the "Please remove the write protection!" dialog appears, even though this return value worked for uploading and downloading, and it appears to be what the manual expects.

Code: Select all

Total Commander usually calls this function twice:
- once with OverWrite==false. If the remote file exists, return FS_FILE_EXISTS. If it doesn't exist, try to copy the file, and return an appropriate error code.
- a second time with OverWrite==true, if the user chose to overwrite the file.
While copying the file, but at least at the beginning and the end, call ProgressProc to show the copy progress and allow the user to abort the operation.

Am I missing something? Total Commander (x64) 11.51, incidentally.

Best regards,
Jeroen

[Dalai]

TC doesn't show a confirmation dialog when a file in the WFX already exists. You have to implement that yourself.
See viewtopic.php?t=19444

[Kelimion]

Huh, curious. That's contrary to what the documentation says for `FsRenMovFileW` ("If the remote file exists, return FS_FILE_EXISTS"), and also not how it works for copying to/from the plugin.

Thanks. It's good to know it's a longstanding bug and not due to my API bindings. And the plugin will still be useful even with this caveat.

[AntonyD]

Are you saying that the problem has existed since 2008 in the "Known" status and nothing has been done to fix it?

[Dalai]

2Kelimion
You can still return FS_FILE_EXISTS in certain cases. My WFX plugins return that value when an exception occurs.

[Kelimion]

To Dalay:
I would expect to return `FS_FILE_NOTSUPPORTED` in that case. Or even better, `FS_READERROR` or `FS_WRITEERROR` depending on where the exception occurs.

As for implementing your own Overwrite dialog, it wouldn't look the same as the official one which is triggered when returning `FS_FILE_EXISTS` from `GetFile` or `PutFile`, and would possibly be just as surprising to the user as "Please remove the write protection!"

To AnthonyD:
> Are you saying that the problem has existed since 2008 in the "Known" status and nothing has been done to fix it?

That appears to be the case. I'm sure it just slipped Christian's mind, as in the linked thread, he says:
> The plugin rename function doesn't currently support the overwriting of already existing files, therefore you currently get an error in this case.

Emphasis on "currently" mine. Given that the API in question contains an Overwrite boolean, it seems like this functionality was intended to be added, and the notes at the bottom about returning `FS_FILE_EXISTS` weren't just copy/pasted from GetFile/PutFile.

It's a minor problem in the grand scheme of things, so I can totally see why this would've been low priority and eventually forgotten. (And it's still the best piece of software I've used ever, first to be installed on a new Windows setup.)

[ghisler(Author)]

FsRenMovFile is used for 3 things: copying, moving and in-place renaming files. In the current implementation, overwriting is only supported when copying or moving files (F5 or F6 between two directories in the plugin), but not when in place renaming. When renaming, you can only choose a different name which doesn't exist yet. Unfortunately I don't remember why I implemented it this way many years ago. There must have been a good reason, so I'm reluctant to change it now.

[Dalai]

As for implementing your own Overwrite dialog, it wouldn't look the same as the official one which is triggered when returning `FS_FILE_EXISTS` from `GetFile` or `PutFile`, and would possibly be just as surprising to the user as "Please remove the write protection!"

I use RequestProc(W) with the flag RT_MsgYesNo for that. Sure, not exactly the same but not a custom dialog to implement either.

[Kelimion]

FsRenMovFile is used for 3 things: copying, moving and in-place renaming files. In the current implementation, overwriting is only supported when copying or moving files (F5 or F6 between two directories in the plugin), but not when in place renaming. When renaming, you can only choose a different name which doesn't exist yet. Unfortunately I don't remember why I implemented it this way many years ago. There must have been a good reason, so I'm reluctant to change it now.

Thanks for the reply. That sort of thing happens.

[Kelimion]

I use RequestProc(W) with the flag RT_MsgYesNo for that. Sure, not exactly the same but not a custom dialog to implement either.

Ah, right. Good shout.

[Kelimion]

FsRenMovFile is used for 3 things: copying, moving and in-place renaming files. In the current implementation, overwriting is only supported when copying or moving files (F5 or F6 between two directories in the plugin), but not when in place renaming. When renaming, you can only choose a different name which doesn't exist yet. Unfortunately I don't remember why I implemented it this way many years ago. There must have been a good reason, so I'm reluctant to change it now.

One last observation, and then I'll go back to supporting the rest of the WFX API.

I just opened the plugin in both panels, hit F6, and gave the file a conflicting name in the resulting "Copy to new dir/name"* dialog. Total Commander now sent both move + overwrite, the combination I expected for in-place rename for an conflicting name. So it seems to me that a plugin should be expected to handle this flag combination whether or not you use Shift-F6.

* This may want to be changed to "Move to new dir/name" when you hit F6 between two plugin panels.

I use RequestProc(W) with the flag RT_MsgYesNo for that. Sure, not exactly the same but not a custom dialog to implement either.

Having given it some more thought, it's unfortunately impossible for the plugin to differentiate Shift-F6 versus F6, and that it should therefore pop-up an appropriate user dialog when the target exists.

Move is sent for both F6 and Shift-F6, and Overwrite is legitimately sent when you copy or move between two plugin panels. The absence of Overwrite requires File Exists to be returned for those cases, so if we popped up a dialog there, the user would see a spurious dialog when moving a file between two plugin panes saying they can't do something it can, and then proceeds to do if we returned File Exists.

[Dalai]

2Kelimion
FsRenMovFile's parameters OldName and NewName should contain the full path. I suggest to check whether or not the target is in the local file-system so that you know if a move or rename operation has been initiated.

[Kelimion]

2Kelimion
FsRenMovFile's parameters OldName and NewName should contain the full path. I suggest to check whether or not the target is in the local file-system so that you know if a move or rename operation has been initiated.

I don't see how checking the file helps disambiguate between move and rename. Plugins already check for it to support F5 and F6's overwrite dialog.
Let's walk through it:

- F5: A -> B
-- B exists: Total Commander doesn't send Overwrite, we need to return File Exists
-- B exists: Total Commander sends Overwrite, we can copy and return Okay
-- B doesn't exist: We can just copy and send back Okay

- F6: A -> B
-- B exists: Total Commander sends Move, we need to return File Exists
-- B exists: Total Commander sends Move and Overwrite, we can overwrite the existing file and delete the old one
-- B doesn't exist: Total Commander sends Move, we can just rename and send back Okay

- Shift-F6: A -> B
-- B exists: Total Commander sends Move. If we call RequestProc to inform the user in-place rename isn't supported, that also impacts users who typed in a conflicting filename using F6's dialog: They'd first get a dialog telling them the operation isn't supported, only for the operation then to proceed as normal.
-- B doesn't exist: Total Commander sends Move, we can just rename and send back Okay

There's no difference here whether it's one local and remote panel, or two remote panels either. B will either exist in the target panel, or it won't. Total Commander will send \-prefixed paths rather than C:\ ones, but we still need to signal back that B exists or does not, and just from \B versus C:\B we can't tell that it's a Shift-F6 rename versus an F6 move, because FsRenMovFile will always receive two \-prefixed paths, whether you're renaming in place in the remote panel and the other is a local panel and used Shift-F6, or you have two remote panels open and used F6.

How does checking the target file's existence help disambiguate between F6 and Shift-F6? I'm genuinely curious.

[AntonyD]

2ghisler(Author)
You may not want to do this in a release - but you can do it in some kind of beta version - which could be tried out by anyone interested.
Because your description of what you have done does not coincide with the logic applicable to such operations.
That is, if you once did it even for good purposes, it is very likely that the reasons for this have long disappeared.
There is no point in just leaving everything in the old state.

[Kelimion]

2ghisler(Author)
You may not want to do this in a release - but you can do it in some kind of beta version - which could be tried out by anyone interested.
Because your description of what you have done does not coincide with the logic applicable to such operations.
That is, if you once did it even for good purposes, it is very likely that the reasons for this have long disappeared.
There is no point in just leaving everything in the old state.

And as I've mentioned above, to the plugin there's no difference between Shift-F6 in one remote panel, and F6 in two remote panels as far as overwriting a renamed file is concerned. Total Commander will send the same \-prefixed source and destination paths, as well as the same Move flag. The only difference is that Shift-F6 never receives the Overwrite flag, but the plugin is still expected to handle the two panel case.

And if for some reason they don't support a move between two connections, if applicable to that plugin, then it's possible they won't support in-place rename or vice versa, and should probably return Not Supported if that's the case.