Save_pre handlers are not triggered if file is read-only #88493

New Issue

Douglas Lassance · 2021-05-23T02:55:02+02:00

Douglas Lassance commented

2021-05-23 02:55:02 +02:00

System Information
Operating system: Windows 10 Pro
Graphics card: GeForce RTX 2080

Blender Version
Broken: 2.92.0, 02948a2cab, master, 2021-02-24 16:25
Worked: I would not know if this used to work.

Short description of error
Hopefully, I am approaching something the wrong way here, but it seems that there might be a small oversight concerning the timing at which the save_pre handlers are processed. If a Blender file is read-only, the save_pre handlers will not be triggered because the Cannot save blend file, path 'C:\my\blender\file.blend' is not writable error is raised before that can happen.

As a result, a mechanism that unlocks a file before saving it cannot be implemented.
In my case, this is making it impossible to create an add-on that will integrate versioning systems such as Perforce or Git into Blender.

I suppose there are cases where you would not want to trigger the save_pre routine unless a file was actually writeable, but checking that should probably be the responsibility of whoever implements the handler.

Exact steps for others to reproduce the error

Open Blender.
Run the following Python script using the Text Editor:

import bpy
from bpy.app.handlers import persistent


@persistent
def save_pre_handler(*args, **kwargs):
    print("Doing something before saving.")
    

bpy.app.handlers.save_pre.append(save_pre_handler)

Open the system console (Window > Toggle System Console).
Save the file (File > Save).
You should see Doing something before saving. printed in the system console.
Make the file read-only (Right-click > Properties > Check Read-only checkbox).
Save the file again (File > Save).
Blender will raise an error and Doing something before saving. will not be printed in the system console.

Thanks in advance for your consideration.

**System Information** Operating system: Windows 10 Pro Graphics card: GeForce RTX 2080 **Blender Version** Broken: 2.92.0, 02948a2cab44, master, 2021-02-24 16:25 Worked: I would not know if this used to work. **Short description of error** Hopefully, I am approaching something the wrong way here, but it seems that there might be a small oversight concerning the timing at which the `save_pre` handlers are processed. If a Blender file is read-only, the `save_pre` handlers will not be triggered because the `Cannot save blend file, path 'C:\my\blender\file.blend' is not writable` error is raised before that can happen. As a result, a mechanism that unlocks a file before saving it cannot be implemented. In my case, this is making it impossible to create an add-on that will integrate versioning systems such as Perforce or Git into Blender. I suppose there are cases where you would not want to trigger the `save_pre` routine unless a file was actually writeable, but checking that should probably be the responsibility of whoever implements the handler. **Exact steps for others to reproduce the error** - Open Blender. - Run the following Python script using the Text Editor: ``` import bpy from bpy.app.handlers import persistent @persistent def save_pre_handler(*args, **kwargs): print("Doing something before saving.") bpy.app.handlers.save_pre.append(save_pre_handler) ``` - Open the system console (Window > Toggle System Console). - Save the file (File > Save). - You should see `Doing something before saving.` printed in the system console. - Make the file read-only (Right-click > Properties > Check `Read-only` checkbox). - Save the file again (File > Save). - Blender will raise an error and `Doing something before saving.` will not be printed in the system console. Thanks in advance for your consideration.

Douglas Lassance commented

2021-05-23 02:55:02 +02:00

Added subscriber: @douglaslassance

Falk David commented

2021-05-25 13:11:35 +02:00

Added subscriber: @filedescriptor

Falk David commented

2021-05-25 13:11:35 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Falk David commented

2021-05-25 13:11:35 +02:00

Looked into this. The code calls the callback after checking the following:

The filepath is not empty
The filepath is not longer than FILE_MAX
The file does not exists and is not read-only
The file does not override a file in bmain->libraries.

I can see that this is a problem, because you might want to detect these conditions in the callback like you mentioned.

Looked into this. The code calls the callback after checking the following: - The filepath is not empty - The filepath is not longer than `FILE_MAX` - The file does not exists and is not read-only - The file does not override a file in `bmain->libraries`. I can see that this is a problem, because you might want to detect these conditions in the callback like you mentioned.

Falk David commented

2021-05-26 12:55:45 +02:00

Changed status from 'Confirmed' to: 'Archived'

Falk David closed this issue

2021-05-26 12:55:45 +02:00

Falk David commented

2021-05-26 12:55:45 +02:00

Since the code is working as intended, I will close this for now.
As it is right now, error handling is done before calling the callback handlers.

