Virtually it's more correct that if `document_rename_file()` fails, `handle_save_as()` would no longer proceed to call `document_save_file_as()`. I'm not sure if unexpected things in runtime would not happen if `document_save_file_as()` somehow succeeds even if `document_rename_file()` fails, but it's certainly not what the user would be expecting. I.e., being able to save to the target file but the originally file remains to exist. This might be a rare case, but it still stands to be programmatically incorrect.
I decided to include changing `document_rename_file()` itself in this update rather than creating another non-API function, because I believe it's the only correct way to do it. Sooner or later, it would have to be changed anyway. And I don't think it's likeable that an old non-gboolean function which no code would use would exist just for the sake of not breaking the API. This should also be helpful to future code that might rely on it. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1180
-- Commit Summary --
* Make document_rename_file() return a gboolean. * Abort save if rename fails
-- File Changes --
M src/dialogs.c (6) M src/document.c (4) M src/document.h (2)
-- Patch Links --
https://github.com/geany/geany/pull/1180.patch https://github.com/geany/geany/pull/1180.diff
Not sure I totally agree that its better to not save at all if the rename fails, all that happens IIUC is an old file is left behind, the new data is still saved at the new location and the Geany buffer points to it. Better to save the buffer for safety, and then let the user handle the leftovers.
On Windows renames fail when the old file is in use. On Linux renames across file systems fail. So its quite possible that the rename fails and the save succeeds.
Also the change of `document_rename_file()` to return a value will need an API bump and an ABI bump. IMHO this isn't important enough to cause an ABI bump, it should be delayed until the next one happens.
all that happens IIUC is an old file is left behind, the new data is still saved at the new location and the Geany buffer points to it.
That's a big thing in my opinion. It's counter-intuitive that the save operation would not abort if rename fails, especially that the user is wanting a "Rename" operation. The user expects the file to be renamed and not just saved. Geany should allow the user to decide what to do with their data if an error occurs.
Better to save the buffer for safety, and then let the user handle the leftovers
Having an implicit safety measure with the unnecessary price of having things happen behind the scene is not a good thing. It's better to let the user decide what to do with it. He can still save it somewhere, either temporarily or permanently, then try to fix the issue. We should at least give meaning to the error message that we show the user everytime a rename operation fails.
I myself would not want to feel cautious about deleting the original file and checking that that changes were actually saved in the new file everytime this happens. I'd rather have things in control. It's easier to fix things that way. Manually deleting things is also an unnecessary trouble. If a user doesn't realize right away that he has an existing old file, it could give him trouble in the future.
It's also an obvious programming choice to abort an operation (and revert changes if necessary) if a crucial step before another ends up with an error. I'm not sure why this is even questioned.
Also the change of document_rename_file() to return a value will need an API bump and an ABI bump. IMHO this isn't important enough to cause an ABI bump, it should be delayed until the next one happens.
Well waiting is not a bad thing, although I'm not sure if there's even a plugin out there that uses the API function. Also, isn't an ABI bump doable to just about every release? We can just inform the users (mostly targetting distro maintainers) through the NEWS file that an API function has slightly changed.
Since this doesn't rename any function, and since the function was originally a `void`, this change would only require a plugin to be recompiled. No immediate change in its code would have to be added.
I checked `geany-plugins`. Among the plugins affected that may need to be recompiled is the binder plugin `geanypy` that compiles with the `document_rename_file()` function, but it isn't necessarily the one that calls/uses it. Recompiling the plugin is unlikely necessary unless something actually uses `Document_rename_file()`. There's also `multiterm`, but it doesn't call the wrapper function. Recompilation is also unlikely necessary.
The reason that the save should go ahead is because the operation being performed is "Save As". The rename function is only a secondary convenience function to save deleting the original file.
It is actually a poorly thought out function anyway. Why rename the existing file if it is about to be overwritten? And if you have "safe" or "GIO" file saving set the sequence is "rename original file, write new data to a new file, rename it over the just renamed original file" :-P
The "Save As" dialog should offer to "Remove original file after saving". And delete the original file after saving. Then its obvious that a failure of removal due to permissions, or any of the reasons given above, has not affected the main function to "Save As". And on save failure the original file is unharmed. And on Linux there is no restriction to the same filesystem when selecting the option.
The reason that the save should go ahead is because the operation being performed is "Save As". The rename function is only a secondary convenience function to save deleting the original file.
But there are no other ways to implement a rename operation on the file, because in the end you want to have the "document-save" signal to be put in one place. So renaming a file is much just like saving. I actually tried to separate `Rename` from `Save As` but I just ended with that problem. `Rename` in the context of Geany is a save operation, but with a necessary step that it first has to rename the source file.
And if you have "safe" or "GIO" file saving set the sequence is "rename original file, write new data to a new file, rename it over the just renamed original file" :-P
I find that unnecessary overkill. There are even more issues that you'd end up with that, and some errors to consider, and what to do when they happen like how you'd abort or how you'd revert changes (the temporary file). That also wouldn't guarantee that the file is actually renamed. If a filesystem or OS requires that a filename be not touched when some other processes are using it, then you might end up saving to the original file but not being able to rename it. To put it simply, that's a lot more complicated and is more prone to more issues.
The "Save As" dialog should offer to "Remove original file after saving".
Not a bad idea. Just rename the "R_ename" button to "Save and R_emove Original", or something else, because it would be less convenient if we remove the button and use a CheckBox. It's actually more correct that way since it's technically in the context of "saving".
Come to think of it, there's no point touching `document_rename_file()` because it's not useful being used alone, and it shouldn't be used alone since you would need to have everyone that listens to "document-save" to be updated. It probably even needs to be dropped from the API.
We better just create another function named `document_rename_file_and_save()` (i.e. `gboolean document_rename_file_and_save(GeanyDocument *doc, const gchar *new_filename);`) for anyone that would want to use it. An example for this usage is an in-place renaming of a document in Documents tab.
I find that unnecessary overkill. ....
Maybe I wasn't clear, thats not a proposal, that is what happens now with those settings, we have no choice, its what the Glib and GIO libraries do. And yes, all the possible problems you mention can happen. For more information you can look at my [PHD thesis](https://wiki.geany.org/config/all_you_never_wanted_to_know_about_file_saving) :)
Hmmm, thinking about it again, the Glib and GIO saving methods were not available back when this was first added (see below), so it wasn't quite as ridiculous then, but would still have been better to save and then delete the original.
"Save and R_emove Original"
Nice suggestion, same mnemonic and all. And rename _was_ originally a checkbox IIRC and was changed to a button (waaaaay back in 2007 or so, I suddenly feel olde) so I doubt anyone will suggest changing it back.
... in place renaming ...
Yes separating rename and save is a better idea still.
Here's a patch. It was pretty easy to make: https://github.com/konsolebox/geany/commit/e329b289df38442b96d2b9add6586b2c3...
I can create a PR if it's really wanted.
... separating rename and save is a better idea still.
In the context of stand-alone renaming yes, but if it's about a document not, because like I said every caller would make sure that all necessary listeners to "document-save" would get notified about the change. I'm not sure if it makes sense to have the GeanyDocument stay as is while the name of the file in the filesystem gets changed. I don't say any use case for it. Not to mention `document_rename_file()` already calls `document_stop_file_monitoring()` and it expects `document_save_file_as()` to be called after. We wouldn't want plugins (or anything that calls `document_rename_file()`) to always do both sequences separately.
... Ok, maybe since nothing is really changed in GeanyDocument when a file is renamed, notifying listeners to "document-save" may not be necessary (until the save operation actually starts), but both operations are still better put together in a single API function due to the fact that `document_rename_file()` can't be used alone in one operating session.
Here's a gist for the function: https://gist.github.com/konsolebox/b95bd34e518ac1d1aa60a6348bd498b9
There are surprising things about `doc->priv->file_disk_status` but I haven't looked into it thoroughly yet. Basically the idea is that `document_save_file_as()` does not revert `doc->priv->file_disk_status` to its original state if the save fails. And so does `document_rename_file()`. Simply because it can't, because it's used separately from `document_save_file_as()`.
konsolebox@e329b28
Sorry that patch does not check if save succeeded. I forgot to put it back while I was rearranging something. Here's the correct one: https://github.com/geany/geany/commit/f3b122f4d9277c61f3809286c5997e71ff4431...
Superceded by #1194 which has been closed.
Closed #1180.
github-comments@lists.geany.org