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.

Sign up for GitHub

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

Update playlist import to only add duplicate playlist items sometimes #5783

Merged
merged 5 commits into from
Oct 10, 2024
Merged

Update playlist import to only add duplicate playlist items sometimes #5783

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26684b4
$ Use early return within `#forEach`
PikachuEXE Oct 2, 2024
f3c6291
* Update playlist import to only add duplicate playlist items sometimes
PikachuEXE Oct 2, 2024
60a839f
Merge branch 'development' into feature/playlist-smart-merge
PikachuEXE Oct 10, 2024
baf4b45
~ Update comment
PikachuEXE Oct 10, 2024
edd3ea5
Update src/renderer/components/data-settings/data-settings.js
PikachuEXE Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 53 additions & 37 deletions src/renderer/components/data-settings/data-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,7 @@ export default defineComponent({
// to the app, so we'll only grab the data we need here.

const playlistObject = {}
const videoIdToBeAddedSet = new Set()

Object.keys(playlistData).forEach((key) => {
if ([requiredKeys, optionalKeys, ignoredKeys].every((ks) => !ks.includes(key))) {
Expand All @@ -888,6 +889,7 @@ export default defineComponent({

if (videoObjectHasAllRequiredKeys) {
videoArray.push(video)
videoIdToBeAddedSet.add(video.videoId)
}
})

Expand All @@ -901,48 +903,62 @@ export default defineComponent({
const playlistObjectKeys = Object.keys(playlistObject)
const playlistObjectHasAllRequiredKeys = requiredKeys.every((k) => playlistObjectKeys.includes(k))

if (playlistObjectHasAllRequiredKeys) {
const existingPlaylist = this.allPlaylists.find((playlist) => {
return playlist.playlistName === playlistObject.playlistName
})
if (!playlistObjectHasAllRequiredKeys) {
const message = this.$t('Settings.Data Settings.Playlist insufficient data', { playlist: playlistData.playlistName })
showToast(message)
return
}

if (existingPlaylist !== undefined) {
playlistObject.videos.forEach((video) => {
let videoExists = false
if (video.playlistItemId != null) {
// Find by `playlistItemId` if present
videoExists = existingPlaylist.videos.some((x) => {
// Allow duplicate (by videoId) videos to be added
return x.videoId === video.videoId && x.playlistItemId === video.playlistItemId
})
} else {
// Older playlist exports have no `playlistItemId` but have `timeAdded`
// Which might be duplicate for copied playlists with duplicate `videoId`
videoExists = existingPlaylist.videos.some((x) => {
// Allow duplicate (by videoId) videos to be added
return x.videoId === video.videoId && x.timeAdded === video.timeAdded
})
}
const existingPlaylist = this.allPlaylists.find((playlist) => {
return playlist.playlistName === playlistObject.playlistName
Copy link
Member

@absidue absidue Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

})

if (!videoExists) {
// Keep original `timeAdded` value
const payload = {
_id: existingPlaylist._id,
videoData: video,
}
if (existingPlaylist === undefined) {
this.addPlaylist(playlistObject)
return
}

this.addVideo(payload)
}
})
// Update playlist's `lastUpdatedAt`
this.updatePlaylist({ _id: existingPlaylist._id })
const duplicateVideoPresentInToBeAdded = playlistObject.videos.length > videoIdToBeAddedSet.size
const existingVideoIdSet = existingPlaylist.videos.reduce((video) => videoIdToBeAddedSet.add(video.videoId), new Set())
const duplicateVideoPresentInExistingPlaylist = existingPlaylist.videos.length > existingVideoIdSet.size
const shouldAddDuplicateVideos = duplicateVideoPresentInToBeAdded || duplicateVideoPresentInExistingPlaylist

playlistObject.videos.forEach((video) => {
let videoExists = false
if (shouldAddDuplicateVideos) {
if (video.playlistItemId != null) {
// Find by `playlistItemId` if present
videoExists = existingPlaylist.videos.some((x) => {
// Allow duplicate (by videoId) videos to be added
return x.videoId === video.videoId && x.playlistItemId === video.playlistItemId
})
} else {
// Older playlist exports have no `playlistItemId` but have `timeAdded`
// Which might be duplicate for copied playlists with duplicate `videoId`
videoExists = existingPlaylist.videos.some((x) => {
// Allow duplicate (by videoId) videos to be added
return x.videoId === video.videoId && x.timeAdded === video.timeAdded
})
}
} else {
this.addPlaylist(playlistObject)
videoExists = existingPlaylist.videos.some((x) => {
// Disallow duplicate (by videoId) videos to be added
return x.videoId === video.videoId
})
}
} else {
const message = this.$t('Settings.Data Settings.Playlist insufficient data', { playlist: playlistData.playlistName })
showToast(message)
}

if (!videoExists) {
// Keep original `timeAdded` value
const payload = {
_id: existingPlaylist._id,
videoData: video,
}

this.addVideo(payload)
}
})
// Update playlist's `lastUpdatedAt`
this.updatePlaylist({ _id: existingPlaylist._id })
})

showToast(this.$t('Settings.Data Settings.All playlists has been successfully imported'))
Expand Down