Skip to content
This repository has been archived by the owner on Feb 4, 2023. It is now read-only.

[Needs improvement] Playlist dialog: Add checkmark to indicate if song is in playlist #432

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

Sogolumbo
Copy link

@Sogolumbo Sogolumbo commented Feb 12, 2018

The "Add to playlist" dialog will show a checkmark behind a playlist if the song is already part of that playlist.
This feature will only be shown if only one song is selected for addition.

In the following screenshot the song is already part of the "Example playlist":
phonograph add to playlist dialog checkmark

The "Add to playlist" dialog will show checkmark behind a playlist if the song is already part of that playlist.
This feature will only be shown if the playlist dialog is opened and only one song is selected.
@kabouzeid
Copy link
Owner

I think this won't perform well on larger playlists, since it's doing an new SQL query for every single item.

@Sogolumbo
Copy link
Author

@kabouzeid Yes, there might be a problem with very big playlists. But I just tested it on my Android device. My biggest playlist contains all of my 1663 tracks and I can't see any difference between the normal app and my fork version.

@Sogolumbo
Copy link
Author

@kabouzeid Do you plan to include this feature? I would love to hear that because think there is no reason not to. The delay may be a drawback. But if the smartphone needs noticeable time to check whether a song is already in a playlist, imagine how long a human would need...
Do you know how big the most playlists of the users are? If they aren't much bigger than 2000 songs I don't think that there will be a noticeable delay.
If you still have reservations about this feature I would suggest adding the possibility to switch it on and off in the options. This feature is a key functionality of any music player which will save the users hours of their time (if they want to have playlist without duplicates).

@arkon
Copy link
Collaborator

arkon commented Feb 20, 2018

An alternative solution would be to allow the user to prune a playlist of duplicates, so it would just be a query within a playlist to check for duplicate IDs.

@kabouzeid
Copy link
Owner

@Sogolumbo Sorry for the delay, I have very little time for this project currently. Also I don't have any large playlist to test against. Could you measure the time in millis on your 1663 playlist how long the for loop actually takes?

@kabouzeid
Copy link
Owner

kabouzeid commented Feb 20, 2018

Instead of displaying a checkmark I would prefer to popup a dialog, in case the playlist already contains at least one song and allow to exclude those songs from adding again to the playlist.

@kabouzeid
Copy link
Owner

@arkon this could be added either ways

@Sogolumbo
Copy link
Author

I just tested how long the for loop takes. Here are the results:
The loop which checks whether the song is already in the playlists takes 91ms ±10ms.

Could you measure the time in millis on your 1663 playlist how long the for loop actually takes?

@kabouzeid What do you mean with "on your 1663 playlist"?
The checkmarks only get shown if i try to add exactly one song to a playlist.

In the test environment (my phone) there are 7 playlists. The longest one contains 1663 songs. The other playlists are smaller (180, 3, 253, 188, 22, 91 songs) (all playlists added up: 2400 songs). Device: honor 6C

By adding a breakpoint the time needed for checking the playlists for the song can be accessed.
@Sogolumbo
Copy link
Author

Sogolumbo commented Feb 20, 2018

Instead of displaying a checkmark I would prefer to popup a dialog, in case the playlist already contains at least one song and allow to exclude those songs from adding again to the playlist.

@kabouzeid I think that is the right solution if the user tries to add multiple songs / a playlist / an album to other playlists. For just one song I prefer my solution because it creates a faster workflow (for the user).
Also the App is missing a possibility to check whether a song is already contained in a playlist. (And the playlists are not ordered alphabetically)

@kabouzeid
Copy link
Owner

@Sogolumbo I apologize, I just notice that I've completely misunderstood your code before. Somehow I thought you would make an SQL query for every song in the playlist. Obviously this is not the case at all. Now I'm even more surprised that this takes 90ms.
I don't think the workflow should be different when you add one or many songs to a playlist. This just leads to confusion.

@kabouzeid
Copy link
Owner

Also if we display the checkmark additionally when the user tries to add exactly one song, it should be a proper material one. In this case we have to add an image view to the items where we display the checkmark. https://material.io/icons/#ic_done

@touficbatache
Copy link

Yes a material design icon would look better! Wouldn't it be even better if the icon is on the right end? What do you think @kabouzeid?

@kabouzeid
Copy link
Owner

@touficbatache absolutely

@touficbatache
Copy link

@kabouzeid How can we add lyrics to music?

@GeoZac
Copy link
Contributor

GeoZac commented Feb 23, 2018

@touficbatache You could add lyrics to tags or add *.LRC file to the same folder as music file location.

@touficbatache
Copy link

touficbatache commented Feb 23, 2018 via email

Merge Updates from kabouzeid/master.
At the end of the playlist name a checkmark is displayed if all songs are in the playlist. If only some of the songs are in the playlist, the checkmark is in brackets.
When adding songs to a playlist duplicates will be filtered out.
@ghost
Copy link

ghost commented Jun 5, 2019

Any update or is the fork the best we're going to get?

@Sogolumbo
Copy link
Author

@dronewizard Well the authors of the Phonograph seem to care a lot about the design of the app. Unfortunately I have absolutely no Idea how the material design works and I don't have the time learn it.
I have fully implemented all the business logic I can. Now either we include this feature as-is or we need someone to take care of the design.

@ghost
Copy link

ghost commented Jun 6, 2019

@dronewizard Well the authors of the Phonograph seem to care a lot about the design of the app. Unfortunately I have absolutely no Idea how the material design works and I don't have the time learn it.
I have fully implemented all the business logic I can. Now either we include this feature as-is or we need someone to take care of the design.

I'll look into it.

@kabouzeid
Copy link
Owner

No ascii checkmark please. The checkmark should be an image (ic_done) and right aligned.

@tralph3
Copy link

tralph3 commented Jun 6, 2019

Can I highjack the thread for a sec? How do you test the app? I want to make some changes for a PR but I don't know how to make a reasonable testing environment. You know, so I can change some code, and check out how it works on the fly.

Changed and added functions in PlaylistUtil to improve the efficiency of the AddToPlaylistDialog. It will only be checked once which song is in which playlist. This information is also used to make sure no songs will be added twice to the same playlist (instead of checking again).
doPlaylistContainsAllSongs returned true if the playlist was empty. That's a wrong result, hence fixed with this commit.
First Prototype:
Working check box menu.
Only two state-check-boxes (All or nothing)
Removed "Add Playlist" from the checkbox area and transformed it into a button.
The cancel button is removed as it takes up a lot of space and is not strictly necessary. (The same action can be performed by tapping next to the dialog.)
@Sogolumbo Sogolumbo changed the title Playlist dialog: Add checkmark to indicate if song is in playlist [Needs improvement] Playlist dialog: Add checkmark to indicate if song is in playlist Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants