search

HookedBehemoth / sys-tune

Background audio player for the Nintendo switch
145 stars 10 forks source link

Bulk song queuing #32

Closed lucasdepetrisd closed 1 year ago

lucasdepetrisd commented 1 year ago

Hey there! This is a pull request from my forked repo. I included the new feature that allows adding songs in bulk to the playlist, as well as a variable to track the number of songs added. Also solved the previous confusion with the changes.

About the feature and the code, I think it'll help to add a limit of the songs that can be added to the playlist before merging branches as to avoid memory failure. Additionally, it seems to me that the click listener of X can be moved from scanCwd to the inputHandler.

If you have any questions or feedback, feel free to communicate. Thank you for considering my contribution!

HookedBehemoth commented 1 year ago

Thanks for the PR! I made some small changes to it. I don't think any of the added class fields were really necessary. The pointless lambda capture of FsDir pointed me to a clear handle leak that I resolved too.

I'm not too happy with the info message in the list but doing it properly - adding a toast or similar - would be too much effort for this.

lucasdepetrisd commented 1 year ago

Thanks for the changes! I tried to provide a response to the user's input using a draw string and renderer in a Custom Drawer, but it's better to leave it. Regarding the A button, implementing the alert function to give a response when adding individual songs would be cool.

HookedBehemoth commented 1 year ago

Added a poor man's toast and modify-able button descriptions img

I'll try to get this merged tomorrow. Too late now though.

HookedBehemoth commented 1 year ago

Couldn't merge this yet, as there is still an issue. Songs are added in folder order and should be added as they are displayed (alphabetically sorted rn). If you want to look into that, go ahead. I'll try on Wednesday.

lucasdepetrisd commented 1 year ago

Sure, I'll take a look. Also, I was thinking of adding a small UI fix to the Playlist menu to show the button for removing songs. Adding it in a second commit.

lucasdepetrisd commented 1 year ago

Individual toast

ee29430d4c3669cc2d8d99bd01bec76d6af5ea09 2023031315090300

Order in bulk-add and the Y button description

bcc26cf4ce85a77fe71525db6421b4e9c070000a and 366f7b69b9c74c4e696e48641a7adec6ac501c3f 2023031315091400

HookedBehemoth commented 1 year ago

This will leak memory on bulk add as you create tsl::elm::ListItem and never free them. In the old code, those elements were given to libtesla which eventually cleans them up.

You should probably not even use the ListItem's here at all. Just make a vector<char[FS_MAX_PATH] or vector and sort those.

HookedBehemoth commented 1 year ago

Also I don't see why the frame in PlaylistGui is now a field. We're not using it anywhere else.

lucasdepetrisd commented 1 year ago

This will leak memory on bulk add as you create tsl::elm::ListItem and never free them. In the old code, those elements were given to libtesla which eventually cleans them up.

You should probably not even use the ListItem's here at all. Just make a vector<char[FS_MAX_PATH] or vector and sort those.

I tried with a vector<char[FS_MAX_PATH]> and other methods of chars, but I found it a bit of a hassle to work with, so I turned to strings which are simpler and provides automatic memory management. If you have some time, please check it out, maybe you can work it out. Additionally, I think I'm confused, but shouldn't the std::vector<tsl::elm::ListItem *> folders, files; in scanCwd also cause memory leak?

Also I don't see why the frame in PlaylistGui is now a field. We're not using it anywhere else.

Sorry, I didn't notice the call to elm_overlayframe.hpp in gui_playlist.cpp so I tried to copy the one you implemented in browser. Already reverted.

HookedBehemoth commented 1 year ago

thanks