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.

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

Fixed a bug that caused erroneous updates of the playlist thumbnails #9755

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
737 changes: 737 additions & 0 deletions app/schemas/org.schabi.newpipe.database.AppDatabase/7.json

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ class DatabaseMigrationTest {
Migrations.MIGRATION_5_6
)

testHelper.runMigrationsAndValidate(
AppDatabase.DATABASE_NAME,
Migrations.DB_VER_7,
true,
Migrations.MIGRATION_6_7
)

val migratedDatabaseV3 = getMigratedDatabase()
val listFromDB = migratedDatabaseV3.streamDAO().all.blockingFirst()

Expand Down
3 changes: 2 additions & 1 deletion app/src/main/java/org/schabi/newpipe/NewPipeDatabase.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import static org.schabi.newpipe.database.Migrations.MIGRATION_3_4;
import static org.schabi.newpipe.database.Migrations.MIGRATION_4_5;
import static org.schabi.newpipe.database.Migrations.MIGRATION_5_6;
import static org.schabi.newpipe.database.Migrations.MIGRATION_6_7;

import android.content.Context;
import android.database.Cursor;
Expand All @@ -26,7 +27,7 @@ private static AppDatabase getDatabase(final Context context) {
return Room
.databaseBuilder(context.getApplicationContext(), AppDatabase.class, DATABASE_NAME)
.addMigrations(MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5,
MIGRATION_5_6)
MIGRATION_5_6, MIGRATION_6_7)
.build();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.schabi.newpipe.database;

import static org.schabi.newpipe.database.Migrations.DB_VER_6;
import static org.schabi.newpipe.database.Migrations.DB_VER_7;

