Fix inconsistency between user interaction and database commit order when re-adding videos to the playlist #8248
Conversation
|
Kudos, SonarCloud Quality Gate passed! |
| if (fragment instanceof LocalPlaylistFragment) { | ||
| ((LocalPlaylistFragment) fragment).commitChanges(); | ||
| } |
There was a problem hiding this comment.
What about the cases when the local playlist fragment is a main page tab?
There was a problem hiding this comment.
This case can also be handled by these lines.
I tested this case on my device. It works fine.
There was a problem hiding this comment.
Mmmh, I don't understand why those lines would handle it. If the fragment is a main page tab, then it is not the parent of the VideoDetailFragment anymore
There was a problem hiding this comment.
I have added a supplementary video for this case. Did I understand you correctly?
There was a problem hiding this comment.
NewPipe allows customizing main page tabs, and even insert local playlists there. See "Settings -> Content -> Content of main page -> + -> Playlist page". That option adds a playlist in the main page (not a "Bookmarked playlists" tab, but really a "Playlist" tab!).
triallax
added
database
TobiGr
force-pushed
the
fix-readd-to-playlist
branch
from
88138f1 to
2de8282
Compare
|
|
Kudos, SonarCloud Quality Gate passed! |
TobiGr
force-pushed
the
fix-readd-to-playlist
branch
3 times, most recently
from
5fbbcf4 to
b1a1e78
Compare
…when re-adding videos to the playlist
TobiGr
force-pushed
the
fix-readd-to-playlist
branch
from
b1a1e78 to
3d5a8af
Compare
|
Kudos, SonarCloud Quality Gate passed! |
What is it?
Description of the changes in your PR
The reason for this issue is an inconsistency between user interaction and database commit order.
Expected behavior
delete - commit (delete) - add - commit (add)
Actual behavior
delete - add - commit (add, commit immediately) - commit (delete, commit after ~10s due to the debouncedSaveSignal)
Best Solution
Removes all debounced commit strategies. Sequential consistency is guaranteed by database transactions.
But I don't know why the delete operation uses the deferred commit strategy. BTW, I realize PR #8221 could have many conflicts with this solution. So I took a conservative approach to fix it.
Current Solution
Concurrent operations on the playlist database occur if and only if the user opens both LocalPlaylistFragment and VideoDetailFragment as described in the related issue. So I force all delete operations to be committed to the database before adding.
Before/After Screenshots/Screen Record
before_small-mute.mp4
after_small-mute.mp4
From main page tab:
after2_mute.mp4
Fixes the following issue(s)
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