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

EMSUSD-852 allow saving locked layers #3504

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

pierrebai-adsk
Copy link
Collaborator

Relevant change is the removal of the call to isReadOnly(). I refactored the function to avoid nested if and complex conditional that are hard to understand. Also commented on what each condition aims to do.

Relevant change is the removal of the call to isReadOnly().
@@ -267,13 +267,21 @@ bool LayerTreeItem::isIncoming() const { return _isIncomingLayer; }

bool LayerTreeItem::needsSaving() const
{
if (_layer) {
if (!isSessionLayer() && !isReadOnly()
Copy link
Collaborator Author

@pierrebai-adsk pierrebai-adsk Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the call to isReadOnly() is the only real change, all the other is just avoiding the complex nesting of conditionals and multi-line conditions, allowing to document what each condition does.

Copy link
Collaborator

@samuelliu-adsk samuelliu-adsk Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand it correctly, the needsSaving() function will return true even if permissionToSave is false, and that flag will be checked later when actually writing the layer

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO, isReadOnly checks permission to edit, not save. We currently do not have check for permission to save, but then again, USD itself will refuse to save, so we don't need to check it to prevent it. When we do the future work to have a UI for permission to save, we might had new verifications.

@pierrebai-adsk pierrebai-adsk added the adsk Related to Autodesk plugin label Dec 8, 2023
@pierrebai-adsk pierrebai-adsk requested review from luceric-adsk and samuelliu-adsk and removed request for luceric-adsk and samuelliu-adsk December 8, 2023 14:27
@pierrebai-adsk
Copy link
Collaborator Author

I found that I can fix EMSUSD-850 at the same time, so I will do so. The fix is related to this one.

The double-click handle was only checking if the layer was anonymous, instead of all teh case where it needed to be saved or was prevented to be saved.
@pierrebai-adsk
Copy link
Collaborator Author

The only PF failures were unrelated material VP2 test screenshot differing. Running those test locally succeeded.

@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Dec 11, 2023
@seando-adsk seando-adsk merged commit d01378b into dev Dec 11, 2023
10 of 11 checks passed
@seando-adsk seando-adsk deleted the bailp/EMSUSD-852/save-locked-layer branch December 11, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adsk Related to Autodesk plugin ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants