Update playlist import to only add duplicate playlist items sometimes #5783
Conversation
Either when existing playlist or incoming playlist has duplicate items
github-actions
bot
added
the
PR: waiting for review
There was a problem hiding this comment.
(A) Both existing & incoming playlists have unique videos only
Expected behavior: Not seeing any duplicates (This is the testcase from the issue)
(B) Only incoming playlist has "duplicate" videos
Expected behavior: You will see the duplicate because PC2 got it in its db
(C) Only existing playlist has "duplicate" videos
Expected behavior: You will see the duplicate because PC2 got it in its db
(D) Only existing playlist has "duplicate" videos
Expected behavior: You will see the duplicate because PC2 got it in its db
LGTM
ChunkyProgrammer
requested review from
absidue,
ChunkyProgrammer and
kommunarr
| }) | ||
| } | ||
| const existingPlaylist = this.allPlaylists.find((playlist) => { | ||
| return playlist.playlistName === playlistObject.playlistName |
There was a problem hiding this comment.
It probably makes sense to try to match by _id first, because if you rename the playlist on one computer you still want it to be the same when you import it on the other computer instead of being treated as a completely separate playlist.
That being said I've just realised that the _id is considered an ignored key, so there is no way that a renamed playlist could ever match, because the import will make sure that the _id won't match, but that problem already existed before this pull request. Also interesting that we don't save the lastPlayedAt field when it's a new playlist.
There was a problem hiding this comment.
Agree but no way this belongs to this PR (plus need test cases)
I can make another PR to make it find by _id first later
lastPlayedAt not saved for new/existing playlist intentionally
Obvious for existing playlist but for new playlist it's never played locally and I consider it as a "local property" (e.g. importing other people's playlist)
There was a problem hiding this comment.
I have to say I personally never use the playlists, but the way I use the other export and import options is for backup purposes, which means I only ever import my own stuff in my FreeTube install (dev mode is an exception because there I only use it to test stuff and that data is saved in a separate location), I would expect everything possible to be imported.
Importing someone else's files is probably a pretty rare use case, that is only really becoming possible because of the open PR to export single playlists.
| // Find by `playlistItemId` if present | ||
| videoExists = existingPlaylist.videos.some((x) => { | ||
| // Allow duplicate (by videoId) videos to be added | ||
| return x.videoId === video.videoId | ||
| }) |
There was a problem hiding this comment.
Could you please update this comment to match what the code does.
* development: (59 commits) Translated using Weblate (Belarusian) Translated using Weblate (Belarusian) Translated using Weblate (Belarusian) Enable Belarusian locale (FreeTubeApp#5841) Translated using Weblate (Russian) Translated using Weblate (Japanese) fix cmd+m shortcut to minimize the window on macos and not mute the video (FreeTubeApp#5828) (FreeTubeApp#5829) Update dependabot.yml to add new eslint packages into the eslint group (FreeTubeApp#5837) Bump the babel group with 4 updates (FreeTubeApp#5831) Bump globals from 15.9.0 to 15.10.0 (FreeTubeApp#5835) Bump lefthook from 1.7.17 to 1.7.18 (FreeTubeApp#5836) Bump the eslint group with 2 updates (FreeTubeApp#5832) Bump @eslint/js from 9.11.1 to 9.12.0 (FreeTubeApp#5833) Bump @eslint/compat from 1.1.1 to 1.2.0 (FreeTubeApp#5834) Translated using Weblate (Belarusian) Translated using Weblate (Hebrew) Translated using Weblate (Chinese (Traditional Han script)) Turn `vue/no-useless-template-attributes` eslint rule back on (FreeTubeApp#5830) Translated using Weblate (Serbian) Translated using Weblate (Belarusian) ...
|
I think you may have missed what I was trying to say with please update the comment, as you updated a different one. The one I originally commented on says that it is searching by playlist item ID, but then the code right below it is searching by video ID... |
github-actions
bot
removed
the
PR: waiting for review
Pull Request Type
Related issue
closes #5038
Description
Updates the behaviour of importing playlists when existing playlist found
Before:
playlistItemIdAfter:
videoIdvalues)Only add playlist items from incoming playlist with
videoIdabsent in existing playlistplaylistItemId(old behaviour)Screenshots
Testing
Favoritescoz that's the name of playlist in those files(A) Both existing & incoming playlists have unique videos only
(B) Only incoming playlist has "duplicate" videos
(C) Only existing playlist has "duplicate" videos
(D) Only existing playlist has "duplicate" videos
playlistItemId)Desktop
Additional context