search

sugarlabs / musicblocks

Music Blocks -- A musical microworld
https://musicblocks.sugarlabs.org/
GNU Affero General Public License v3.0
539 stars 726 forks source link

Add bulk delete feature #3885

Closed apsinghdev closed 1 month ago

apsinghdev commented 1 month ago

fixes #3840

This PR adds a bulk delete feature. It lets a user drag to select blocks and delete them in a single click.

Here is the demo of this

https://github.com/sugarlabs/musicblocks/assets/109718740/c228aa13-e43d-41d7-b0dc-687491702c37

apsinghdev commented 1 month ago

@walterbender please take a look and suggest modifications if any.

walterbender commented 1 month ago

In general, it seems to work well. However, there is one critical bug (apparent in my testing and in your video). If you delete a Note Block, it seems to be creating a Silence Block. I suspect that this is due to the order in which the blocks are being deleted. If you first delete the Pitch Block inside of a Note Block through the normal delete mechanism, it will create a Silence Block. That Silence Block is not on the bulk-delete list. So I think you may need to delete from the outside-in rather than in either an arbitrary order or inside out.

One small issue with coding style: please be consistent with the use of spaces around operators.

Also, it seems that restore will restore one block at a time, which I guess is OK for now. At some point, we may consider restoring the entire bulk delete at once. But I would save that for a different MR.

apsinghdev commented 1 month ago

@walterbender sir Thanks for the review. I've got your point about getting the silence block after deleting the blocks. I will fix this bug as well as the coding style soon. Also, after completing this issue, I'll go ahead and implement Bulk-Restore as well.

apsinghdev commented 1 month ago

@walterbender can you please elaborate on this line "you may need to delete from the outside-in rather than in either an arbitrary order or inside out." a bit more?

I did some digging in the codebase and found the currently used delete mechanism to delete blocks. are there other better methods available in the codebase to delete the blocks?

walterbender commented 1 month ago

I mean delete the parent before the children in the context of a clamp-style block.

On Sun, May 12, 2024, 1:08 PM Ajeet Pratap Singh @.***> wrote:

@walterbender https://github.com/walterbender can you please elaborate this line "you may need to delete from the outside-in rather than in either an arbitrary order or inside out." a bit more?

— Reply to this email directly, view it on GitHub https://github.com/sugarlabs/musicblocks/pull/3885#issuecomment-2106316850, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6PXYPDTTBKTYF733QR4HTZB6OZNAVCNFSM6AAAAABHHAPVH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWGMYTMOBVGA . You are receiving this because you were mentioned.Message ID: @.***>

apsinghdev commented 1 month ago

@walterbender I have made the changes and the feature seems working fine. Please take a look.

https://github.com/sugarlabs/musicblocks/assets/109718740/6f71940a-259b-4253-a99b-7ea13fa15bb0