From 60879351a925c1d6671ccbd45fc98c1f50f91b3d Mon Sep 17 00:00:00 2001 From: Coffeemakr Date: Sun, 8 Oct 2017 17:37:02 +0200 Subject: [PATCH 1/3] Code improvement and logging --- .../org/schabi/newpipe/util/StateSaver.java | 93 ++++++++++++++----- 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/util/StateSaver.java b/app/src/main/java/org/schabi/newpipe/util/StateSaver.java index bd268abf7..51dceddf3 100644 --- a/app/src/main/java/org/schabi/newpipe/util/StateSaver.java +++ b/app/src/main/java/org/schabi/newpipe/util/StateSaver.java @@ -21,6 +21,7 @@ package org.schabi.newpipe.util; import android.content.Context; +import android.os.Build; import android.os.Bundle; import android.os.Parcel; import android.os.Parcelable; @@ -29,6 +30,7 @@ import android.support.annotation.Nullable; import android.text.TextUtils; import android.util.Log; +import org.schabi.newpipe.BuildConfig; import org.schabi.newpipe.MainActivity; import java.io.File; @@ -110,6 +112,7 @@ public class StateSaver { /** * Try to restore the state from memory and disk, using the {@link StateSaver.WriteRead#readFrom(Queue)} from the writeRead. */ + @Nullable private static SavedState tryToRestore(@NonNull SavedState savedState, @NonNull WriteRead writeRead) { if (MainActivity.DEBUG) { Log.d(TAG, "tryToRestore() called with: savedState = [" + savedState + "], writeRead = [" + writeRead + "]"); @@ -117,7 +120,7 @@ public class StateSaver { FileInputStream fileInputStream = null; try { - Queue savedObjects = stateObjectsHolder.remove(savedState.prefixFileSaved); + Queue savedObjects = stateObjectsHolder.remove(savedState.getPrefixFileSaved()); if (savedObjects != null) { writeRead.readFrom(savedObjects); if (MainActivity.DEBUG) { @@ -126,8 +129,13 @@ public class StateSaver { return savedState; } - File file = new File(savedState.pathFileSaved); - if (!file.exists()) return null; + File file = new File(savedState.getPathFileSaved()); + if (!file.exists()) { + if(MainActivity.DEBUG) { + Log.d(TAG, "Cache file doesn't exist: " + file.getAbsolutePath()); + } + return null; + } fileInputStream = new FileInputStream(file); ObjectInputStream inputStream = new ObjectInputStream(fileInputStream); @@ -139,7 +147,7 @@ public class StateSaver { return savedState; } catch (Exception e) { - e.printStackTrace(); + Log.e(TAG, "Failed to restore state", e); } finally { if (fileInputStream != null) { try { @@ -154,10 +162,17 @@ public class StateSaver { /** * @see #tryToSave(boolean, String, String, WriteRead) */ + @Nullable public static SavedState tryToSave(boolean isChangingConfig, @Nullable SavedState savedState, Bundle outState, WriteRead writeRead) { - String currentSavedPrefix = savedState == null || TextUtils.isEmpty(savedState.prefixFileSaved) - ? System.nanoTime() - writeRead.hashCode() + "" - : savedState.prefixFileSaved; + @NonNull + String currentSavedPrefix; + if (savedState == null || TextUtils.isEmpty(savedState.getPrefixFileSaved())) { + // Generate unique prefix + currentSavedPrefix = System.nanoTime() - writeRead.hashCode() + ""; + } else { + // Reuse prefix + currentSavedPrefix = savedState.getPrefixFileSaved(); + } savedState = tryToSave(isChangingConfig, currentSavedPrefix, writeRead.generateSuffix(), writeRead); if (savedState != null) { @@ -173,22 +188,33 @@ public class StateSaver { * to the file with the name of prefixFileName + suffixFileName, in a cache folder got from the {@link #init(Context)}. *

* It checks if the file already exists and if it does, just return the path, so a good way to save is: - *

  • A fixed prefix for the file - *
  • A changing suffix + *
      + *
    • A fixed prefix for the file
      • + *
    • A changing suffix
      • + *
    + * + * @param isChangingConfig + * @param prefixFileName + * @param suffixFileName + * @param writeRead */ + @Nullable private static SavedState tryToSave(boolean isChangingConfig, final String prefixFileName, String suffixFileName, WriteRead writeRead) { if (MainActivity.DEBUG) { Log.d(TAG, "tryToSave() called with: isChangingConfig = [" + isChangingConfig + "], prefixFileName = [" + prefixFileName + "], suffixFileName = [" + suffixFileName + "], writeRead = [" + writeRead + "]"); } - Queue savedObjects = new LinkedList<>(); + LinkedList savedObjects = new LinkedList<>(); writeRead.writeTo(savedObjects); if (isChangingConfig) { if (savedObjects.size() > 0) { stateObjectsHolder.put(prefixFileName, savedObjects); return new SavedState(prefixFileName, ""); - } else return null; + } else { + if(MainActivity.DEBUG) Log.d(TAG, "Nothing to save"); + return null; + } } FileOutputStream fileOutputStream = null; @@ -197,8 +223,12 @@ public class StateSaver { if (!cacheDir.exists()) throw new RuntimeException("Cache dir does not exist > " + cacheDirPath); cacheDir = new File(cacheDir, CACHE_DIR_NAME); if (!cacheDir.exists()) { - boolean mkdirResult = cacheDir.mkdir(); - if (!mkdirResult) return null; + if(!cacheDir.mkdir()) { + if(BuildConfig.DEBUG) { + Log.e(TAG, "Failed to create cache directory " + cacheDir.getAbsolutePath()); + } + return null; + } } if (TextUtils.isEmpty(suffixFileName)) suffixFileName = ".cache"; @@ -214,7 +244,9 @@ public class StateSaver { return name.contains(prefixFileName); } }); - for (File file1 : files) file1.delete(); + for (File fileToDelete : files) { + fileToDelete.delete(); + } } fileOutputStream = new FileOutputStream(file); @@ -223,7 +255,7 @@ public class StateSaver { return new SavedState(prefixFileName, file.getAbsolutePath()); } catch (Exception e) { - e.printStackTrace(); + Log.e(TAG, "Failed to save state", e); } finally { if (fileOutputStream != null) { try { @@ -241,11 +273,11 @@ public class StateSaver { public static void onDestroy(SavedState savedState) { if (MainActivity.DEBUG) Log.d(TAG, "onDestroy() called with: savedState = [" + savedState + "]"); - if (savedState != null && !TextUtils.isEmpty(savedState.pathFileSaved)) { - stateObjectsHolder.remove(savedState.prefixFileSaved); + if (savedState != null && !TextUtils.isEmpty(savedState.getPathFileSaved())) { + stateObjectsHolder.remove(savedState.getPrefixFileSaved()); try { //noinspection ResultOfMethodCallIgnored - new File(savedState.pathFileSaved).delete(); + new File(savedState.getPathFileSaved()).delete(); } catch (Exception ignored) { } } @@ -271,9 +303,12 @@ public class StateSaver { // Inner //////////////////////////////////////////////////////////////////////////*/ + /** + * Information about the saved state on the disk + */ public static class SavedState implements Parcelable { - public String prefixFileSaved; - public String pathFileSaved; + private final String prefixFileSaved; + private final String pathFileSaved; public SavedState(String prefixFileSaved, String pathFileSaved) { this.prefixFileSaved = prefixFileSaved; @@ -287,7 +322,7 @@ public class StateSaver { @Override public String toString() { - return prefixFileSaved + " > " + pathFileSaved; + return getPrefixFileSaved() + " > " + getPathFileSaved(); } @Override @@ -313,6 +348,22 @@ public class StateSaver { return new SavedState[size]; } }; + + /** + * Get the prefix of the saved file + * @return the file prefix + */ + public String getPrefixFileSaved() { + return prefixFileSaved; + } + + /** + * Get the path to the saved file + * @return the path to the saved file + */ + public String getPathFileSaved() { + return pathFileSaved; + } } From 69302fcbd048bdd0acd46a835458c9aff24517e3 Mon Sep 17 00:00:00 2001 From: Coffeemakr Date: Sun, 8 Oct 2017 17:41:27 +0200 Subject: [PATCH 2/3] Add icepicker proguard rules --- app/proguard-rules.pro | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/proguard-rules.pro b/app/proguard-rules.pro index d38a631a2..1b2ac6835 100644 --- a/app/proguard-rules.pro +++ b/app/proguard-rules.pro @@ -24,4 +24,14 @@ -dontwarn org.mozilla.javascript.tools.** -dontwarn android.arch.util.paging.CountedDataSource --dontwarn android.arch.persistence.room.paging.LimitOffsetDataSource \ No newline at end of file +-dontwarn android.arch.persistence.room.paging.LimitOffsetDataSource + + +# Rules for icepick. Copy paste from https://github.com/frankiesardo/icepick +-dontwarn icepick.** +-keep class icepick.** { *; } +-keep class **$$Icepick { *; } +-keepclasseswithmembernames class * { + @icepick.* ; +} +-keepnames class * { @icepick.State *;} From 89b11ff71c8d6f820e221eefb6d46985af294465 Mon Sep 17 00:00:00 2001 From: Coffeemakr Date: Sun, 8 Oct 2017 21:04:37 +0200 Subject: [PATCH 3/3] Fail-fast for service id == -1 --- .../subscription/SubscriptionEntity.java | 3 ++- .../fragments/detail/VideoDetailFragment.java | 3 ++- .../fragments/list/BaseListInfoFragment.java | 3 ++- .../fragments/list/search/SearchFragment.java | 3 ++- .../java/org/schabi/newpipe/util/Constants.java | 2 ++ .../org/schabi/newpipe/util/ExtractorHelper.java | 16 ++++++++++++++++ 6 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/database/subscription/SubscriptionEntity.java b/app/src/main/java/org/schabi/newpipe/database/subscription/SubscriptionEntity.java index 567bec309..12d1764cc 100644 --- a/app/src/main/java/org/schabi/newpipe/database/subscription/SubscriptionEntity.java +++ b/app/src/main/java/org/schabi/newpipe/database/subscription/SubscriptionEntity.java @@ -7,6 +7,7 @@ import android.arch.persistence.room.Index; import android.arch.persistence.room.PrimaryKey; import org.schabi.newpipe.extractor.channel.ChannelInfoItem; +import org.schabi.newpipe.util.Constants; import static org.schabi.newpipe.database.subscription.SubscriptionEntity.SUBSCRIPTION_SERVICE_ID; import static org.schabi.newpipe.database.subscription.SubscriptionEntity.SUBSCRIPTION_TABLE; @@ -28,7 +29,7 @@ public class SubscriptionEntity { private long uid = 0; @ColumnInfo(name = SUBSCRIPTION_SERVICE_ID) - private int serviceId = -1; + private int serviceId = Constants.NO_SERVICE_ID; @ColumnInfo(name = SUBSCRIPTION_URL) private String url; diff --git a/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java index 5f954cad2..4bb0c2cca 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java @@ -65,6 +65,7 @@ import org.schabi.newpipe.player.PopupVideoPlayer; import org.schabi.newpipe.player.old.PlayVideoActivity; import org.schabi.newpipe.report.ErrorActivity; import org.schabi.newpipe.report.UserAction; +import org.schabi.newpipe.util.Constants; import org.schabi.newpipe.util.ExtractorHelper; import org.schabi.newpipe.util.InfoCache; import org.schabi.newpipe.util.ListHelper; @@ -110,7 +111,7 @@ public class VideoDetailFragment extends BaseStateFragment implement private boolean wasRelatedStreamsExpanded = false; @State - protected int serviceId = -1; + protected int serviceId = Constants.NO_SERVICE_ID; @State protected String name; @State diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListInfoFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListInfoFragment.java index 34fcaf873..4baf323ff 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListInfoFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListInfoFragment.java @@ -8,6 +8,7 @@ import android.view.View; import org.schabi.newpipe.extractor.ListExtractor; import org.schabi.newpipe.extractor.ListInfo; +import org.schabi.newpipe.util.Constants; import java.util.Queue; @@ -21,7 +22,7 @@ import io.reactivex.schedulers.Schedulers; public abstract class BaseListInfoFragment extends BaseListFragment { @State - protected int serviceId = -1; + protected int serviceId = Constants.NO_SERVICE_ID; @State protected String name; @State diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java index 93ac00207..db036859e 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java @@ -41,6 +41,7 @@ import org.schabi.newpipe.extractor.search.SearchResult; import org.schabi.newpipe.fragments.list.BaseListFragment; import org.schabi.newpipe.history.HistoryListener; import org.schabi.newpipe.report.UserAction; +import org.schabi.newpipe.util.Constants; import org.schabi.newpipe.util.ExtractorHelper; import org.schabi.newpipe.util.NavigationHelper; import org.schabi.newpipe.util.StateSaver; @@ -86,7 +87,7 @@ public class SearchFragment extends BaseListFragment searchFor(final int serviceId, final String query, final int pageNumber, final String searchLanguage, final SearchEngine.Filter filter) { + checkServiceId(serviceId); return Single.fromCallable(new Callable() { @Override public SearchResult call() throws Exception { @@ -61,6 +68,7 @@ public final class ExtractorHelper { } public static Single getMoreSearchItems(final int serviceId, final String query, final int nextPageNumber, final String searchLanguage, final SearchEngine.Filter filter) { + checkServiceId(serviceId); return searchFor(serviceId, query, nextPageNumber, searchLanguage, filter) .map(new Function() { @Override @@ -71,6 +79,7 @@ public final class ExtractorHelper { } public static Single> suggestionsFor(final int serviceId, final String query, final String searchLanguage) { + checkServiceId(serviceId); return Single.fromCallable(new Callable>() { @Override public List call() throws Exception { @@ -80,6 +89,7 @@ public final class ExtractorHelper { } public static Single getStreamInfo(final int serviceId, final String url, boolean forceLoad) { + checkServiceId(serviceId); return checkCache(forceLoad, serviceId, url, Single.fromCallable(new Callable() { @Override public StreamInfo call() throws Exception { @@ -89,6 +99,7 @@ public final class ExtractorHelper { } public static Single getChannelInfo(final int serviceId, final String url, boolean forceLoad) { + checkServiceId(serviceId); return checkCache(forceLoad, serviceId, url, Single.fromCallable(new Callable() { @Override public ChannelInfo call() throws Exception { @@ -98,6 +109,7 @@ public final class ExtractorHelper { } public static Single getMoreChannelItems(final int serviceId, final String url, final String nextStreamsUrl) { + checkServiceId(serviceId); return Single.fromCallable(new Callable() { @Override public NextItemsResult call() throws Exception { @@ -107,6 +119,7 @@ public final class ExtractorHelper { } public static Single getPlaylistInfo(final int serviceId, final String url, boolean forceLoad) { + checkServiceId(serviceId); return checkCache(forceLoad, serviceId, url, Single.fromCallable(new Callable() { @Override public PlaylistInfo call() throws Exception { @@ -116,6 +129,7 @@ public final class ExtractorHelper { } public static Single getMorePlaylistItems(final int serviceId, final String url, final String nextStreamsUrl) { + checkServiceId(serviceId); return Single.fromCallable(new Callable() { @Override public NextItemsResult call() throws Exception { @@ -133,6 +147,7 @@ public final class ExtractorHelper { * and put the results in the cache. */ private static Single checkCache(boolean forceLoad, int serviceId, String url, Single loadFromNetwork) { + checkServiceId(serviceId); loadFromNetwork = loadFromNetwork.doOnSuccess(new Consumer() { @Override public void accept(@NonNull I i) throws Exception { @@ -157,6 +172,7 @@ public final class ExtractorHelper { * Default implementation uses the {@link InfoCache} to get cached results */ public static Maybe loadFromCache(final int serviceId, final String url) { + checkServiceId(serviceId); return Maybe.defer(new Callable>() { @Override public MaybeSource call() throws Exception {