Since the code is working as intended, I will close this for now. As it is right now, error handling is done before calling the callback handlers.

Douglas Lassance commented

2021-05-26 20:35:31 +02:00

Added subscriber: @ideasman42

Douglas Lassance commented

2021-05-26 20:35:31 +02:00

Since the code is working as intended this now falls under feature requests. Where is the right place to post these?

With more and more people working using VCS that tend to be designed around file locking for collaboration on binary files, addressing this feature would allow for nice integrations into Blender. I understand what @ideasman42 is saying about providing a guaranteed pre and post save pair at all times as well as backward compatibility with previously written handlers, nevertheless, any sort of opinionated code like this comes with limitations such as the one I have described in this post. There is a case for a save_pre that does exactly what it says while documentation explains the potential safety checks that the Python developers should implement. That said, I would be equally happy with another handler that runs before the current safety checks. The current workaround of creating a custom save operator and overriding shortcuts is not very elegant.

Since the code is working as intended this now falls under feature requests. Where is the right place to post these? With more and more people working using VCS that tend to be designed around file locking for collaboration on binary files, addressing this feature would allow for nice integrations into Blender. I understand what @ideasman42 is saying about providing a guaranteed pre and post save pair at all times as well as backward compatibility with previously written handlers, nevertheless, any sort of opinionated code like this comes with limitations such as the one I have described in this post. There is a case for a `save_pre` that does exactly what it says while documentation explains the potential safety checks that the Python developers should implement. That said, I would be equally happy with another handler that runs before the current safety checks. The current workaround of creating a custom save operator and overriding shortcuts is not very elegant.

Campbell Barton commented

2021-05-28 05:35:40 +02:00

As far as I know perforce is the only popular version control which relies on file locking.
IIRC unlocking a file is in is meant to be an explicit use action, so having to support this within every application that needs to save files feels like you're working against the design of the system you're using.

