search

nukeop / nuclear

Streaming music player that finds free music for you
https://nuclear.js.org/
GNU Affero General Public License v3.0
11.72k stars 1.02k forks source link

Shuffle visualization preset feature #1555 #1612

Open CarlosBor opened 1 month ago

CarlosBor commented 1 month ago

A couple matters:

Regarding the third point of the guidelines, "If you're fixing a bug, or adding a new functionality", this is a new functionality, but when checking the tests for the visualizerOverlay component there are only snapshots. I've updated them, but should I add tests on it and the randomizing function I'm using or is it understood to be simple enough to be ok as it is?

Also, as it is right now, the name of the preset is stored in the "settings" slice of the redux store, I've thus added the shuffle toggle to the same slice in a similar way, is that okay or should I change it to use a new "visualizer" slice?

Besides that, the button to toggle shuffle appears like so: image

And the shuffle happens if enabled when a track ends.

Besides that, awaiting for revision, thanks for your patience.

nukeop commented 1 month ago

Hi thanks for contributing this. I will do a review and comment in the files individually, but it would be great to have tests that reproduce the behavior of the user using this function, so something that mounts the visualizer, selects the shuffle mode, waits for the end of the current song to see if it's changed, etc. Basically everything you added in this PR.

nukeop commented 1 month ago

I looked at your code and I have no additional remarks. It's all great.

CarlosBor commented 2 weeks ago

Went with the pull request update rather than writing into Discord this time, should be tidier this way.

Long story short, I did manage to mock the library but I reckon that was the wrong way to go about things, I just can't get the handleFinishedPlaying() function from the SoundContainer component to fire. I'm guessing it ultimately triggers as a response to an event fired somewhere but I'm lost regarding how to simulate the behavior and frankly I have the suspicion that there is something obvious that I'm missing.

I'd be thankful If you could take a crack at it in case it's not too much work. Otherwise I could really do with some pointers.

Thanks for your patience.

nukeop commented 2 weeks ago

Hey, I'm working on fixing the Windows build first. I'll take a look as soon as I can. I'm sure we'll figure it out together somehow.

CarlosBor commented 2 weeks ago

Sure thing. I use Windows so if I can offer any assistance I'm up for it, don't know much about the matter though.

In the meantime I'll take a small gander at #1539, will comment in that thread if I start taking care of it proper.