From 3c4882569931f91de0fc5dd0a980739781d6526c Mon Sep 17 00:00:00 2001 From: GGAutomaton <32899400+GGAutomaton@users.noreply.github.com> Date: Fri, 15 Apr 2022 20:44:54 +0800 Subject: [PATCH] Debounced saver & bugfix & clean code --- .../database/playlist/PlaylistLocalItem.java | 4 + .../playlist/PlaylistMetadataEntry.java | 2 +- .../database/playlist/dao/PlaylistDAO.java | 14 + .../playlist/dao/PlaylistStreamDAO.java | 14 +- .../playlist/model/PlaylistEntity.java | 11 + .../playlist/model/PlaylistRemoteEntity.java | 2 +- .../newpipe/local/LocalItemListAdapter.java | 1 - .../local/bookmark/BookmarkFragment.java | 286 +++++++++++++----- .../local/dialog/PlaylistAppendDialog.java | 2 +- .../local/playlist/LocalPlaylistManager.java | 24 +- .../local/playlist/RemotePlaylistManager.java | 26 +- 11 files changed, 288 insertions(+), 98 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistLocalItem.java b/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistLocalItem.java index 5bf50cd97..47c6dd617 100644 --- a/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistLocalItem.java +++ b/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistLocalItem.java @@ -45,6 +45,10 @@ public interface PlaylistLocalItem extends LocalItem { addItem(result, localPlaylists.get(i), itemsWithSameIndex); i++; } + while (j < remotePlaylists.size()) { + addItem(result, remotePlaylists.get(j), itemsWithSameIndex); + j++; + } addItemsWithSameIndex(result, itemsWithSameIndex); return result; diff --git a/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistMetadataEntry.java b/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistMetadataEntry.java index f54ffff13..ff80049a3 100644 --- a/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistMetadataEntry.java +++ b/app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistMetadataEntry.java @@ -17,7 +17,7 @@ public class PlaylistMetadataEntry implements PlaylistLocalItem { @ColumnInfo(name = PLAYLIST_THUMBNAIL_URL) public final String thumbnailUrl; @ColumnInfo(name = PLAYLIST_DISPLAY_INDEX) - public final long displayIndex; + public long displayIndex; @ColumnInfo(name = PLAYLIST_STREAM_COUNT) public final long streamCount; diff --git a/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistDAO.java b/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistDAO.java index 70aaa3b2d..d8071e0af 100644 --- a/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistDAO.java +++ b/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistDAO.java @@ -2,6 +2,7 @@ package org.schabi.newpipe.database.playlist.dao; import androidx.room.Dao; import androidx.room.Query; +import androidx.room.Transaction; import org.schabi.newpipe.database.BasicDAO; import org.schabi.newpipe.database.playlist.model.PlaylistEntity; @@ -36,4 +37,17 @@ public interface PlaylistDAO extends BasicDAO { @Query("SELECT COUNT(*) FROM " + PLAYLIST_TABLE) Flowable getCount(); + + @Transaction + default long upsertPlaylist(final PlaylistEntity playlist) { + final long playlistId = playlist.getUid(); + + if (playlistId == -1) { + // This situation is probably impossible. + return insert(playlist); + } else { + update(playlist); + return playlistId; + } + } } diff --git a/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistStreamDAO.java b/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistStreamDAO.java index 3fb96a21f..0fce984f3 100644 --- a/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistStreamDAO.java +++ b/app/src/main/java/org/schabi/newpipe/database/playlist/dao/PlaylistStreamDAO.java @@ -82,7 +82,19 @@ public interface PlaylistStreamDAO extends BasicDAO { + " FROM " + PLAYLIST_TABLE + " LEFT JOIN " + PLAYLIST_STREAM_JOIN_TABLE + " ON " + PLAYLIST_ID + " = " + JOIN_PLAYLIST_ID - + " GROUP BY " + JOIN_PLAYLIST_ID + + " GROUP BY " + PLAYLIST_ID + " ORDER BY " + PLAYLIST_NAME + " COLLATE NOCASE ASC") Flowable> getPlaylistMetadata(); + + @Transaction + @Query("SELECT " + PLAYLIST_ID + ", " + PLAYLIST_NAME + ", " + PLAYLIST_THUMBNAIL_URL + ", " + + PLAYLIST_DISPLAY_INDEX + ", " + + "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 + + " GROUP BY " + PLAYLIST_ID + + " ORDER BY " + PLAYLIST_DISPLAY_INDEX) + Flowable> getDisplayIndexOrderedPlaylistMetadata(); } diff --git a/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistEntity.java b/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistEntity.java index cdbbdebc0..272e8a5bc 100644 --- a/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistEntity.java +++ b/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistEntity.java @@ -2,12 +2,15 @@ package org.schabi.newpipe.database.playlist.model; import androidx.room.ColumnInfo; import androidx.room.Entity; +import androidx.room.Ignore; import androidx.room.Index; import androidx.room.PrimaryKey; 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.database.playlist.PlaylistMetadataEntry; + @Entity(tableName = PLAYLIST_TABLE, indices = {@Index(value = {PLAYLIST_NAME})}) public class PlaylistEntity { @@ -36,6 +39,14 @@ public class PlaylistEntity { this.displayIndex = displayIndex; } + @Ignore + public PlaylistEntity(final PlaylistMetadataEntry item) { + this.uid = item.uid; + this.name = item.name; + this.thumbnailUrl = item.thumbnailUrl; + this.displayIndex = item.displayIndex; + } + public long getUid() { return uid; } diff --git a/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistRemoteEntity.java b/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistRemoteEntity.java index 454526769..adea2738b 100644 --- a/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistRemoteEntity.java +++ b/app/src/main/java/org/schabi/newpipe/database/playlist/model/PlaylistRemoteEntity.java @@ -54,7 +54,7 @@ public class PlaylistRemoteEntity implements PlaylistLocalItem { private String uploader; @ColumnInfo(name = REMOTE_PLAYLIST_DISPLAY_INDEX) - private long displayIndex; + private long displayIndex = -1; // Make sure the new item is on the top @ColumnInfo(name = REMOTE_PLAYLIST_STREAM_COUNT) private Long streamCount; diff --git a/app/src/main/java/org/schabi/newpipe/local/LocalItemListAdapter.java b/app/src/main/java/org/schabi/newpipe/local/LocalItemListAdapter.java index e2bfd5977..05e2fdac0 100644 --- a/app/src/main/java/org/schabi/newpipe/local/LocalItemListAdapter.java +++ b/app/src/main/java/org/schabi/newpipe/local/LocalItemListAdapter.java @@ -142,7 +142,6 @@ public class LocalItemListAdapter extends RecyclerView.Adapter, Void> { + // todo: add to playlists, item handle should be invisible + + // Save the list 10s after the last change occurred + private static final long SAVE_DEBOUNCE_MILLIS = 10000; private static final int MINIMUM_INITIAL_DRAG_VELOCITY = 12; @State protected Parcelable itemsListState; @@ -55,6 +67,16 @@ public final class BookmarkFragment extends BaseLocalListFragment debouncedSaveSignal; + + /* Has the playlist been fully loaded from db */ + private AtomicBoolean isLoadingComplete; + /* Has the playlist been modified (e.g. items reordered or deleted) */ + private AtomicBoolean isModified; + + // Map from (uid, local/remote item) to the saved display index in the database. + private Map, Long> displayIndexInDatabase; + /////////////////////////////////////////////////////////////////////////// // Fragment LifeCycle - Creation /////////////////////////////////////////////////////////////////////////// @@ -69,6 +91,12 @@ public final class BookmarkFragment extends BaseLocalListFragment(); } @Nullable @@ -154,6 +182,10 @@ public final class BookmarkFragment extends BaseLocalListFragment subscriptions) { - - // If displayIndex does not match actual index, update displayIndex. - // This may happen when a new list is created - // or on the first run after database update - // or displayIndex is not continuous for some reason. - checkDisplayIndexUpdate(subscriptions); - - handleResult(subscriptions); + if (isModified == null || !isModified.get()) { + checkDisplayIndexModified(subscriptions); + handleResult(subscriptions); + isLoadingComplete.set(true); + } if (databaseSubscription != null) { databaseSubscription.request(1); } @@ -296,86 +338,170 @@ public final class BookmarkFragment extends BaseLocalListFragment result) { + if (isModified != null && isModified.get()) { return; } - if (DEBUG) { - Log.d(TAG, "Updating local playlist id=[" + id + "] " - + "with new display_index=[" + displayIndex + "]"); - } + displayIndexInDatabase.clear(); - final Disposable disposable = - localPlaylistManager.changePlaylistDisplayIndex(id, displayIndex) - .observeOn(AndroidSchedulers.mainThread()) - .subscribe(longs -> { /*Do nothing on success*/ }, throwable -> showError( - new ErrorInfo(throwable, - UserAction.REQUESTED_BOOKMARK, - "Changing local playlist display_index"))); - disposables.add(disposable); - } - - private void changeRemotePlaylistDisplayIndex(final long id, final long displayIndex) { - - if (remotePlaylistManager == null) { - return; - } - - if (DEBUG) { - Log.d(TAG, "Updating remote playlist id=[" + id + "] " - + "with new display_index=[" + displayIndex + "]"); - } - - final Disposable disposable = - remotePlaylistManager.changePlaylistDisplayIndex(id, displayIndex) - .observeOn(AndroidSchedulers.mainThread()) - .subscribe(longs -> { /*Do nothing on success*/ }, throwable -> showError( - new ErrorInfo(throwable, - UserAction.REQUESTED_BOOKMARK, - "Changing remote playlist display_index"))); - disposables.add(disposable); - } - - private void checkDisplayIndexUpdate(@NonNull final List result) { + // If the display index does not match actual index in the list, update the display index. + // This may happen when a new list is created + // or on the first run after database update + // or displayIndex is not continuous for some reason. + boolean isDisplayIndexModified = false; for (int i = 0; i < result.size(); i++) { final PlaylistLocalItem item = result.get(i); if (item.getDisplayIndex() != i) { - if (item instanceof PlaylistMetadataEntry) { - changeLocalPlaylistDisplayIndex(((PlaylistMetadataEntry) item).uid, i); - } else if (item instanceof PlaylistRemoteEntity) { - changeRemotePlaylistDisplayIndex(((PlaylistRemoteEntity) item).getUid(), i); - } + isDisplayIndexModified = true; } + + // Updating display index in the item does not affect the value inserts into + // database, which will be recalculated during the database update. Updating + // display index in the item here is to determine whether it is recently modified. + // Save the index read from the database. + if (item instanceof PlaylistMetadataEntry) { + + displayIndexInDatabase.put(new Pair<>(((PlaylistMetadataEntry) item).uid, + LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM), item.getDisplayIndex()); + ((PlaylistMetadataEntry) item).displayIndex = i; + + } else if (item instanceof PlaylistRemoteEntity) { + + displayIndexInDatabase.put(new Pair<>(((PlaylistRemoteEntity) item).getUid(), + LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM), + item.getDisplayIndex()); + ((PlaylistRemoteEntity) item).setDisplayIndex(i); + + } + } + + if (isDisplayIndexModified) { + saveChanges(); } } - private void saveImmediate() { - if (localPlaylistManager == null || remotePlaylistManager == null - || itemListAdapter == null) { + private void saveChanges() { + if (isModified == null || debouncedSaveSignal == null) { return; } - // todo: debounce - /* + + isModified.set(true); + debouncedSaveSignal.onNext(System.currentTimeMillis()); + } + + private Disposable getDebouncedSaver() { + if (debouncedSaveSignal == null) { + return Disposable.empty(); + } + + return debouncedSaveSignal + .debounce(SAVE_DEBOUNCE_MILLIS, TimeUnit.MILLISECONDS) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe(ignored -> saveImmediate(), throwable -> + showError(new ErrorInfo(throwable, UserAction.SOMETHING_ELSE, + "Debounced saver"))); + } + + private void saveImmediate() { + if (itemListAdapter == null) { + return; + } + // List must be loaded and modified in order to save if (isLoadingComplete == null || isModified == null || !isLoadingComplete.get() || !isModified.get()) { - Log.w(TAG, "Attempting to save playlist when local playlist " - + "is not loaded or not modified: playlist id=[" + playlistId + "]"); + Log.w(TAG, "Attempting to save playlists in bookmark when bookmark " + + "is not loaded or playlists not modified"); return; } - */ - // todo: is it correct? + final List items = itemListAdapter.getItemsList(); + final List localItemsUpdate = new ArrayList<>(); + final List localItemsDeleteUid = new ArrayList<>(); + final List remoteItemsUpdate = new ArrayList<>(); + final List remoteItemsDeleteUid = new ArrayList<>(); + + // Calculate display index for (int i = 0; i < items.size(); i++) { final LocalItem item = items.get(i); + if (item instanceof PlaylistMetadataEntry) { - changeLocalPlaylistDisplayIndex(((PlaylistMetadataEntry) item).uid, i); + ((PlaylistMetadataEntry) item).displayIndex = i; + + final Long uid = ((PlaylistMetadataEntry) item).uid; + final Pair key = new Pair<>(uid, + LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM); + final Long databaseIndex = displayIndexInDatabase.remove(key); + + if (databaseIndex != null) { + if (databaseIndex != i) { + localItemsUpdate.add((PlaylistMetadataEntry) item); + } + } else { + // This should be impossible. + continue; + } } else if (item instanceof PlaylistRemoteEntity) { - changeLocalPlaylistDisplayIndex(((PlaylistRemoteEntity) item).getUid(), i); + ((PlaylistRemoteEntity) item).setDisplayIndex(i); + + final Long uid = ((PlaylistRemoteEntity) item).getUid(); + final Pair key = new Pair<>(uid, + LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM); + final Long databaseIndex = displayIndexInDatabase.remove(key); + + if (databaseIndex != null) { + if (databaseIndex != i) { + remoteItemsUpdate.add((PlaylistRemoteEntity) item); + } + } else { + // This should be impossible. + continue; + } } } + + // Find deleted items + for (final Pair key : displayIndexInDatabase.keySet()) { + if (key.second.equals(LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM)) { + localItemsDeleteUid.add(key.first); + } else if (key.second.equals(LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM)) { + remoteItemsDeleteUid.add(key.first); + } + } + + displayIndexInDatabase.clear(); + + // 1. Update local playlists + // 2. Update remote playlists + // 3. Set isModified false + disposables.add(localPlaylistManager.updatePlaylists(localItemsUpdate, localItemsDeleteUid) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe(() -> disposables.add(remotePlaylistManager.updatePlaylists( + remoteItemsUpdate, remoteItemsDeleteUid) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe(() -> { + if (isModified != null) { + isModified.set(false); + } + }, + throwable -> showError(new ErrorInfo(throwable, + UserAction.REQUESTED_BOOKMARK, + "Saving playlist")) + )), + throwable -> showError(new ErrorInfo(throwable, + UserAction.REQUESTED_BOOKMARK, "Saving playlist")) + )); + } private ItemTouchHelper.SimpleCallback getItemTouchCallback() { @@ -404,17 +530,26 @@ public final class BookmarkFragment extends BaseLocalListFragment { - showDeleteDialog(selectedItem.name, - localPlaylistManager.deletePlaylist(selectedItem.uid)); + showDeleteDialog(selectedItem.name, selectedItem); dialog.dismiss(); }) .create() .show(); } - private void showDeleteDialog(final String name, final Single deleteReactor) { + private void showDeleteDialog(final String name, final PlaylistLocalItem item) { if (activity == null || disposables == null) { return; } @@ -476,13 +610,7 @@ public final class BookmarkFragment extends BaseLocalListFragment - disposables.add(deleteReactor - .observeOn(AndroidSchedulers.mainThread()) - .subscribe(ignored -> { /*Do nothing on success*/ }, throwable -> - showError(new ErrorInfo(throwable, - UserAction.REQUESTED_BOOKMARK, - "Deleting playlist"))))) + .setPositiveButton(R.string.delete, (dialog, i) -> deleteItem(item)) .setNegativeButton(R.string.cancel, null) .show(); } diff --git a/app/src/main/java/org/schabi/newpipe/local/dialog/PlaylistAppendDialog.java b/app/src/main/java/org/schabi/newpipe/local/dialog/PlaylistAppendDialog.java index a874cdd62..58a10af22 100644 --- a/app/src/main/java/org/schabi/newpipe/local/dialog/PlaylistAppendDialog.java +++ b/app/src/main/java/org/schabi/newpipe/local/dialog/PlaylistAppendDialog.java @@ -85,7 +85,7 @@ public final class PlaylistAppendDialog extends PlaylistDialog { final View newPlaylistButton = view.findViewById(R.id.newPlaylist); newPlaylistButton.setOnClickListener(ignored -> openCreatePlaylistDialog()); - playlistDisposables.add(playlistManager.getPlaylists() + playlistDisposables.add(playlistManager.getDisplayIndexOrderedPlaylists() .observeOn(AndroidSchedulers.mainThread()) .subscribe(this::onPlaylistsReceived)); } diff --git a/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistManager.java b/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistManager.java index 47817f9e4..c68a22b01 100644 --- a/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistManager.java +++ b/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistManager.java @@ -41,6 +41,7 @@ public class LocalPlaylistManager { } final StreamEntity defaultStream = streams.get(0); + // Save to the database directly. // Make sure the new playlist is always on the top of bookmark. // The index will be reassigned to non-negative number in BookmarkFragment. final PlaylistEntity newPlaylist = @@ -85,10 +86,31 @@ public class LocalPlaylistManager { })).subscribeOn(Schedulers.io()); } + public Completable updatePlaylists(final List updateItems, + final List deletedItems) { + final List items = new ArrayList<>(updateItems.size()); + for (final PlaylistMetadataEntry item : updateItems) { + items.add(new PlaylistEntity(item)); + } + return Completable.fromRunnable(() -> database.runInTransaction(() -> { + for (final Long uid: deletedItems) { + playlistTable.deletePlaylist(uid); + } + for (final PlaylistEntity item: items) { + playlistTable.upsertPlaylist(item); + } + })).subscribeOn(Schedulers.io()); + } + public Flowable> getPlaylists() { return playlistStreamTable.getPlaylistMetadata().subscribeOn(Schedulers.io()); } + public Flowable> getDisplayIndexOrderedPlaylists() { + return playlistStreamTable.getDisplayIndexOrderedPlaylistMetadata() + .subscribeOn(Schedulers.io()); + } + public Flowable> getPlaylistStreams(final long playlistId) { return playlistStreamTable.getOrderedStreamsOf(playlistId).subscribeOn(Schedulers.io()); } @@ -107,7 +129,7 @@ public class LocalPlaylistManager { return modifyPlaylist(playlistId, null, thumbnailUrl, -1); } - public Maybe changePlaylistDisplayIndex(final long playlistId, + public Maybe updatePlaylistDisplayIndex(final long playlistId, final long displayIndex) { return modifyPlaylist(playlistId, null, null, displayIndex); } diff --git a/app/src/main/java/org/schabi/newpipe/local/playlist/RemotePlaylistManager.java b/app/src/main/java/org/schabi/newpipe/local/playlist/RemotePlaylistManager.java index b49f149d6..1dbd726ae 100644 --- a/app/src/main/java/org/schabi/newpipe/local/playlist/RemotePlaylistManager.java +++ b/app/src/main/java/org/schabi/newpipe/local/playlist/RemotePlaylistManager.java @@ -7,16 +7,18 @@ import org.schabi.newpipe.extractor.playlist.PlaylistInfo; import java.util.List; +import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Flowable; -import io.reactivex.rxjava3.core.Maybe; import io.reactivex.rxjava3.core.Single; import io.reactivex.rxjava3.schedulers.Schedulers; public class RemotePlaylistManager { + private final AppDatabase database; private final PlaylistRemoteDAO playlistRemoteTable; public RemotePlaylistManager(final AppDatabase db) { + database = db; playlistRemoteTable = db.playlistRemoteDAO(); } @@ -34,18 +36,16 @@ public class RemotePlaylistManager { .subscribeOn(Schedulers.io()); } - public Maybe changePlaylistDisplayIndex(final long playlistId, - final long displayIndex) { - return playlistRemoteTable.getPlaylist(playlistId) - .firstElement() - .filter(playlistRemoteEntities -> !playlistRemoteEntities.isEmpty()) - .map(playlistRemoteEntities -> { - final PlaylistRemoteEntity playlist = playlistRemoteEntities.get(0); - if (displayIndex != -1) { - playlist.setDisplayIndex(displayIndex); - } - return playlistRemoteTable.update(playlist); - }).subscribeOn(Schedulers.io()); + public Completable updatePlaylists(final List updateItems, + final List deletedItems) { + return Completable.fromRunnable(() -> database.runInTransaction(() -> { + for (final Long uid: deletedItems) { + playlistRemoteTable.deletePlaylist(uid); + } + for (final PlaylistRemoteEntity item: updateItems) { + playlistRemoteTable.upsert(item); + } + })).subscribeOn(Schedulers.io()); } public Single onBookmark(final PlaylistInfo playlistInfo) {