Skip to content

Commit 898a936

Browse files
GGAutomatonGGAutomaton
committed
Update index modification logic & redo sorting in the merge algorithm
1 parent 4e401bc commit 898a936

5 files changed

Lines changed: 39 additions & 101 deletions

File tree

‎app/src/main/java/org/schabi/newpipe/database/playlist/PlaylistLocalItem.java‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,29 +21,18 @@ public interface PlaylistLocalItem extends LocalItem {
2121
* Merge localPlaylists and remotePlaylists by the display index.
2222
* If two items have the same display index, sort them in {@code CASE_INSENSITIVE_ORDER}.
2323
*
24-
* @param localPlaylists local playlists in the display index order
25-
* @param remotePlaylists remote playlists in the display index order
24+
* @param localPlaylists local playlists
25+
* @param remotePlaylists remote playlists
2626
* @return merged playlists
2727
*/
2828
static List<PlaylistLocalItem> merge(
2929
final List<PlaylistMetadataEntry> localPlaylists,
3030
final List<PlaylistRemoteEntity> remotePlaylists) {
3131

32-
for (int i = 1; i < localPlaylists.size(); i++) {
33-
if (localPlaylists.get(i).getDisplayIndex()
34-
< localPlaylists.get(i - 1).getDisplayIndex()) {
35-
throw new IllegalArgumentException(
36-
"localPlaylists is not in the display index order");
37-
}
38-
}
39-
40-
for (int i = 1; i < remotePlaylists.size(); i++) {
41-
if (remotePlaylists.get(i).getDisplayIndex()
42-
< remotePlaylists.get(i - 1).getDisplayIndex()) {
43-
throw new IllegalArgumentException(
44-
"remotePlaylists is not in the display index order");
45-
}
46-
}
32+
Collections.sort(localPlaylists,
33+
Comparator.comparingLong(PlaylistMetadataEntry::getDisplayIndex));
34+
Collections.sort(remotePlaylists,
35+
Comparator.comparingLong(PlaylistRemoteEntity::getDisplayIndex));
4736

4837
// This algorithm is similar to the merge operation in merge sort.
4938

‎app/src/main/java/org/schabi/newpipe/local/bookmark/BookmarkFragment.java‎

Lines changed: 29 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,7 @@
4141
import org.schabi.newpipe.util.OnClickGesture;
4242

4343
import java.util.ArrayList;
44-
import java.util.HashMap;
4544
import java.util.List;
46-
import java.util.Map;
4745
import java.util.concurrent.atomic.AtomicBoolean;
4846

4947
import icepick.State;
@@ -70,8 +68,7 @@ public final class BookmarkFragment extends BaseLocalListFragment<List<PlaylistL
7068

7169
private DebounceSaver debounceSaver;
7270

73-
// Map from (uid, local/remote item) to the saved display index in the database.
74-
private Map<Pair<Long, LocalItem.LocalItemType>, Long> displayIndexInDatabase;
71+
private List<Pair<Long, LocalItem.LocalItemType>> deletedItems;
7572

7673
///////////////////////////////////////////////////////////////////////////
7774
// Fragment LifeCycle - Creation
@@ -89,9 +86,9 @@ public void onCreate(final Bundle savedInstanceState) {
8986
disposables = new CompositeDisposable();
9087

9188
isLoadingComplete = new AtomicBoolean();
92-
debounceSaver = new DebounceSaver(this);
89+
debounceSaver = new DebounceSaver(3000, this);
9390

94-
displayIndexInDatabase = new HashMap<>();
91+
deletedItems = new ArrayList<>();
9592
}
9693

9794
@Nullable
@@ -186,7 +183,8 @@ public void startLoading(final boolean forceLoad) {
186183
isLoadingComplete.set(false);
187184

188185
Flowable.combineLatest(localPlaylistManager.getDisplayIndexOrderedPlaylists(),
189-
remotePlaylistManager.getDisplayIndexOrderedPlaylists(), PlaylistLocalItem::merge)
186+
remotePlaylistManager.getDisplayIndexOrderedPlaylists(),
187+
PlaylistLocalItem::merge)
190188
.onBackpressureLatest()
191189
.observeOn(AndroidSchedulers.mainThread())
192190
.subscribe(getPlaylistsSubscriber());
@@ -237,7 +235,7 @@ public void onDestroy() {
237235
itemsListState = null;
238236

239237
isLoadingComplete = null;
240-
displayIndexInDatabase = null;
238+
deletedItems = null;
241239
}
242240

243241
///////////////////////////////////////////////////////////////////////////
@@ -343,17 +341,23 @@ private void deleteItem(final PlaylistLocalItem item) {
343341
}
344342
itemListAdapter.removeItem(item);
345343

346-
debounceSaver.saveChanges();
344+
if (item instanceof PlaylistMetadataEntry) {
345+
deletedItems.add(new Pair<>(item.getUid(),
346+
LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM));
347+
} else if (item instanceof PlaylistRemoteEntity) {
348+
deletedItems.add(new Pair<>(item.getUid(),
349+
LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM));
350+
}
351+
352+
debounceSaver.setHasChangesToSave();
347353
}
348354

349355
private void checkDisplayIndexModified(@NonNull final List<PlaylistLocalItem> result) {
350356
if (debounceSaver != null && debounceSaver.getIsModified()) {
351357
return;
352358
}
353359

354-
displayIndexInDatabase.clear();
355-
356-
// If the display index does not match actual index in the list, update the display index.
360+
// Check if the display index does not match the actual index in the list.
357361
// This may happen when a new list is created
358362
// or on the first run after database migration
359363
// or display index is not continuous for some reason
@@ -363,29 +367,12 @@ private void checkDisplayIndexModified(@NonNull final List<PlaylistLocalItem> re
363367
final PlaylistLocalItem item = result.get(i);
364368
if (item.getDisplayIndex() != i) {
365369
isDisplayIndexModified = true;
366-
}
367-
368-
// Updating display index in the item does not affect the value inserts into
369-
// database, which will be recalculated during the database update. Updating
370-
// display index in the item here is to determine whether it is recently modified.
371-
// Save the index read from the database.
372-
if (item instanceof PlaylistMetadataEntry) {
373-
374-
displayIndexInDatabase.put(new Pair<>(item.getUid(),
375-
LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM), item.getDisplayIndex());
376-
item.setDisplayIndex(i);
377-
378-
} else if (item instanceof PlaylistRemoteEntity) {
379-
380-
displayIndexInDatabase.put(new Pair<>(item.getUid(),
381-
LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM), item.getDisplayIndex());
382-
item.setDisplayIndex(i);
383-
370+
break;
384371
}
385372
}
386373

387374
if (debounceSaver != null && isDisplayIndexModified) {
388-
debounceSaver.saveChanges();
375+
debounceSaver.setHasChangesToSave();
389376
}
390377
}
391378

@@ -414,43 +401,28 @@ public void saveImmediate() {
414401
final LocalItem item = items.get(i);
415402

416403
if (item instanceof PlaylistMetadataEntry) {
417-
((PlaylistMetadataEntry) item).setDisplayIndex(i);
418-
419-
final Long uid = ((PlaylistMetadataEntry) item).getUid();
420-
final Pair<Long, LocalItem.LocalItemType> key = new Pair<>(uid,
421-
LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM);
422-
final Long databaseIndex = displayIndexInDatabase.remove(key);
423-
424-
// The database index should not be null because inserting new item into database
425-
// is not handled here. NullPointerException has occurred once, but I can't
426-
// reproduce it. Enhance robustness here.
427-
if (databaseIndex != null && databaseIndex != i) {
404+
if (((PlaylistMetadataEntry) item).getDisplayIndex() != i) {
405+
((PlaylistMetadataEntry) item).setDisplayIndex(i);
428406
localItemsUpdate.add((PlaylistMetadataEntry) item);
429407
}
430408
} else if (item instanceof PlaylistRemoteEntity) {
431-
((PlaylistRemoteEntity) item).setDisplayIndex(i);
432-
433-
final Long uid = ((PlaylistRemoteEntity) item).getUid();
434-
final Pair<Long, LocalItem.LocalItemType> key = new Pair<>(uid,
435-
LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM);
436-
final Long databaseIndex = displayIndexInDatabase.remove(key);
437-
438-
if (databaseIndex != null && databaseIndex != i) {
409+
if (((PlaylistRemoteEntity) item).getDisplayIndex() != i) {
410+
((PlaylistRemoteEntity) item).setDisplayIndex(i);
439411
remoteItemsUpdate.add((PlaylistRemoteEntity) item);
440412
}
441413
}
442414
}
443415

444416
// Find deleted items
445-
for (final Pair<Long, LocalItem.LocalItemType> key : displayIndexInDatabase.keySet()) {
446-
if (key.second.equals(LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM)) {
447-
localItemsDeleteUid.add(key.first);
448-
} else if (key.second.equals(LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM)) {
449-
remoteItemsDeleteUid.add(key.first);
417+
for (final Pair<Long, LocalItem.LocalItemType> item : deletedItems) {
418+
if (item.second.equals(LocalItem.LocalItemType.PLAYLIST_LOCAL_ITEM)) {
419+
localItemsDeleteUid.add(item.first);
420+
} else if (item.second.equals(LocalItem.LocalItemType.PLAYLIST_REMOTE_ITEM)) {
421+
remoteItemsDeleteUid.add(item.first);
450422
}
451423
}
452424

453-
displayIndexInDatabase.clear();
425+
deletedItems.clear();
454426

455427
// 1. Update local playlists
456428
// 2. Update remote playlists
@@ -515,7 +487,7 @@ public boolean onMove(@NonNull final RecyclerView recyclerView,
515487
final int targetIndex = target.getBindingAdapterPosition();
516488
final boolean isSwapped = itemListAdapter.swapItems(sourceIndex, targetIndex);
517489
if (isSwapped) {
518-
debounceSaver.saveChanges();
490+
debounceSaver.setHasChangesToSave();
519491
}
520492
return isSwapped;
521493
}

‎app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,7 @@ public void removeWatchedStreams(final boolean removePartiallyWatched) {
441441

442442
itemListAdapter.clearStreamItemList();
443443
itemListAdapter.addItems(notWatchedItems);
444-
debounceSaver.saveChanges();
444+
debounceSaver.setHasChangesToSave();
445445

446446

447447
if (thumbnailVideoRemoved) {
@@ -609,7 +609,7 @@ private void deleteItem(final PlaylistStreamEntry item) {
609609
}
610610

611611
setVideoCount(itemListAdapter.getItemsList().size());
612-
debounceSaver.saveChanges();
612+
debounceSaver.setHasChangesToSave();
613613
}
614614

615615
@Override
@@ -687,7 +687,7 @@ public boolean onMove(@NonNull final RecyclerView recyclerView,
687687
final int targetIndex = target.getBindingAdapterPosition();
688688
final boolean isSwapped = itemListAdapter.swapItems(sourceIndex, targetIndex);
689689
if (isSwapped) {
690-
debounceSaver.saveChanges();
690+
debounceSaver.setHasChangesToSave();
691691
}
692692
return isSwapped;
693693
}

‎app/src/main/java/org/schabi/newpipe/util/debounce/DebounceSaver.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ public Disposable getDebouncedSaver() {
7070
UserAction.SOMETHING_ELSE, "Debounced saver")));
7171
}
7272

73-
public void saveChanges() {
73+
public void setHasChangesToSave() {
7474
if (isModified == null || debouncedSaveSignal == null) {
7575
return;
7676
}

‎app/src/test/java/org/schabi/newpipe/database/playlist/PlaylistLocalItemTest.java‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -36,16 +36,6 @@ public void onlyLocalPlaylists() {
3636
assertEquals(3, mergedPlaylists.get(2).getDisplayIndex());
3737
}
3838

39-
@Test(expected = IllegalArgumentException.class)
40-
public void invalidLocalPlaylists() {
41-
final List<PlaylistMetadataEntry> localPlaylists = new ArrayList<>();
42-
final List<PlaylistRemoteEntity> remotePlaylists = new ArrayList<>();
43-
localPlaylists.add(new PlaylistMetadataEntry(1, "name1", "", 2, 1));
44-
localPlaylists.add(new PlaylistMetadataEntry(2, "name2", "", 1, 1));
45-
localPlaylists.add(new PlaylistMetadataEntry(3, "name3", "", 0, 1));
46-
PlaylistLocalItem.merge(localPlaylists, remotePlaylists);
47-
}
48-
4939
@Test
5040
public void onlyRemotePlaylists() {
5141
final List<PlaylistMetadataEntry> localPlaylists = new ArrayList<>();
@@ -65,19 +55,6 @@ public void onlyRemotePlaylists() {
6555
assertEquals(4, mergedPlaylists.get(2).getDisplayIndex());
6656
}
6757

68-
@Test(expected = IllegalArgumentException.class)
69-
public void invalidRemotePlaylists() {
70-
final List<PlaylistMetadataEntry> localPlaylists = new ArrayList<>();
71-
final List<PlaylistRemoteEntity> remotePlaylists = new ArrayList<>();
72-
remotePlaylists.add(new PlaylistRemoteEntity(
73-
1, "name1", "url1", "", "", 1, 1L));
74-
remotePlaylists.add(new PlaylistRemoteEntity(
75-
2, "name2", "url2", "", "", 3, 1L));
76-
remotePlaylists.add(new PlaylistRemoteEntity(
77-
3, "name3", "url3", "", "", 0, 1L));
78-
PlaylistLocalItem.merge(localPlaylists, remotePlaylists);
79-
}
80-
8158
@Test
8259
public void sameIndexWithDifferentName() {
8360
final List<PlaylistMetadataEntry> localPlaylists = new ArrayList<>();

0 commit comments

Comments
 (0)