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-1149 prevent unlocking system-locked layer when unlocking parents #3706

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

pierrebai-adsk
Copy link
Collaborator

  • When unlocking recursively, do not unlock sub-layers that are system-locked.
  • Correctly check that the request was for system lock when undoing a lock command.
  • Add more lock tests to cover more cases, like having sub-layers and system-locked layers.

- When unlocking recursively, do not unlock sub-layers that are system-locked.
- Correctly check that the *request* was for system lock when undoing a lock command.
- Add more lock tests to cover more cases, like having sub-layers and system-locked layers.
@pierrebai-adsk pierrebai-adsk self-assigned this Apr 9, 2024
@pierrebai-adsk pierrebai-adsk added bug Something isn't working adsk Related to Autodesk plugin labels Apr 9, 2024
Copy link
Collaborator

@AramAzhari-adsk AramAzhari-adsk left a comment

Choose a reason for hiding this comment

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

The design for the locklayer command is to allow locking / systemlocking / unlocking any layer and its sublayers through script, but not through UI.

I believe this will prevent the scenario of going through the script.

@pierrebai-adsk
Copy link
Collaborator Author

Well, if we all recursive unlocking of system-locked layers in the command, then the recursive option will not be useful because we will need to do the recursion in the UI code and skip the system-locked layer. The UI will not be able to use the recursive flag.

@AramAzhari-adsk
Copy link
Collaborator

Well, if we all recursive unlocking of system-locked layers in the command, then the recursive option will not be useful because we will need to do the recursion in the UI code and skip the system-locked layer. The UI will not be able to use the recursive flag.

We may either have to add a new parameter to mark the command to be run as a super user or a way to pass the information through UI to know it's being run through there. Any other suggestions are also welcome!

@pierrebai-adsk
Copy link
Collaborator Author

pierrebai-adsk commented Apr 11, 2024

No, that starts to be byzantine. The Maya command hook will do the layer recursion. Maaaybe there could be a flag to skip system-locked layers? That seems reasonable.

@AramAzhari-adsk
Copy link
Collaborator

No, that starts to be byzantine. The Maya command hook will do the layer recursion. Maaaybe there could be a flag to skip system-locked layers? That seems reasonable.

Either that or a generic way to indicate that overrides are allowed, but that creates the concept of user roles.
Even something as simple as a force parameter.

…system-locked layers

- Add a flag to control the lock command affecting sub-layers that are system-locked.
- Use teh flag from the UI, in the Maya command hook.
- Update the unit test.
@pierrebai-adsk
Copy link
Collaborator Author

Added -skipSystemLocked flag. Seems explicit enough to make its behavior clear.

@@ -868,7 +883,7 @@ class LockLayer : public BaseCmd
// Execute lock commands
for (size_t layerIndex = 0; layerIndex < _layers.size(); layerIndex++) {
// Note: the undo of system-locked is unlocked by design.
if (_previousStates[layerIndex] == MayaUsd::LayerLockType::LayerLock_SystemLocked) {
if (_lockType == MayaUsd::LayerLockType::LayerLock_SystemLocked) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this should also be conditioned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand what you mean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The previous state will always have the correct value when undoing, so there is no need to protect, at worst it will set the same state it already was.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as any consecutive number of system-locks results in Unlock, then it's all good.

@@ -208,8 +208,13 @@ void MayaCommandHook::lockLayer(
MayaUsd::LayerLockType lockState,
bool includeSubLayers)
{
// Per design, we refuse to change the lock state of system-locked
// layers through the UI.
if (MayaUsd::isLayerSystemLocked(usdLayer))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also needs to be conditioned whether we skip system locked files or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again, I don't understand what you mean. We never want to affect system-locked layer from thr UI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it guaranteed that the mayaCommandHook is only called from the UI?

If so, then it's all good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I would suggest though, is to add the skipSystemLocked. Because unlike Muting, sub-layers of a system-locked layer can be unlocked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, so we should et it do its work if include sub-layers is true? OK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually the code behind the lock layer and sublayer menu is

if (!$isSessionLayer && $hasSublayers) {
if ($isLocked)
$label = getMayaUsdString("kMenuUnlockLayerAndSublayers");
else
$label = getMayaUsdString("kMenuLockLayerAndSublayers");
$cmd = makeCommand($panelName, "lockLayerAndSubLayers");
$enabled = !$isSystemLocked;
menuItem -label $label -enable $enabled -c $cmd;
}

which seems to be enabled only if the right-clicked layer isn't system locked.

Copy link
Collaborator

@AramAzhari-adsk AramAzhari-adsk left a comment

Choose a reason for hiding this comment

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

MayaCommandHook::lockLayer should allow unlocking sub-layers of a system-locked layer since the system-lock is not inheritance-based such as Muting layers.

Copy link
Collaborator

@AramAzhari-adsk AramAzhari-adsk left a comment

Choose a reason for hiding this comment

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

Actually, if the menu item is disabled, then there's no need to apply to sub-layers.
The sub-layers can be unlocked through the selection of those layers.

All good.

@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Apr 11, 2024
@seando-adsk seando-adsk added workflows Related to in-context workflows and removed bug Something isn't working adsk Related to Autodesk plugin labels Apr 11, 2024
@seando-adsk seando-adsk merged commit 7f34c62 into dev Apr 11, 2024
11 checks passed
@seando-adsk seando-adsk deleted the bailp/EMSUSD-1149/system-lock-vs-unlock branch April 11, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants