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

Jump to bottom

Undo Feature For Remove From Playlist. #5885

Open
wants to merge 9 commits into
base: development
Choose a base branch
from
Open

Undo Feature For Remove From Playlist. #5885

wants to merge 9 commits into from

Conversation

Soham456
Copy link

@Soham456 Soham456 commented Oct 17, 2024 •
edited by PikachuEXE
Loading

Pull Request Type

  • Feature Implementation

Related issue

closes #5421

Additional context

resolved yarn.lock conflict

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 17, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 17, 2024 09:43
@absidue absidue added the DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix label Oct 17, 2024
@absidue
Copy link
Member

absidue commented Oct 22, 2024

Could you please also undo the changes to the unrelated files? This time please do it in this pull request instead of closing and creating a duplicate one.

absidue
absidue reviewed Oct 22, 2024
View reviewed changes
src/renderer/components/FtElementList/FtElementList.vue Outdated
@@ -129,11 +129,10 @@ function moveVideoDown(videoId, playlistItemId) {
}

/**
* @param {string} videoId
* @param {string} playlistItemId
* @param {string} result
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {string} result
* @param {any} video

src/renderer/components/FtElementList/FtElementList.vue Outdated
Comment on lines 134 to 135
function removeFromPlaylist(result) {
emit('remove-from-playlist', result)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function removeFromPlaylist(result) {
emit('remove-from-playlist', result)
function removeFromPlaylist(video) {
emit('remove-from-playlist', video)

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix labels Oct 23, 2024
@absidue absidue added DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix and removed DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix labels Oct 23, 2024
auto-merge was automatically disabled October 27, 2024 15:00

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 27, 2024 15:00
@Soham456 Soham456 requested a review from absidue October 28, 2024 09:17
@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Nov 3, 2024
PikachuEXE
PikachuEXE requested changes Nov 3, 2024
View reviewed changes
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

We already have a file telling editor to use spaces instead of tabs for indent
https://github.com/FreeTubeApp/FreeTube/blob/development/.editorconfig
Please setup your editor (info at https://editorconfig.org) and fix those tabs

src/renderer/views/Playlist/Playlist.js Outdated
Comment on lines 600 to 616
this.removeVideo({
_id: this.playlistId,
videoId: videoData.videoId,
playlistItemId: videoData.playlistItemId,
})

// Update playlist's `lastUpdatedAt`
this.updatePlaylist({ _id: this.playlistId })
showToast(this.$t('User Playlists.SinglePlaylistView.Toast["Video has been removed. Click here to undo."]'), 3000, () => {
this.addVideoBackToPlaylist(videoData);
}
);
} catch (e) {
showToast(this.$t('User Playlists.SinglePlaylistView.Toast.There was a problem with removing this video'))
console.error(e)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

these
Also remove the last blank line before the end of method?

src/renderer/views/Playlist/Playlist.js Outdated
showToast(this.$t('User Playlists.SinglePlaylistView.Toast.There was a problem with removing this video'))
console.error(e)
showToast(this.$t('User Playlists.SinglePlaylistView.Toast.There was a problem with adding this video back to playlist'));
console.error(e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove unnecessary semicolons

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 3, 2024
auto-merge was automatically disabled November 16, 2024 11:47

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 16, 2024 11:48
auto-merge was automatically disabled November 16, 2024 12:21

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 16, 2024 12:21
PikachuEXE
PikachuEXE requested changes Nov 17, 2024
View reviewed changes
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

In src/renderer/views/Playlist/Playlist.vue there is another usage of removeVideoFromPlaylist that should be updated

auto-merge was automatically disabled November 17, 2024 12:47

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 17, 2024 12:48
efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc requested changes Nov 17, 2024
View reviewed changes
Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Undo should place the video in the same position it was in before. Now it behaves like you have deleted the video and manually added it again to the playlist.

VirtualBoxVM_W0FVtMIPuA.mp4

@Soham456
Copy link
Author

2024-11-24.13-55-47.1.1.mp4

can you please recheck? It works fine for every sort type, It happens only for custom filter where anyways videos can be rearranged.

@absidue
Copy link
Member

absidue commented Nov 24, 2024 •
edited
Loading

@Soham456 "Custom" is how the videos are actually stored in the playlist, that is the real order of the videos. So the problem happens regardless of the sort order, it is just that the other ones hide it from you. An "undo" feature should undo the change properly, as if the change had never even occurred, so an "undo" button that doesn't place the video back in its original place in the playlist is only doing half of its job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absidue absidue Awaiting requested review from absidue

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

@kommunarr kommunarr Awaiting requested review from kommunarr

@PikachuEXE PikachuEXE Awaiting requested review from PikachuEXE

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc Awaiting requested review from efb4f5ff-1298-471a-8973-3d47447115dc

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
PR: changes requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Undo for "Remove from Playlist"
5 participants
@Soham456 @absidue @PikachuEXE @efb4f5ff-1298-471a-8973-3d47447115dc @ChunkyProgrammer