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

move items in a playlist #224

Merged
merged 6 commits into from
Jul 30, 2023

Conversation

cobbinma
Copy link
Contributor

add track actions to move items up and down in the current playlist 🎵

Screenshot 2023-07-26 at 18 18 30

closes #197

add track actions to move items up and down in the current playlist

Closes: 197
@aome510
Copy link
Owner

aome510 commented Jul 28, 2023

Hi @cobbinma, thanks for the PR. I think the original issue wants to implement the feature as commands instead of actions. I can find it quite troublesome, let's say, to move an item up 5 times as you need to call GetAction... command, wait for the popup then select the action. With command, you can just press a key.

It shouldn't be too difficult to change to command.

use commands instead of track actions to move playlist items
@cobbinma
Copy link
Contributor Author

Hi @cobbinma, thanks for the PR. I think the original issue wants to implement the feature as commands instead of actions. I can find it quite troublesome, let's say, to move an item up 5 times as you need to call GetAction... command, wait for the popup then select the action. With command, you can just press a key.

It shouldn't be too difficult to change to command.

Thanks @aome510

I agree, that is much easier 👍

.find(|playlist| &playlist.id == playlist_id)
.is_some_and(|playlist| {
Some(&playlist.owner.1)
== data.user_data.user.as_ref().map(|user| &user.id)
Copy link
Owner

@aome510 aome510 Jul 29, 2023

Choose a reason for hiding this comment

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

This owner check can be moved inside the find function right.

Edit: then you can use .any instead of .find.is_some() to make the code cleaner

Copy link
Owner

@aome510 aome510 left a comment

Choose a reason for hiding this comment

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

Looking good. Only need a minor change

@aome510
Copy link
Owner

aome510 commented Jul 29, 2023

I just tried the feature but the ordering change does not persist after restart. It does change the ordering in the UI though, which I think mainly because we also update the cached. @cobbinma does it work for you?

@cobbinma
Copy link
Contributor Author

I just tried the feature but the ordering change does not persist after restart. It does change the ordering in the UI though, which I think mainly because we also update the cached. @cobbinma does it work for you?

@aome510 It's only persisting moving upwards... 😕

@aome510
Copy link
Owner

aome510 commented Jul 29, 2023

It's only persisting moving upwards... 😕

Oh right, also only persists moving upwards for me. Very interesting. Do you think you can investigate this behaviour? Otherwise, I think the PR is in a good state already.

@cobbinma
Copy link
Contributor Author

It's only persisting moving upwards... 😕

Oh right, also only persists moving upwards for me. Very interesting. Do you think you can investigate this behaviour? Otherwise, I think the PR is in a good state already.

@aome510

I misunderstood the difference between insert before and index when using update playlist items.

Should be fixed now 👍

@cobbinma cobbinma requested a review from aome510 July 30, 2023 10:15
partial eq is no longer needed on track action
@aome510 aome510 merged commit 0dbb01e into aome510:master Jul 30, 2023
3 checks passed
@aome510
Copy link
Owner

aome510 commented Jul 30, 2023

Thanks @cobbinma

@cobbinma cobbinma deleted the feat/197-move-playlist-items branch July 30, 2023 13:36
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.

shortcuts for moving items in a playlist?
2 participants