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

Add ability to add to queue #471

Merged
merged 8 commits into from
Jun 17, 2024
Merged

Add ability to add to queue #471

merged 8 commits into from
Jun 17, 2024

Conversation

SO9010
Copy link
Contributor

@SO9010 SO9010 commented May 7, 2024

This adds the ability to right-click on a song and add it to play next in the queue. Please do leave me feedback and critiques as this is my first time programming in rust!

Solves #431 !

@jacksongoode
Copy link
Collaborator

This looks good but it sure would be nice to see the queue as a view. I'm wondering how difficult it would be to create a list page for a view of this queue? I could see us merging this feature before that, but it would feel incomplete... @Insprill what do you think?

@SO9010
Copy link
Contributor Author

SO9010 commented May 10, 2024

I just realised that the queue isn't held when switching to another playlist or clicking on another song to play now so I will quickly fix that.

@SO9010
Copy link
Contributor Author

SO9010 commented May 10, 2024

It will take me a decent amount of time to get it so it can keep the queued song so it can be played next in a playlist the song isn't in.

@SO9010
Copy link
Contributor Author

SO9010 commented May 12, 2024

Okay, so I have added some updates to my code, the added queue is added to a new vector which means it can be held in place, additionally it means that I used the push_back method meaning that it doesn't make it play next, rather more like the official client it adds at the end of a user created queue... I hope that makes sense, but it is a lot better now.

@jacksongoode
Copy link
Collaborator

Really excited to have a look at this today @SO9010 !

@jacksongoode jacksongoode requested a review from Insprill May 15, 2024 17:58
@jacksongoode
Copy link
Collaborator

It seems like queuing, switching contexts (to a different playlist or album) and then adding again to the queue causes past queued items to disappear. Is this not the case in your behavior?

@SO9010
Copy link
Contributor Author

SO9010 commented May 31, 2024

Hello, sorry this was an oversight! I assumed that the user would want to have the queue reset when they change the playlist. But that would be counter-intuitive. I have fixed this issue.

I'm sorry if this is very time-consuming for you, I'm still pretty new to open-source contribution.

@jacksongoode
Copy link
Collaborator

I just tested this out and it feels right to me! Looking over the code now!

Copy link
Collaborator

@jacksongoode jacksongoode left a comment

Choose a reason for hiding this comment

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

Looks good for the most part! Just some linting cleanup that needs to happen (use the standard linter) and just some clarification on the arch decisions.

psst-core/src/player/mod.rs Outdated Show resolved Hide resolved
psst-core/src/player/queue.rs Outdated Show resolved Hide resolved
psst-core/src/player/queue.rs Outdated Show resolved Hide resolved
psst-gui/src/controller/playback.rs Outdated Show resolved Hide resolved
psst-gui/src/data/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@jacksongoode jacksongoode left a comment

Choose a reason for hiding this comment

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

Alright, looks good!

@jacksongoode jacksongoode merged commit f9a54a6 into jpochyla:master Jun 17, 2024
4 of 6 checks passed
@SO9010
Copy link
Contributor Author

SO9010 commented Jun 17, 2024

Thank you so much!!!!

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.

2 participants