Fix disable delete layer button when only one layer exists #4070
Conversation
|
|
||
| const ContextReader reader(context); | ||
| const Sprite* sprite(reader.sprite()); | ||
| return sprite && sprite->totalFrames() > 1; |
There was a problem hiding this comment.
Hi @The-Vector, it looks like you are asking for the number of frames here but it should be the number of layers at the root level.
There was a problem hiding this comment.
Ah, I don't know how I missed that.
It should be fixed now.
|
|
||
| const ContextReader reader(context); | ||
| const Sprite* sprite(reader.sprite()); | ||
| return sprite && sprite->allLayersCount() > 1; |
There was a problem hiding this comment.
Hi @The-Vector, actually here you are counting the total number of layers (including layers in subgroups) which I think it's not the expected behavior, e.g. if you have a group with one child, that is allLayersCount=2 and enables the delete command for the group (which is the only layer at the root level).
You have to count the number of layers in the root (sprite->root()).
Also I'm seeing that the commit has 0quokka0@gmail.com as email but the CLA was signed with other email address, you should sign the CLA with the same name and email that you will use in the commit message.
There was a problem hiding this comment.
@The-Vector was you able to see the above comments?
There was a problem hiding this comment.
Yep, I filled out the CLA with the correct email, and I'm just testing my code so there's no other edge cases that I missed.
|
I believe this should work now. |
| if (!context->checkFlags(ContextFlags::ActiveDocumentIsWritable | | ||
| ContextFlags::HasActiveLayer)) | ||
| return false; |
There was a problem hiding this comment.
You can keep the same flags here:
| if (!context->checkFlags(ContextFlags::ActiveDocumentIsWritable | | |
| ContextFlags::HasActiveLayer)) | |
| return false; | |
| if (!context->checkFlags(ContextFlags::ActiveDocumentIsWritable | | |
| ContextFlags::HasActiveSprite | | |
| ContextFlags::HasActiveLayer)) | |
| return false; |
| return sprite && !((sprite->root()->layersCount() == 1) && (layer->parent() == sprite->root())); | ||
|
|
There was a problem hiding this comment.
This expression is confusing and I think we can simplify it checking for the true cases (intead of the inverse):
| return sprite && !((sprite->root()->layersCount() == 1) && (layer->parent() == sprite->root())); | |
| return sprite && layer && | |
| // We can remove all layers from non-root groups | |
| ((layer->parent() != sprite->root()) || | |
| // Check that we are not removing the last layer in the sprite | |
| (sprite->root()->layersCount() > 1)); |
But I saw that you wrote it originally in this way in 56726d2 and then reverted with 5b02460. Logically they do the same (by De Morgan's law). Can you explain why did you change this? Did this last commit really fix what it says it fix? (because it should work exactly the same as the previous one).
There was a problem hiding this comment.
I believe I changed it when testing my code and I saw that checking the true cases was still allowing you to delete a group if it was the root, but I checked just now and the code is identical and works the same, so I must have done something wrong in testing, sorry.
I'll add your suggestions and push the code with comments.
Sorry again for not being as thorough with testing my code.
|
Merged squashed in ea35725, thanks @The-Vector 👍 |
Fixes #3649
I agree that my contributions are licensed under the Individual Contributor License Agreement V4.0 ("CLA") as stated in https://github.com/igarastudio/cla/blob/main/cla.md
I have signed the CLA following the steps given in https://github.com/igarastudio/cla#signing