Having said that, we could have extra handlers to support this kind of thing since some use cases for them exist (it's not limited to unlocking files on save).

So for any action we could have:

{action}_init run before the action takes place (this is what you'd need).
{action}_pre run immediately before the action.
{action}_post run immediately after the action.
{action}_cancel runs if the action failed to run (could also be called error or fail).

This follows what we already have for the render handler.

I would accept a patch that implements these for save and load.

As far as I know perforce is the only popular version control which relies on file locking. IIRC unlocking a file is in is meant to be an explicit use action, so having to support this within every application that needs to save files feels like you're working against the design of the system you're using. Having said that, we could have extra handlers to support this kind of thing since some use cases for them exist (it's not limited to unlocking files on save). So for any action we could have: - `{action}_init` run before the action takes place (this is what you'd need). - `{action}_pre` run immediately before the action. - `{action}_post` run immediately after the action. - `{action}_cancel` runs if the action failed to run (could also be called `error` or `fail`). This follows what we already have for the `render` handler. I would accept a patch that implements these for save and load.

Douglas Lassance commented

2021-05-28 18:55:58 +02:00

Thanks for looking into this further and very excited about the perspective of these new handlers.

To add to your comments, [Git LFS ]] is Git's solution to match Perforce's workflow benefits. The project is getting quite a lot of traction in the game industry and it does more than handling binaries well, with the addition of exclusive [ https:*github.com/git-lfs/git-lfs/wiki/File-Locking | locks on files resulting in restricted writing permission on the local files.

Concerning the explicit unlocking of files, software like Unreal add additional dialogs when trying to save a file. Users will be prompted to "check out" the file before it's saved. Alternatively, it will inform them that a file cannot be saved because someone else has it checked out.

The new save_init handler should allow implementing these behaviors with the desired amount of prompting.

Thanks for looking into this further and very excited about the perspective of these new handlers. To add to your comments, [Git LFS ]] is Git's solution to match Perforce's workflow benefits. The project is getting quite a lot of traction in the game industry and it does more than handling binaries well, with the addition of exclusive [[ https:*github.com/git-lfs/git-lfs/wiki/File-Locking | locks ](https:*github.com/git-lfs/git-lfs) on files resulting in restricted writing permission on the local files. Concerning the explicit unlocking of files, software like Unreal add additional dialogs when trying to save a file. Users will be prompted to "check out" the file before it's saved. Alternatively, it will inform them that a file cannot be saved because someone else has it checked out. The new `save_init` handler should allow implementing these behaviors with the desired amount of prompting.

Campbell Barton commented

2021-06-01 02:30:25 +02:00

Git LFS is a 3rd party program, not part of a typical GIT workflow. Nevertheless it's an example that uses locking.

Using handlers still seems like an awkward solution. As far as I can see it won't work when using "Save As" since Python doesn't have access to the target path in that case. Support for this can be added but as is this doesn't seem like it's a complete solution for the problem you're looking to solve.

Is this likely to be a problem? Or is this functionality not needed when using save-as?

Git LFS is a 3rd party program, not part of a typical GIT workflow. Nevertheless it's an example that uses locking. Using handlers still seems like an awkward solution. As far as I can see it won't work when using "Save As" since Python doesn't have access to the target path in that case. Support for this can be added but as is this doesn't seem like it's a complete solution for the problem you're looking to solve. Is this likely to be a problem? Or is this functionality not needed when using save-as?

Douglas Lassance commented

2021-06-01 05:48:52 +02:00

Save as a new file would not be an issue, but save as an existing file that is locked would be. I wish the handler was passed data such as previous_filename, new_filename as it would allow handling "save as "properly. That said, this happening more rarely, so one can probably live without it.

Save as a new file would not be an issue, but save as an existing file that is locked would be. I wish the handler was passed data such as `previous_filename`, `new_filename` as it would allow handling "save as "properly. That said, this happening more rarely, so one can probably live without it.

Campbell Barton commented

2021-06-01 07:53:45 +02:00

Note that Blender can save other kinds of files besides blend files (operators that export, saving images, saving a blend file may request saving modified images, Blender can also have external cache for physics and sculpting.

If it's a quick hack you doing your own system for convenience that's fine as a stop-gap, from a user perspective I think this is going to be unreliable.

Note that Blender can save other kinds of files besides blend files (operators that export, saving images, saving a blend file may request saving modified images, Blender can also have external cache for physics and sculpting. If it's a quick hack you doing your own system for convenience that's fine as a stop-gap, from a user perspective I think this is going to be unreliable.

Douglas Lassance commented

2021-06-01 09:11:35 +02:00

For exports such as Alembic or FBX, because Blender does not retain export options from one session to the next, the best solution is still to create custom wrapper operators. In turn, those can take care of any "unlocking" before the file is written. It is my understanding that save handlers are not called on export and that there are currently no export handlers. That could be a nice addition for a deeper integration of VCS, but ultimately it's outside of the scope of this original task (see [D6703 ](https://developer.blender.org/D6703)).

What I am trying to get to is that there are clear benefits (not having to worry about export settings) for users to use a custom one-click export button instead of going through File > Export. On the other hand, having to use a custom save button instead of going through the traditional Ctrl + S or File > Save is seen as an annoyance. That's where handlers come into play in my opinion.

So I would not say that I am trying to quick hack this, more so trying to integrate unlocking where it makes the most sense for my users.
I'd love to have better support for "save as" of course. In that regard, is it out of the question to pass data to the handlers? I guess that would compromise backward compatibility with any handler ever written.

For exports such as Alembic or FBX, because Blender does not retain export options from one session to the next, the best solution is still to create custom wrapper operators. In turn, those can take care of any "unlocking" before the file is written. It is my understanding that `save` handlers are not called on export and that there are currently no `export` handlers. That could be a nice addition for a deeper integration of VCS, but ultimately it's outside of the scope of this original task (see [[D6703](https://archive.blender.org/developer/D6703) ](https://developer.blender.org/D6703)). What I am trying to get to is that there are clear benefits (not having to worry about export settings) for users to use a custom one-click export button instead of going through **File > Export**. On the other hand, having to use a custom save button instead of going through the traditional **Ctrl + S** or **File > Save** is seen as an annoyance. That's where handlers come into play in my opinion. So I would not say that I am trying to quick hack this, more so trying to integrate unlocking where it makes the most sense for my users. I'd love to have better support for "save as" of course. In that regard, is it out of the question to pass data to the handlers? I guess that would compromise backward compatibility with any handler ever written.

Campbell Barton referenced a pull request that will close this issue

2023-02-15 07:13:15 +01:00

PyAPI: extend save/load handlers, optionally take a filepath argument #104769

Brecht Van Lommel referenced this issue

2023-02-15 07:48:35 +01:00

PyAPI: extend save/load handlers, optionally take a filepath argument #104769

Kathryn Long referenced this issue

2023-02-16 23:16:32 +01:00

PyAPI: extend save/load handlers, optionally take a filepath argument #104769

Campbell Barton referenced this issue

2023-02-17 01:34:41 +01:00

PyAPI: extend save/load handlers, optionally take a filepath argument #104769