import androidx.room.Database;
import androidx.room.RoomDatabase;
Expand Down Expand Up @@ -38,7 +38,7 @@
FeedEntity.class, FeedGroupEntity.class, FeedGroupSubscriptionEntity.class,
FeedLastUpdatedEntity.class
},
version = DB_VER_6
version = DB_VER_7
)
public abstract class AppDatabase extends RoomDatabase {
public static final String DATABASE_NAME = "newpipe.db";
Expand Down
42 changes: 42 additions & 0 deletions app/src/main/java/org/schabi/newpipe/database/Migrations.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public final class Migrations {
public static final int DB_VER_4 = 4;
public static final int DB_VER_5 = 5;
public static final int DB_VER_6 = 6;
public static final int DB_VER_7 = 7;

private static final String TAG = Migrations.class.getName();
public static final boolean DEBUG = MainActivity.DEBUG;
Expand Down Expand Up @@ -197,6 +198,47 @@ public void migrate(@NonNull final SupportSQLiteDatabase database) {
}
};

public static final Migration MIGRATION_6_7 = new Migration(DB_VER_6, DB_VER_7) {
@Override
public void migrate(@NonNull final SupportSQLiteDatabase database) {
// Create a new column thumbnail_stream_id
database.execSQL("ALTER TABLE `playlists` ADD COLUMN `thumbnail_stream_id` "
+ "INTEGER NOT NULL DEFAULT -1");

// Migrate the thumbnail_url to the thumbnail_stream_id
database.execSQL("CREATE TEMPORARY TABLE temporary_table AS"
+ " SELECT p.uid AS playlist_uid, s.uid AS stream_uid"
+ " FROM playlists p"
+ " LEFT JOIN playlist_stream_join ps ON p.uid = ps.playlist_id"
+ " LEFT JOIN streams s ON s.uid = ps.stream_id"
+ " WHERE s.thumbnail_url = p.thumbnail_url");

database.execSQL("UPDATE playlists SET thumbnail_stream_id = ("
+ "SELECT CASE WHEN COUNT(*) != 0 then stream_uid ELSE -1 END "
+ "FROM temporary_table "
Jared234 marked this conversation as resolved.
Show resolved Hide resolved
+ "WHERE playlist_uid = playlists.uid)");

database.execSQL("DROP TABLE temporary_table");

// Remove the thumbnail_url field in the playlist table
database.execSQL("CREATE TABLE IF NOT EXISTS `playlists_new`"
+ "(uid INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "
+ "name TEXT, "
+ "is_thumbnail_permanent INTEGER NOT NULL, "
+ "thumbnail_stream_id INTEGER NOT NULL)");
Stypox marked this conversation as resolved.
Show resolved Hide resolved

database.execSQL("INSERT INTO playlists_new"
+ " SELECT uid, name, is_thumbnail_permanent, thumbnail_stream_id "
+ " FROM playlists");


database.execSQL("DROP TABLE playlists");
database.execSQL("ALTER TABLE playlists_new RENAME TO playlists");
database.execSQL("CREATE INDEX IF NOT EXISTS "
+ "`index_playlists_name` ON `playlists` (`name`)");
}
};

private Migrations() {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@

import static org.schabi.newpipe.database.playlist.PlaylistDuplicatesEntry.PLAYLIST_TIMES_STREAM_IS_CONTAINED;
import static org.schabi.newpipe.database.playlist.PlaylistMetadataEntry.PLAYLIST_STREAM_COUNT;
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.DEFAULT_THUMBNAIL;
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.PLAYLIST_ID;
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.PLAYLIST_NAME;
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.PLAYLIST_TABLE;
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.PLAYLIST_THUMBNAIL_STREAM_ID;
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.PLAYLIST_THUMBNAIL_URL;
import static org.schabi.newpipe.database.playlist.model.PlaylistStreamEntity.JOIN_INDEX;
import static org.schabi.newpipe.database.playlist.model.PlaylistStreamEntity.JOIN_PLAYLIST_ID;
Expand Down Expand Up @@ -57,14 +59,14 @@ default Flowable<List<PlaylistStreamEntity>> listByService(final int serviceId)
+ " WHERE " + JOIN_PLAYLIST_ID + " = :playlistId")
Flowable<Integer> getMaximumIndexOf(long playlistId);

@Query("SELECT CASE WHEN COUNT(*) != 0 then " + STREAM_THUMBNAIL_URL + " ELSE :defaultUrl END"
@Query("SELECT CASE WHEN COUNT(*) != 0 then " + STREAM_ID + " ELSE -1 END"
+ " FROM " + STREAM_TABLE
+ " LEFT JOIN " + PLAYLIST_STREAM_JOIN_TABLE
+ " ON " + STREAM_ID + " = " + JOIN_STREAM_ID
+ " WHERE " + JOIN_PLAYLIST_ID + " = :playlistId "
+ " LIMIT 1"
)
Flowable<String> getAutomaticThumbnailUrl(long playlistId, String defaultUrl);
Flowable<Long> getAutomaticThumbnailUrl(long playlistId);
Jared234 marked this conversation as resolved.
Show resolved Hide resolved

@RewriteQueriesToDropUnusedColumns
@Transaction
Expand All @@ -87,21 +89,36 @@ default Flowable<List<PlaylistStreamEntity>> listByService(final int serviceId)
Flowable<List<PlaylistStreamEntry>> getOrderedStreamsOf(long playlistId);

@Transaction
@Query("SELECT " + PLAYLIST_ID + ", " + PLAYLIST_NAME + ", " + PLAYLIST_THUMBNAIL_URL + ", "
+ "COALESCE(COUNT(" + JOIN_PLAYLIST_ID + "), 0) AS " + PLAYLIST_STREAM_COUNT
@Query("SELECT " + PLAYLIST_ID + ", " + PLAYLIST_NAME + ","

+ " CASE WHEN " + PLAYLIST_THUMBNAIL_STREAM_ID + " = -1"
+ " THEN " + "'" + DEFAULT_THUMBNAIL + "'"
+ " ELSE (SELECT " + STREAM_THUMBNAIL_URL
+ " FROM " + STREAM_TABLE
+ " WHERE " + STREAM_TABLE + "." + STREAM_ID + " = " + PLAYLIST_THUMBNAIL_STREAM_ID
+ " ) END AS " + PLAYLIST_THUMBNAIL_URL + ", "

+ PLAYLIST_NAME + ", "
Jared234 marked this conversation as resolved.
Show resolved Hide resolved
+ "COALESCE(COUNT(" + JOIN_PLAYLIST_ID + "), 0) AS " + PLAYLIST_STREAM_COUNT
+ " FROM " + PLAYLIST_TABLE
+ " LEFT JOIN " + PLAYLIST_STREAM_JOIN_TABLE
+ " ON " + PLAYLIST_ID + " = " + JOIN_PLAYLIST_ID
+ " ON " + PLAYLIST_TABLE + "." + PLAYLIST_ID + " = " + JOIN_PLAYLIST_ID
+ " GROUP BY " + PLAYLIST_ID
+ " ORDER BY " + PLAYLIST_NAME + " COLLATE NOCASE ASC")
Flowable<List<PlaylistMetadataEntry>> getPlaylistMetadata();

@Transaction
@Query("SELECT " + PLAYLIST_TABLE + "." + PLAYLIST_ID + ", "
+ PLAYLIST_NAME + ", "
+ PLAYLIST_TABLE + "." + PLAYLIST_THUMBNAIL_URL + ", "
+ "COALESCE(COUNT(" + JOIN_PLAYLIST_ID + "), 0) AS " + PLAYLIST_STREAM_COUNT + ", "

+ " CASE WHEN " + PLAYLIST_THUMBNAIL_STREAM_ID + " = -1"
+ " THEN " + "'" + DEFAULT_THUMBNAIL + "'"
+ " ELSE (SELECT " + STREAM_THUMBNAIL_URL
+ " FROM " + STREAM_TABLE
+ " WHERE " + STREAM_TABLE + "." + STREAM_ID + " = " + PLAYLIST_THUMBNAIL_STREAM_ID
+ " ) END AS " + PLAYLIST_THUMBNAIL_URL

+ ", COALESCE(COUNT(" + JOIN_PLAYLIST_ID + "), 0) AS " + PLAYLIST_STREAM_COUNT + ", "
Jared234 marked this conversation as resolved.
Show resolved Hide resolved
+ "COALESCE(SUM(" + STREAM_URL + " = :streamUrl), 0) AS "
+ PLAYLIST_TIMES_STREAM_IS_CONTAINED

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,20 @@
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.PLAYLIST_NAME;
import static org.schabi.newpipe.database.playlist.model.PlaylistEntity.PLAYLIST_TABLE;

import org.schabi.newpipe.R;

@Entity(tableName = PLAYLIST_TABLE,
indices = {@Index(value = {PLAYLIST_NAME})})
public class PlaylistEntity {

public static final String DEFAULT_THUMBNAIL = "drawable://"
+ R.drawable.placeholder_thumbnail_playlist;
public static final String PLAYLIST_TABLE = "playlists";
public static final String PLAYLIST_ID = "uid";
public static final String PLAYLIST_NAME = "name";
public static final String PLAYLIST_THUMBNAIL_URL = "thumbnail_url";
public static final String PLAYLIST_THUMBNAIL_PERMANENT = "is_thumbnail_permanent";
public static final String PLAYLIST_THUMBNAIL_STREAM_ID = "thumbnail_stream_id";

@PrimaryKey(autoGenerate = true)
@ColumnInfo(name = PLAYLIST_ID)
Expand All @@ -24,17 +30,17 @@ public class PlaylistEntity {
@ColumnInfo(name = PLAYLIST_NAME)
private String name;

@ColumnInfo(name = PLAYLIST_THUMBNAIL_URL)
private String thumbnailUrl;

@ColumnInfo(name = PLAYLIST_THUMBNAIL_PERMANENT)
private boolean isThumbnailPermanent;

public PlaylistEntity(final String name, final String thumbnailUrl,
final boolean isThumbnailPermanent) {
@ColumnInfo(name = PLAYLIST_THUMBNAIL_STREAM_ID)
private long thumbnailStreamId;

public PlaylistEntity(final String name, final boolean isThumbnailPermanent,
final long thumbnailStreamId) {
this.name = name;
this.thumbnailUrl = thumbnailUrl;
this.isThumbnailPermanent = isThumbnailPermanent;
this.thumbnailStreamId = thumbnailStreamId;
}

public long getUid() {
Expand All @@ -53,12 +59,12 @@ public void setName(final String name) {
this.name = name;
}

public String getThumbnailUrl() {
return thumbnailUrl;
public long getThumbnailStreamId() {
return thumbnailStreamId;
}

public void setThumbnailUrl(final String thumbnailUrl) {
this.thumbnailUrl = thumbnailUrl;
public void setThumbnailStreamId(final long thumbnailStreamId) {
this.thumbnailStreamId = thumbnailStreamId;
}

public boolean getIsThumbnailPermanent() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,10 @@ private void showLocalDialog(final PlaylistMetadataEntry selectedItem) {
showDeleteDialog(selectedItem.name,
localPlaylistManager.deletePlaylist(selectedItem.uid));
} else if (isThumbnailPermanent && items.get(index).equals(unsetThumbnail)) {
final String thumbnailUrl = localPlaylistManager
.getAutomaticPlaylistThumbnail(selectedItem.uid);
final long thumbnailStreamId = localPlaylistManager
.getAutomaticPlaylistThumbnailStreamId(selectedItem.uid);
localPlaylistManager
.changePlaylistThumbnail(selectedItem.uid, thumbnailUrl, false)
.changePlaylistThumbnail(selectedItem.uid, thumbnailStreamId, false)
.observeOn(AndroidSchedulers.mainThread())
.subscribe();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import org.schabi.newpipe.NewPipeDatabase;
import org.schabi.newpipe.R;
import org.schabi.newpipe.database.playlist.model.PlaylistEntity;
import org.schabi.newpipe.database.playlist.PlaylistDuplicatesEntry;
import org.schabi.newpipe.database.stream.model.StreamEntity;
import org.schabi.newpipe.local.LocalItemListAdapter;
Expand Down Expand Up @@ -154,17 +155,19 @@ private void onPlaylistSelected(@NonNull final LocalPlaylistManager manager,

final Toast successToast = Toast.makeText(getContext(), toastText, Toast.LENGTH_SHORT);

if (playlist.thumbnailUrl
.equals("drawable://" + R.drawable.placeholder_thumbnail_playlist)) {
playlistDisposables.add(manager
.changePlaylistThumbnail(playlist.uid, streams.get(0).getThumbnailUrl(), false)
.observeOn(AndroidSchedulers.mainThread())
.subscribe(ignored -> successToast.show()));
}

playlistDisposables.add(manager.appendToPlaylist(playlist.uid, streams)
.observeOn(AndroidSchedulers.mainThread())
.subscribe(ignored -> successToast.show()));
.subscribe(ignored -> {
successToast.show();

if (playlist.thumbnailUrl.equals(PlaylistEntity.DEFAULT_THUMBNAIL)) {
playlistDisposables.add(manager
.changePlaylistThumbnail(playlist.uid, streams.get(0).getUid(),
false)
.observeOn(AndroidSchedulers.mainThread())
.subscribe(ignore -> successToast.show()));
}
Stypox marked this conversation as resolved.
Show resolved Hide resolved
}));

requireDialog().dismiss();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@
import io.reactivex.rxjava3.subjects.PublishSubject;

public class LocalPlaylistFragment extends BaseLocalListFragment<List<PlaylistStreamEntry>, Void> {
public static final long DEFAULT_THUMBNAIL_ID = -1;
Jared234 marked this conversation as resolved.
Show resolved Hide resolved
public static final long NO_THUMBNAIL_ID = -2;
Jared234 marked this conversation as resolved.
Show resolved Hide resolved
// Save the list 10 seconds after the last change occurred
private static final long SAVE_DEBOUNCE_MILLIS = 10000;
private static final int MINIMUM_INITIAL_DRAG_VELOCITY = 12;
Expand Down Expand Up @@ -417,8 +419,8 @@ public void removeWatchedStreams(final boolean removePartiallyWatched) {
if (indexInHistory < 0) {
itemsToKeep.add(playlistItem);
} else if (!isThumbnailPermanent && !thumbnailVideoRemoved
&& playlistManager.getPlaylistThumbnail(playlistId)
.equals(playlistItem.getStreamEntity().getThumbnailUrl())) {
&& playlistManager.getPlaylistThumbnailStreamId(playlistId)
== playlistItem.getStreamEntity().getUid()) {
thumbnailVideoRemoved = true;
}
}
Expand All @@ -438,8 +440,8 @@ public void removeWatchedStreams(final boolean removePartiallyWatched) {
&& !streamStateEntity.isFinished(duration))) {
itemsToKeep.add(playlistItem);
} else if (!isThumbnailPermanent && !thumbnailVideoRemoved
&& playlistManager.getPlaylistThumbnail(playlistId)
.equals(playlistItem.getStreamEntity().getThumbnailUrl())) {
&& playlistManager.getPlaylistThumbnailStreamId(playlistId)
== playlistItem.getStreamEntity().getUid()) {
thumbnailVideoRemoved = true;
}
}
Expand Down Expand Up @@ -587,7 +589,7 @@ private void changePlaylistName(final String title) {
disposables.add(disposable);
}

private void changeThumbnailUrl(final String thumbnailUrl, final boolean isPermanent) {
private void changeThumbnailStreamId(final long thumbnailStreamId, final boolean isPermanent) {
if (playlistManager == null || (!isPermanent && playlistManager
.getIsPlaylistThumbnailPermanent(playlistId))) {
return;
Expand All @@ -599,11 +601,11 @@ private void changeThumbnailUrl(final String thumbnailUrl, final boolean isPerma

if (DEBUG) {
Log.d(TAG, "Updating playlist id=[" + playlistId + "] "
+ "with new thumbnail url=[" + thumbnailUrl + "]");
+ "with new thumbnail stream id=[" + thumbnailStreamId + "]");
}

final Disposable disposable = playlistManager
.changePlaylistThumbnail(playlistId, thumbnailUrl, isPermanent)
.changePlaylistThumbnail(playlistId, thumbnailStreamId, isPermanent)
.observeOn(AndroidSchedulers.mainThread())
.subscribe(ignore -> successToast.show(), throwable ->
showError(new ErrorInfo(throwable, UserAction.REQUESTED_BOOKMARK,
Expand All @@ -616,16 +618,16 @@ private void updateThumbnailUrl() {
return;
}

final String newThumbnailUrl;
final long thumbnailStreamId;

if (!itemListAdapter.getItemsList().isEmpty()) {
newThumbnailUrl = ((PlaylistStreamEntry) itemListAdapter.getItemsList().get(0))
.getStreamEntity().getThumbnailUrl();
thumbnailStreamId = ((PlaylistStreamEntry) itemListAdapter.getItemsList().get(0))
.getStreamEntity().getUid();
} else {
newThumbnailUrl = "drawable://" + R.drawable.placeholder_thumbnail_playlist;
thumbnailStreamId = DEFAULT_THUMBNAIL_ID;
}

changeThumbnailUrl(newThumbnailUrl, false);
changeThumbnailStreamId(thumbnailStreamId, false);
}

private void deleteItem(final PlaylistStreamEntry item) {
Expand All @@ -634,8 +636,7 @@ private void deleteItem(final PlaylistStreamEntry item) {
}

itemListAdapter.removeItem(item);
if (playlistManager.getPlaylistThumbnail(playlistId)
.equals(item.getStreamEntity().getThumbnailUrl())) {
if (playlistManager.getPlaylistThumbnailStreamId(playlistId) == item.getStreamId()) {
updateThumbnailUrl();
}

Expand Down Expand Up @@ -793,7 +794,7 @@ context, getPlayQueueStartingAt(item), true))
.setAction(
StreamDialogDefaultEntry.SET_AS_PLAYLIST_THUMBNAIL,
(f, i) ->
changeThumbnailUrl(item.getStreamEntity().getThumbnailUrl(),
changeThumbnailStreamId(item.getStreamEntity().getUid(),
true))
.setAction(
StreamDialogDefaultEntry.DELETE,
Expand Down
Loading