Fixed a bug that incorrectly removed videos from a playlist when using the "Remove Viewed" dialog

[Jared234]

What is it?

• Bugfix (user facing)
• Feature (user facing)
• Codebase improvement (dev facing)
• Meta improvement to the project (dev facing)

Description of the changes in your PR

After a lot of troubleshooting, I found out that this bug is bigger than I initially thought, as I encountered another issue: If you manually remove videos from your playlist and then use the "Removed Watched Videos" feature, the manually deleted videos reappear.
The reason why these bugs occur is that the streamStateTable is not up-to-date when you remove the watched videos. However, updating the playlist/streamStateTable beforehand doesn't work either, as we only save the state of an entity if it has been viewed for more than 5 seconds (maybe you want to use this feature directly after starting to watch a video). I wouldn't recommend lowering this number, as it would cause accidentally clicked videos to be counted as viewed as well.
Therefore, I decided to slightly change the way we remove these videos. Instead of clearing the entire playlist and reinserting all the “not watched videos”, I decided to collect the “watched videos” and remove them from the playlist. This approach is IMO the best option.
This will be my first bug fix for NewPipe, so if there is anything I did wrong, please let me know and I will fix it.

Fixes the following issue(s)

• Fixes Playlist 'Remove Watched' bug #9122

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

• I read the contribution guidelines.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[Stypox]

Thank you!

I am not sure I understand: what's the difference between storing the watched items or the not-watched ones? Aren't the following statements always true?

• whole playlist ≡ non watched items ∪ watched items
• non watched items ∩ watched items ≡ ∅

Unless I am getting something wrong, I think it's better if you revert the parts of the PR regarding inverting watched and not-watched item lists.

[Jared234]

Thanks for the review.
IMO my implementation is more intuitive and I thought that it would also fix some other issues that existed in the "Remove Watched" feature. For example, try to follow these steps:

1. Go to Playlist
2. Remove a video in the playlist
3. Click on "Remove Watched" and then on "Ok".

This leads to the deleted video being re-added to the playlist, even though we just deleted it.

After further debugging, I found out today that we can also fix this by updating the playlist immediately after deleting the video. So, it would be possible to revert my changes and save the notWatchedItems instead of the watchedItems.
However, I still think my solution is more intuitive. Why would we want to delete all the videos and re-add the not watched ones when we can simply remove the watched ones from the current playlist?
However, I leave it up to you. Do you still think that I should undo my changes and implement your suggestion?

[Stypox]

This leads to the deleted video being re-added to the playlist, even though we just deleted it.

This problem is not caused by the method we use to store (non) watched items, but rather is a bug in the way the internal list is updated when an item is deleted. Inverting the solution would maybe solve this problem but may then fail on the opposite problem (i.e. adding manually a non-watched video to a playlist, then using "Remove watched" => suddenly the newly added video is not there anymore because the list from which to remove watchedVideos was not up-to-date and did not contain the newly added video).

However, I still think my solution is more intuitive. Why would we want to delete all the videos and re-add the not watched ones when we can simply remove the watched ones from the current playlist?

Because removing all videos and then readding the non-watched ones takes O(n) time, while removing each watched video from a list containing ~n videos takes O(n²) time. The latter is because each time removeItem is called it traverses the whole list to find the item to remove, and does so for each item you want to remove. Instead, removing all items takes O(n) (list is traversed only once) and then adding each item takes O(1), which for n items yields another O(n), so keeping track of the videos you want to keep is more efficient.

I also don't agree on the solution being more intuitive. The confusing thing in my opinion is the name of the list, i.e. nonWatchedItems. Maybe you could rename that to itemsToKeep.

Do you still think that I should undo my changes and implement your suggestion?

So yes, in my opinion you should undo that part of the changes, sorry for this ;-)

[Jared234]

Thanks for the detailed answer. I understand you a lot better now and will revert my changes.

If there isn't one already, should I just create a new issue regarding the faulty updating of the internal list, or should I try to fix it in this PR?

[Stypox]

I think it's better to create a separate PR for that, so opening an issue might be a good idea. If you then want to take care of it feel free to ;-)

[Stypox]

Works, thanks!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication