Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

playlist-first playback flow + autoqueue files in directory #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Firepal
Copy link

@Firepal Firepal commented Sep 4, 2023

  • playbackInfo is now playlist-centric, filepath field is replaced by an index into the playlist. inspired by VLC's behavior
  • when changing file, slurp up all possibly-compatible files via extension (will change this to use GetFileType later)
  • "playlist file" code to be tentatively removed (m3u files do the same stuff, right?)

fixes #22

Copy link
Owner

@oreo639 oreo639 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

will change this to use GetFileType later

Just using the extension should be fine, although it probably should ensure that the filename selected is always added to the playback queue and is the initial file selected.

"playlist file" code to be tentatively removed (m3u files do the same stuff, right?)

The json playlist file stuff was more of a debugging thing for netradio before m3u and pls was supported (with ParseNC() being parse netradio config iirc). That said, I don't see the need to comment it out as the fix is pretty trivial.

Displaying the full filepath isn't great, as only the file name is important (especially since I don't have text scrolling atm), which is what playbackInfo->filename was for. Not sure if you have something different in mind, I'll have to think about how best to handle it later.

Overall, good work.

src/gui/menus/PlayerMenu.hpp Outdated Show resolved Hide resolved
src/gui/menus/PlayerMenu.cpp Outdated Show resolved Hide resolved
src/gui/menus/PlayerMenu.cpp Outdated Show resolved Hide resolved
src/gui/menus/PlayerMenu.cpp Outdated Show resolved Hide resolved
src/player.hpp Outdated Show resolved Hide resolved
src/player.cpp Outdated Show resolved Hide resolved
src/player.cpp Outdated Show resolved Hide resolved
@Firepal
Copy link
Author

Firepal commented Sep 6, 2023

Just using the extension should be fine

Well I suppose we keep the extension code then, but I would like some clarification on the extra formats "Xmi, Mus, Hmi, Hmp" listed in the readme.

MIDI formats, okay!

Displaying the full filepath isn't great, as only the file name is important

Already planned to restore this after making sure my playlist shotgun-implementation worked. I preserved the filepath field as filename for the purpose of display

it probably should ensure that the filename selected is always added to the playback queue and is the initial file selected

Planned too

Thanks for your review!

@Firepal
Copy link
Author

Firepal commented Sep 6, 2023

The json playlist file stuff was more of a debugging thing for netradio before m3u and pls was supported (with ParseNC() being parse netradio config iirc). That said, I don't see the need to comment it out as the fix is pretty trivial.

I personally can't wrap my head around how the playlist in settings_t is linked to the playlist in playbackInfo_t or how to get the former into the latter. I'm reinstating the playlist field in settings_t just so I don't break anything.

@Firepal Firepal force-pushed the playlist_basic branch 2 times, most recently from 33caa3d to eed9a39 Compare September 6, 2023 09:58
@Firepal Firepal marked this pull request as ready for review September 6, 2023 09:59
@Firepal
Copy link
Author

Firepal commented Sep 6, 2023

Alright. I think I'm happy with the current state of this PR.

@Firepal Firepal force-pushed the playlist_basic branch 3 times, most recently from 49c29ed to e15f6aa Compare September 6, 2023 10:08
// if we don't support the file, we won't go any further
// to prevent unnecessarily stopping the music
std::string extension = filepath.substr(filepath.find_last_of('.') + 1);
if (!is_extension_supported(extension))
Copy link
Owner

@oreo639 oreo639 Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be done for playlist files and m3u, pls, and json should not be in SUPPORTED_EXTS.

@oreo639
Copy link
Owner

oreo639 commented Sep 6, 2023

I personally can't wrap my head around how the playlist in settings_t is linked to the playlist in playbackInfo_t or how to get the former into the latter.

ParseNC is not related to settings settings_t.

The playlist field in settings_t was completely unused originally with the idea that it was something that I would implement later, I'll probably remove it entirely.

@Firepal
Copy link
Author

Firepal commented May 16, 2024

I'm not sure I understand what exactly the problem is with having the playlist extensions in the supported extension list.
By doing that it passes through to the rest of the code below...

@Firepal Firepal force-pushed the playlist_basic branch 3 times, most recently from e15f6aa to 1a7a6e6 Compare May 16, 2024 14:29
@Firepal
Copy link
Author

Firepal commented May 16, 2024

I think I got it

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.

[Feature Request] Read folder as playlist
2 participants