diff --git a/app/src/main/java/com/httrack/android/HTTrackActivity.java b/app/src/main/java/com/httrack/android/HTTrackActivity.java index 87a786ca..34814960 100755 --- a/app/src/main/java/com/httrack/android/HTTrackActivity.java +++ b/app/src/main/java/com/httrack/android/HTTrackActivity.java @@ -123,6 +123,12 @@ public class HTTrackActivity extends FragmentActivity { protected static final int LAYOUT_MIRROR_PROGRESS = 3; protected static final int LAYOUT_FINISHED = 4; + // The options map: carried on the intent both ways, and saved in either activity's bundle. + protected static final String MAP_NAME = "com.httrack.android.map"; + // Build stamp of a bundle; another build's R.id values key that same map differently. + protected static final String VERSION_CODE_NAME = "com.httrack.android.version"; + protected static final String PANE_NAME = "com.httrack.android.pane_id"; + // Preferences protected static final String PREFS_NAME = "HTTrackPreferences"; protected static final String BASE_NAME = "BasePath"; @@ -669,6 +675,16 @@ protected boolean ensureExternalStorage() { } } + /** This build's own PackageInfo; not finding our own package is unrecoverable. **/ + protected static PackageInfo packageInfo(final Context context) { + try { + return context.getPackageManager().getPackageInfo( + context.getPackageName(), 0); + } catch (final NameNotFoundException e) { + throw new RuntimeException(e); + } + } + @Override protected void onCreate(final Bundle savedInstanceState) { Log.d(getClass().getSimpleName(), "onCreate"); @@ -686,14 +702,9 @@ protected void onCreate(final Bundle savedInstanceState) { } // Android package version code - try { - final PackageInfo info = getPackageManager().getPackageInfo( - getPackageName(), 0); - versionCode = info.versionCode; - versionName = info.versionName; - } catch (final NameNotFoundException e) { - throw new RuntimeException(e); - } + final PackageInfo info = packageInfo(this); + versionCode = info.versionCode; + versionName = info.versionName; // Compute target directory on external storage ensureExternalStorage(); @@ -2639,7 +2650,7 @@ public void onClickOptions(final View view) { // Then start new activity final Intent intent = new Intent(this, OptionsActivity.class); fillExtra(intent); - intent.putExtra("com.httrack.android.map", mapper.serialize()); + intent.putExtra(MAP_NAME, mapper.serialize()); Log.d(getClass().getSimpleName(), "map size: " + mapper.size()); startActivityForResult(intent, ACTIVITY_OPTIONS); } @@ -2678,7 +2689,7 @@ protected void onActivityResult(final int requestCode, final int resultCode, case ACTIVITY_OPTIONS: if (resultCode == Activity.RESULT_OK) { // Load modified map - loadParcelable(data.getParcelableExtra("com.httrack.android.map")); + loadParcelable(data.getParcelableExtra(MAP_NAME)); } break; case ACTIVITY_FILE_CHOOSER: @@ -2936,16 +2947,16 @@ protected void saveInstanceState(final Bundle outState) { outState.putString("com.httrack.android.sessionID", sessionID); // Version ID - outState.putInt("com.httrack.android.version", versionCode); + outState.putInt(VERSION_CODE_NAME, versionCode); // Map keys - outState.putParcelable("com.httrack.android.map", mapper.serialize()); + outState.putParcelable(MAP_NAME, mapper.serialize()); // Which project's profile the map holds, so the reload guard survives recreation. outState.putString("com.httrack.android.loadedProjectName", loadedProjectName); // Current pane - outState.putInt("com.httrack.android.pane_id", pane_id); + outState.putInt(PANE_NAME, pane_id); // Current focus id outState.putIntArray("com.httrack.android.focus_id", getCurrentFocusId()); @@ -3059,8 +3070,7 @@ protected void sendSystemNotification(final CharSequence title, /** Restore a saved instance state. **/ protected void restoreInstanceState(final Bundle savedInstanceState) { // Check version ID - final int version = savedInstanceState - .getInt("com.httrack.android.version"); + final int version = savedInstanceState.getInt(VERSION_CODE_NAME); if (version != versionCode) { Log.d(getClass().getSimpleName(), "refused bundle version " + version); return; @@ -3070,15 +3080,14 @@ protected void restoreInstanceState(final Bundle savedInstanceState) { sessionID = savedInstanceState.getString("com.httrack.android.sessionID"); // Switch pane id - final int id = savedInstanceState.getInt("com.httrack.android.pane_id"); + final int id = savedInstanceState.getInt(PANE_NAME); // Current focus final int[] focus_ids = savedInstanceState .getIntArray("com.httrack.android.focus_id"); // Load map - final Parcelable data = savedInstanceState - .getParcelable("com.httrack.android.map"); + final Parcelable data = savedInstanceState.getParcelable(MAP_NAME); // Load map if (data != null) { diff --git a/app/src/main/java/com/httrack/android/OptionsActivity.java b/app/src/main/java/com/httrack/android/OptionsActivity.java index 2d90aeec..9af1e663 100755 --- a/app/src/main/java/com/httrack/android/OptionsActivity.java +++ b/app/src/main/java/com/httrack/android/OptionsActivity.java @@ -54,7 +54,8 @@ * FragmentActivity rather than Activity: predictive back is dispatched through the AndroidX * OnBackPressedDispatcher, which a plain Activity does not have. */ -public class OptionsActivity extends FragmentActivity implements View.OnClickListener { +public class OptionsActivity extends FragmentActivity implements + View.OnClickListener, OptionsInstanceState.Screen { /* List of all tabs. */ @SuppressWarnings("unchecked") protected static Class[] tabClasses = new Class[] { @@ -84,6 +85,9 @@ public class OptionsActivity extends FragmentActivity implements View.OnClickLis // use large screen ? (tablets) protected boolean isTabletMode; + // Build this instance belongs to, stamped on the bundle it saves + protected int versionCode; + /** * The tab activit(ies) common interface. */ @@ -370,6 +374,8 @@ private void setViewMenu() { protected void onCreate(final Bundle savedInstanceState) { super.onCreate(savedInstanceState); + versionCode = HTTrackActivity.packageInfo(this).versionCode; + getOnBackPressedDispatcher().addCallback(this, backCallback); // Large screen ? Enable special tablet features in such case... @@ -396,7 +402,7 @@ protected void onCreate(final Bundle savedInstanceState) { // Pinned to Parcelable: inlined, T infers as File & Parcelable and unserialize(File) // matches just as well, which javac rejects as ambiguous. final Parcelable savedMap = - getIntent().getParcelableExtra("com.httrack.android.map"); + getIntent().getParcelableExtra(HTTrackActivity.MAP_NAME); mapper.unserialize(savedMap); Log.d(getClass().getSimpleName(), "map size: " + mapper.size()); @@ -404,6 +410,69 @@ protected void onCreate(final Bundle savedInstanceState) { setViewMenu(); } + @Override + public void flushVisibleTab() { + saveIfNeeded(); + } + + @Override + public Parcelable serializeMap() { + return mapper.serialize(); + } + + @Override + public void unserializeMap(final Parcelable map) { + mapper.unserialize(map); + } + + /** Index of CLS in tabClasses, NO_PANE when it names no tab. **/ + protected static int paneIndexOf(final Class cls) { + for (int i = 0; i < tabClasses.length; i++) { + if (tabClasses[i] == cls) { + return i; + } + } + return OptionsInstanceState.NO_PANE; + } + + /** Number of tabs, as a static so a test can pin it without an Activity. **/ + protected static int tabCount() { + return tabClasses.length; + } + + @Override + public int paneCount() { + return tabCount(); + } + + @Override + public int visiblePane() { + return paneIndexOf(activityClass); + } + + @Override + public void openPane(final int index) { + setPane(index); + } + + @Override + protected void onSaveInstanceState(final Bundle outState) { + Log.d(getClass().getSimpleName(), "onSaveInstanceState"); + super.onSaveInstanceState(outState); + OptionsInstanceState.save(this, new OptionsInstanceState.BundleStore( + outState), versionCode); + } + + @Override + protected void onRestoreInstanceState(final Bundle savedInstanceState) { + Log.d(getClass().getSimpleName(), "onRestoreInstanceState"); + super.onRestoreInstanceState(savedInstanceState); + if (!OptionsInstanceState.restore(this, + new OptionsInstanceState.BundleStore(savedInstanceState), versionCode)) { + Log.d(getClass().getSimpleName(), "refused bundle"); + } + } + /* * Map getter. */ @@ -425,7 +494,7 @@ public void finish() { // Declare result final Intent intent = new Intent(); - intent.putExtra("com.httrack.android.map", mapper.serialize()); + intent.putExtra(HTTrackActivity.MAP_NAME, mapper.serialize()); setResult(Activity.RESULT_OK, intent); super.finish(); } diff --git a/app/src/main/java/com/httrack/android/OptionsInstanceState.java b/app/src/main/java/com/httrack/android/OptionsInstanceState.java new file mode 100644 index 00000000..9fe00e0b --- /dev/null +++ b/app/src/main/java/com/httrack/android/OptionsInstanceState.java @@ -0,0 +1,131 @@ +/* +HTTrack Android Java Interface. + +HTTrack Website Copier, Offline Browser for Windows and Unix +Copyright (C) Xavier Roche and other contributors + +This program is free software; you can redistribute it and/or +modify it under the terms of the GNU General Public License +as published by the Free Software Foundation; either version 3 +of the License, or any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program; if not, write to the Free Software +Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + */ + +package com.httrack.android; + +import android.os.Bundle; +import android.os.Parcelable; + +/** + * What the options screen saves across a recreation, kept out of OptionsActivity so both + * directions run over seams: production stores into a Bundle, the tests into a plain map. + */ +final class OptionsInstanceState { + /** No tab is open: the menu is showing. **/ + static final int NO_PANE = -1; + + private OptionsInstanceState() { + } + + /** The bundle slots the state occupies. **/ + interface Store { + void putInt(final String key, final int value); + + int getInt(final String key, final int defaultValue); + + void putParcelable(final String key, final Parcelable value); + + Parcelable getParcelable(final String key); + } + + /** The options screen the state is taken from and given back to. **/ + interface Screen { + /** Flush the visible tab's widgets into the map, which is what gets saved. **/ + void flushVisibleTab(); + + Parcelable serializeMap(); + + void unserializeMap(final Parcelable map); + + /** Number of tabs, so a saved index naming none can be refused. **/ + int paneCount(); + + /** Index in tabClasses of the open tab, NO_PANE on the menu. **/ + int visiblePane(); + + void openPane(final int index); + } + + /** Store over the real thing. **/ + static final class BundleStore implements Store { + private final Bundle bundle; + + BundleStore(final Bundle bundle) { + this.bundle = bundle; + } + + @Override + public void putInt(final String key, final int value) { + bundle.putInt(key, value); + } + + @Override + public int getInt(final String key, final int defaultValue) { + return bundle.getInt(key, defaultValue); + } + + @Override + public void putParcelable(final String key, final Parcelable value) { + bundle.putParcelable(key, value); + } + + @Override + public Parcelable getParcelable(final String key) { + return bundle.getParcelable(key); + } + } + + /** Save what it takes to re-open SCREEN as the user left it. **/ + static void save(final Screen screen, final Store store, + final int versionCode) { + // Edits on the visible tab only reach the map when that tab is left. + screen.flushVisibleTab(); + + store.putInt(HTTrackActivity.VERSION_CODE_NAME, versionCode); + // The live map, as HTTrackActivity does; safe only because the restoring mapper is a new one. + store.putParcelable(HTTrackActivity.MAP_NAME, screen.serializeMap()); + store.putInt(HTTrackActivity.PANE_NAME, screen.visiblePane()); + } + + /** Restore SCREEN from STORE; false when the bundle held nothing usable. **/ + static boolean restore(final Screen screen, final Store store, + final int versionCode) { + // Another build renumbers R.id, so its map keys and pane index name other fields and tabs. + if (store.getInt(HTTrackActivity.VERSION_CODE_NAME, 0) != versionCode) { + return false; + } + + // Without a map, keep the one onCreate took from the intent. + final Parcelable map = store.getParcelable(HTTrackActivity.MAP_NAME); + if (map == null) { + return false; + } + + // The map first: opening a tab loads its fields from it. + screen.unserializeMap(map); + // A rebuild at the same versionCode may drop or reorder tabs, so the index can name none. + final int pane = store.getInt(HTTrackActivity.PANE_NAME, NO_PANE); + if (pane >= 0 && pane < screen.paneCount()) { + screen.openPane(pane); + } + return true; + } +} diff --git a/app/src/test/java/com/httrack/android/OptionsInstanceStateTest.java b/app/src/test/java/com/httrack/android/OptionsInstanceStateTest.java new file mode 100644 index 00000000..aa016fd0 --- /dev/null +++ b/app/src/test/java/com/httrack/android/OptionsInstanceStateTest.java @@ -0,0 +1,264 @@ +package com.httrack.android; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.junit.Test; + +import android.os.Parcel; +import android.os.Parcelable; + +/** + * OptionsActivity's instance state (issue #129): without it, any recreation reverted every + * unsaved option edit. Both directions run here, over the seams the activity itself uses. + */ +public class OptionsInstanceStateTest { + private static final int VERSION = 95; + + /** Stands in for the serialized map; only its identity is looked at. */ + private static class FakeMap implements Parcelable { + @Override + public int describeContents() { + return 0; + } + + @Override + public void writeToParcel(final Parcel dest, final int flags) { + } + } + + private static class FakeStore implements OptionsInstanceState.Store { + private final Map values = new HashMap(); + + @Override + public void putInt(final String key, final int value) { + values.put(key, value); + } + + // Like Bundle, answer the default when the slot holds another type: that is how a getter + // reaching for the wrong key stays silent. + @Override + public int getInt(final String key, final int defaultValue) { + final Object value = values.get(key); + return value instanceof Integer ? Integer.class.cast(value) + : defaultValue; + } + + @Override + public void putParcelable(final String key, final Parcelable value) { + values.put(key, value); + } + + @Override + public Parcelable getParcelable(final String key) { + final Object value = values.get(key); + return value instanceof Parcelable ? Parcelable.class.cast(value) : null; + } + } + + /** Screen recording what was asked of it, in order. */ + private static class FakeScreen implements OptionsInstanceState.Screen { + private final List calls = new ArrayList(); + private Parcelable map; + private int pane; + private boolean restored; + + FakeScreen(final Parcelable map, final int pane) { + this.map = map; + this.pane = pane; + } + + @Override + public void flushVisibleTab() { + calls.add("flush"); + } + + @Override + public Parcelable serializeMap() { + calls.add("serialize"); + return map; + } + + @Override + public void unserializeMap(final Parcelable map) { + calls.add("unserialize"); + this.map = map; + } + + // The real one, so a wrong count fails here instead of only on a device. + @Override + public int paneCount() { + return OptionsActivity.tabCount(); + } + + @Override + public int visiblePane() { + calls.add("visiblePane"); + return pane; + } + + @Override + public void openPane(final int index) { + calls.add("openPane"); + this.pane = index; + } + } + + /** The fresh screen SAVED's state lands in, restoring as build VERSION. */ + private static FakeScreen restore(final FakeScreen saved, final int version) { + final FakeStore store = new FakeStore(); + OptionsInstanceState.save(saved, store, VERSION); + final FakeScreen restored = new FakeScreen(null, + OptionsInstanceState.NO_PANE); + restored.restored = OptionsInstanceState.restore(restored, store, version); + return restored; + } + + /** The screen a bundle naming PANE restores onto. */ + private static FakeScreen restorePane(final int pane) { + final FakeStore store = new FakeStore(); + store.putInt(HTTrackActivity.VERSION_CODE_NAME, VERSION); + store.putParcelable(HTTrackActivity.MAP_NAME, new FakeMap()); + store.putInt(HTTrackActivity.PANE_NAME, pane); + final FakeScreen restored = new FakeScreen(null, + OptionsInstanceState.NO_PANE); + restored.restored = OptionsInstanceState.restore(restored, store, VERSION); + return restored; + } + + @Test + public void theMapAndTheOpenTabSurviveTheRoundTrip() { + for (int i = 0; i < OptionsActivity.tabClasses.length; i++) { + final Parcelable map = new FakeMap(); + final FakeScreen restored = restore(new FakeScreen(map, i), VERSION); + assertTrue(restored.restored); + assertSame(map, restored.map); + assertEquals(i, restored.pane); + } + } + + @Test + public void theMenuComesBackAsTheMenu() { + // No tab was open, so the map is restored but nothing is re-opened over the menu. + final Parcelable map = new FakeMap(); + final FakeScreen restored = restore( + new FakeScreen(map, OptionsInstanceState.NO_PANE), VERSION); + assertSame(map, restored.map); + assertFalse(restored.calls.contains("openPane")); + } + + @Test + public void aBundleFromAnotherBuildRestoresNothing() { + // R.id values are renumbered by any layout change, so that build's keys name other fields. + final FakeScreen restored = restore(new FakeScreen(new FakeMap(), 0), + VERSION + 1); + assertFalse(restored.restored); + assertNull(restored.map); + assertEquals(OptionsInstanceState.NO_PANE, restored.pane); + } + + @Test + public void aBundleWithoutAMapRestoresNothing() { + final FakeStore store = new FakeStore(); + store.putInt(HTTrackActivity.VERSION_CODE_NAME, VERSION); + store.putInt(HTTrackActivity.PANE_NAME, 0); + final FakeScreen restored = new FakeScreen(null, + OptionsInstanceState.NO_PANE); + assertFalse(OptionsInstanceState.restore(restored, store, VERSION)); + assertFalse(restored.calls.contains("openPane")); + } + + @Test + public void theVisibleTabIsFlushedBeforeTheMapIsRead() { + // A tab's widgets only reach the map when that tab is left. + final FakeScreen saved = new FakeScreen(new FakeMap(), 0); + OptionsInstanceState.save(saved, new FakeStore(), VERSION); + assertTrue(saved.calls.indexOf("flush") < saved.calls.indexOf("serialize")); + } + + @Test + public void theMapIsRestoredBeforeTheTabIsReopened() { + // Opening a tab loads its fields from the map, so a reopen first would load the stale one. + final FakeScreen restored = restore(new FakeScreen(new FakeMap(), 2), + VERSION); + assertTrue(restored.calls.indexOf("unserialize") < restored.calls + .indexOf("openPane")); + } + + @Test + public void everyTabHasItsOwnPaneIndex() { + // A duplicate would make two menu entries share one index, and reopen the wrong tab. + for (int i = 0; i < OptionsActivity.tabClasses.length; i++) { + assertEquals(OptionsActivity.tabClasses[i].getName(), i, + OptionsActivity.paneIndexOf(OptionsActivity.tabClasses[i])); + } + } + + @Test + public void whatIsNotATabIsNotAPane() { + // activityClass is null on the menu, and never a class outside the list. + assertEquals(OptionsInstanceState.NO_PANE, + OptionsActivity.paneIndexOf(null)); + assertEquals(OptionsInstanceState.NO_PANE, + OptionsActivity.paneIndexOf(OptionsActivity.class)); + } + + @Test + public void aPaneIndexNamingNoTabLeavesTheMenu() { + // An out-of-range index still passes the version guard, but must not reach openPane. + for (final int pane : new int[] { OptionsActivity.tabClasses.length, -2 }) { + final FakeScreen restored = restorePane(pane); + assertTrue("bundle refused for pane " + pane, restored.restored); + assertFalse("reopened pane " + pane, + restored.calls.contains("openPane")); + } + } + + @Test + public void theCountStopsAtTheLastPane() { + // restore() bounds the saved index with the count, and setPane throws one past the end. + final int last = OptionsActivity.tabCount() - 1; + assertTrue("the count runs past the last tab", + last < OptionsActivity.tabClasses.length); + assertEquals(last, + OptionsActivity.paneIndexOf(OptionsActivity.tabClasses[last])); + } + + @Test + public void theSavedIndexIsTheVisibleTabs() throws IOException { + // The fake screen answers visiblePane() itself, so only the source pins the real one. + assertTrue("visiblePane() does not use paneIndexOf", + body("public int visiblePane(").contains( + "return paneIndexOf(activityClass);")); + } + + @Test + public void bothLifecycleHooksAreOverridden() throws IOException { + // The Activity overrides are the one thing above no seam reaches. + assertTrue("no onSaveInstanceState", + body("protected void onSaveInstanceState(").contains( + "OptionsInstanceState.save(this,")); + assertTrue("no onRestoreInstanceState", + body("protected void onRestoreInstanceState(").contains( + "OptionsInstanceState.restore(this,")); + } + + /** Body of an OptionsActivity method, up to the closing brace at method indent. */ + private static String body(final String signature) throws IOException { + final String source = TestSources.javaSource("OptionsActivity"); + final int start = source.indexOf(signature); + assertTrue("not declared: " + signature, start >= 0); + final int end = source.indexOf("\n }", start); + assertTrue("unterminated: " + signature, end > start); + return source.substring(start, end); + } +}