Skip to content

Drag and drop to upload samples#4345

Merged
walterbender merged 5 commits intosugarlabs:masterfrom
therealharshit:sampler/drab-and-drop
Feb 21, 2025
Merged

Drag and drop to upload samples#4345
walterbender merged 5 commits intosugarlabs:masterfrom
therealharshit:sampler/drab-and-drop

Conversation

@therealharshit
Copy link
Member

drag-and-drop_samples.mp4

@walterbender I have been working on the drag-and-drop functionality to upload samples, it's not complete but ready for testing.
Please test and let me know what do you think.

@therealharshit therealharshit marked this pull request as draft February 4, 2025 14:16
@haroon10725
Copy link
Contributor

@therealharshit We can already upload samples by clicking upload files button.

@therealharshit
Copy link
Member Author

@therealharshit We can already upload samples by clicking upload files button.

Yes @haroon10725 I am aware of that, it's an addition to that, with this functionality we can upload the samples just by dragging and droping it to sampler canvas.

@haroon10725
Copy link
Contributor

haroon10725 commented Feb 4, 2025

But how the user will know about it, that this feature exists.

@therealharshit
Copy link
Member Author

But how the user will know about it, that this feature exists.

I will update it in https://github.com/sugarlabs/musicblocks/blob/master/guide%2FREADME.md once it complete.

@haroon10725
Copy link
Contributor

I think there is no need for it. But still @walterbender and @pikurasa decide it.

@pikurasa
Copy link
Collaborator

pikurasa commented Feb 4, 2025

I haven't tested it yet, but I think it's fine to have this additional functionality.

It's not a game changer, but it does add a level of convenience.

I agree with @haroon10725 that we should consider how to make it discoverable. That said, in general, we have a few drag and drop options that are not obvious from the UI, such as MB HTML files and MIDI files.

I suggest that, in addition to adding to the README, we add a new section in help that points out what filetypes the user can expect to be able to drag and drop into Music Blocks.

@therealharshit therealharshit marked this pull request as ready for review February 4, 2025 18:03
@therealharshit
Copy link
Member Author

@pikurasa can you please tell my why we have to restrict the sample size to be less than 1MB ?
I tested it can handle more than that.

@pikurasa
Copy link
Collaborator

pikurasa commented Feb 4, 2025

@pikurasa can you please tell my why we have to restrict the sample size to be less than 1MB ?
I tested it can handle more than that.

IIRC, it is a somewhat arbitrary number. We want to restrict to a certain size, because otherwise users may insert "samples" that are actually entire songs. It's not useful to have audio files of entire songs or long musical passages as "samples".

@therealharshit
Copy link
Member Author

IIRC, it is a somewhat arbitrary number. We want to restrict to a certain size, because otherwise users may insert "samples" that are actually entire songs. It's not useful to have audio files of entire songs or long musical passages as "samples".

That's a great point! thanks for the explanation.

@pikurasa
Copy link
Collaborator

pikurasa commented Feb 5, 2025

@therealharshit could you please put a few .wav samples here to help with testing? I'm not finding any .wav samples on my machine, only .mp3

@therealharshit
Copy link
Member Author

@pikurasa github doesn't allow me to upload .wav files, so here is the .zip folder for the samples it has two .wav samples to test.
sample.zip

@therealharshit
Copy link
Member Author

@walterbender @pikurasa I have done some refactoring with the code, please test when you got a chance.

@walterbender
Copy link
Member

@pikurasa

@pikurasa
Copy link
Collaborator

@walterbender @pikurasa I have done some refactoring with the code, please test when you got a chance.

It works.

I'm somewhat wondering if we should allow the user to drag and drop a sample whether or not they're in the Sampler widget. For example, a user could drop a sample into MB, and that would generate the Sampler blocks' macros and start the widget, with the sample open.

@walterbender
Copy link
Member

I do like the idea of opening the sample widget if a sound file is drag and dropped. But let's do that in a separate PR.

@walterbender
Copy link
Member

Hmm. Not working for me.
Screenshot From 2025-02-14 16-33-03

Here is the WAV file I was using...
piano.zip

@therealharshit
Copy link
Member Author

therealharshit commented Feb 14, 2025

Hmm. Not working for me.

@walterbender I tested with your sample it's working fine on my machine.

sample-piano.mp4

The error on your says "upload failed" maybe you are dropping a wrong file, can you please test it again.

@therealharshit
Copy link
Member Author

I do like the idea of opening the sample widget if a sound file is drag and dropped. But let's do that in a separate PR.

I also like that idea, I'll work on it.

@pikurasa
Copy link
Collaborator

Here is the WAV file I was using...
piano.zip

I tested that .wav file, and it worked for me. (On HEAD is now at f547b10b0 Refactor: Remove redundant code)

@pikurasa
Copy link
Collaborator

pikurasa commented Feb 18, 2025

@walterbender please test once more and do a code review (or delegate a code review).

@pikurasa
Copy link
Collaborator

@haroon10725 perhaps you could do an initial code review of this PR for us?

@haroon10725
Copy link
Contributor

Sure @pikurasa I will do it.

@haroon10725
Copy link
Contributor

@walterbender @pikurasa I have done the code review.

@walterbender walterbender merged commit 0e5fc93 into sugarlabs:master Feb 21, 2025
4 checks passed
@haroon10725
Copy link
Contributor

haroon10725 commented Feb 21, 2025

@walterbender There was some formatting issues in the code and some code was extra. I mentioned in code review.

@walterbender
Copy link
Member

@haroon10725 I don't see your comments other than you said you did a review. I guess if there are changes you think we need to make, please make a new PR as this one is merged.

@therealharshit
Copy link
Member Author

@haroon10725 please tell what are the required changes.

@haroon10725
Copy link
Contributor

haroon10725 commented Feb 21, 2025

image
@walterbender I did code review but not sure why it didn't appear for others.
created a PR

@haroon10725
Copy link
Contributor

haroon10725 commented Feb 21, 2025

@walterbender My bad I didn't submitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants