From 90cd59c86686d4b9cb4d2cc4fca625843ccb3131 Mon Sep 17 00:00:00 2001 From: Samuel Holland Date: Mon, 21 Aug 2017 21:25:31 -0500 Subject: ConfigActivity: Fix fragment state when leaving/entering app Do this by making the fragment transition functions idempotent. Signed-off-by: Jason A. Donenfeld --- .../com/wireguard/android/BaseConfigActivity.java | 6 +- .../com/wireguard/android/BaseConfigFragment.java | 2 + .../java/com/wireguard/android/ConfigActivity.java | 261 +++++++++++++-------- .../com/wireguard/android/ConfigListFragment.java | 2 +- 4 files changed, 162 insertions(+), 109 deletions(-) diff --git a/app/src/main/java/com/wireguard/android/BaseConfigActivity.java b/app/src/main/java/com/wireguard/android/BaseConfigActivity.java index 4413f313..0051d6bf 100644 --- a/app/src/main/java/com/wireguard/android/BaseConfigActivity.java +++ b/app/src/main/java/com/wireguard/android/BaseConfigActivity.java @@ -16,10 +16,6 @@ import com.wireguard.config.Config; abstract class BaseConfigActivity extends Activity { protected static final String KEY_CURRENT_CONFIG = "currentConfig"; - protected static final String TAG_DETAIL = "detail"; - protected static final String TAG_EDIT = "edit"; - protected static final String TAG_LIST = "list"; - protected static final String TAG_PLACEHOLDER = "placeholder"; private Config currentConfig; private String initialConfig; @@ -60,6 +56,8 @@ abstract class BaseConfigActivity extends Activity { } public void setCurrentConfig(final Config config) { + if (currentConfig == config) + return; currentConfig = config; onCurrentConfigChanged(currentConfig); } diff --git a/app/src/main/java/com/wireguard/android/BaseConfigFragment.java b/app/src/main/java/com/wireguard/android/BaseConfigFragment.java index 4a754b63..42912648 100644 --- a/app/src/main/java/com/wireguard/android/BaseConfigFragment.java +++ b/app/src/main/java/com/wireguard/android/BaseConfigFragment.java @@ -41,6 +41,8 @@ abstract class BaseConfigFragment extends Fragment { } public void setCurrentConfig(final Config config) { + if (currentConfig == config) + return; currentConfig = config; onCurrentConfigChanged(currentConfig); } diff --git a/app/src/main/java/com/wireguard/android/ConfigActivity.java b/app/src/main/java/com/wireguard/android/ConfigActivity.java index bd455be8..628aaeb6 100644 --- a/app/src/main/java/com/wireguard/android/ConfigActivity.java +++ b/app/src/main/java/com/wireguard/android/ConfigActivity.java @@ -16,38 +16,135 @@ import com.wireguard.config.Config; */ public class ConfigActivity extends BaseConfigActivity { - private ConfigDetailFragment detailFragment; - private ConfigEditFragment editFragment; + private static final String TAG_DETAIL = "detail"; + private static final String TAG_EDIT = "edit"; + private static final String TAG_LIST = "list"; + private final FragmentManager fm = getFragmentManager(); + private final FragmentCache fragments = new FragmentCache(fm); private boolean isEditing; + private boolean isLayoutFinished; private boolean isServiceAvailable; private boolean isSplitLayout; private boolean isStateSaved; - private ConfigListFragment listFragment; private int mainContainer; - private boolean wasUpdateSkipped; + private String visibleFragmentTag; + + /** + * Updates the fragment visible in the UI. + * Sets visibleFragmentTag. + * + * @param config The config that should be visible. + * @param tag The tag of the fragment that should be visible. + */ + private void moveToFragment(final Config config, final String tag) { + // Sanity check. + if (tag == null && config != null) + throw new IllegalArgumentException("Cannot set a config on a null fragment"); + if ((tag == null && !isSplitLayout) || (TAG_LIST.equals(tag) && isSplitLayout)) + throw new IllegalArgumentException("Requested tag " + tag + " does not match layout"); + // First tear down fragments as necessary. + if (tag == null || TAG_LIST.equals(tag) || (TAG_DETAIL.equals(tag) + && TAG_EDIT.equals(visibleFragmentTag))) { + while (visibleFragmentTag != null && !visibleFragmentTag.equals(tag) && + fm.getBackStackEntryCount() > 0) { + final Fragment removedFragment = fm.findFragmentById(mainContainer); + // The fragment *must* be removed first, or it will stay attached to the layout! + fm.beginTransaction().remove(removedFragment).commit(); + fm.popBackStackImmediate(); + // Recompute the visible fragment. + if (TAG_EDIT.equals(visibleFragmentTag)) + visibleFragmentTag = TAG_DETAIL; + else if (!isSplitLayout && TAG_DETAIL.equals(visibleFragmentTag)) + visibleFragmentTag = TAG_LIST; + else + throw new IllegalStateException(); + } + } + // Now build up intermediate entries in the back stack as necessary. + if (TAG_EDIT.equals(tag) && !TAG_DETAIL.equals(visibleFragmentTag)) + moveToFragment(config, TAG_DETAIL); + // Finally, set the main container's content to the new top-level fragment. + if (tag == null) { + if (visibleFragmentTag != null) { + final BaseConfigFragment fragment = fragments.get(visibleFragmentTag); + fm.beginTransaction().remove(fragment).commit(); + fm.executePendingTransactions(); + visibleFragmentTag = null; + } + } else if (!TAG_LIST.equals(tag)) { + final BaseConfigFragment fragment = fragments.get(tag); + if (!tag.equals(visibleFragmentTag)) { + final FragmentTransaction transaction = fm.beginTransaction(); + if (TAG_EDIT.equals(tag) || (!isSplitLayout && TAG_DETAIL.equals(tag))) { + transaction.addToBackStack(null); + transaction.setTransition(FragmentTransaction.TRANSIT_FRAGMENT_FADE); + } + transaction.replace(mainContainer, fragment, tag).commit(); + visibleFragmentTag = tag; + } + if (fragment.getCurrentConfig() != config) + fragment.setCurrentConfig(config); + } + } + + /** + * Transition the state machine to the desired state, if possible. + * Sets currentConfig and isEditing. + * + * @param config The desired config to show in the UI. + * @param shouldBeEditing Whether or not the config should be in the editing state. + */ + private void moveToState(final Config config, final boolean shouldBeEditing) { + // Update the saved state. + setCurrentConfig(config); + isEditing = shouldBeEditing; + // Avoid performing fragment transactions when the app is not fully initialized. + if (!isLayoutFinished || !isServiceAvailable || isStateSaved) + return; + // Ensure the list is present in the master pane. It will be restored on activity restarts! + final BaseConfigFragment listFragment = fragments.get(TAG_LIST); + if (fm.findFragmentById(R.id.master_fragment) == null) { + fm.beginTransaction().add(R.id.master_fragment, listFragment, TAG_LIST).commit(); + fm.executePendingTransactions(); + } + // In the single-pane layout, the main container starts holding the list fragment. + if (!isSplitLayout && visibleFragmentTag == null) + visibleFragmentTag = TAG_LIST; + // Forward any config changes to the list (they may have come from the intent or editing). + listFragment.setCurrentConfig(config); + // Ensure the correct main fragment is visible, adjusting the back stack as necessary. + moveToFragment(config, shouldBeEditing ? TAG_EDIT : + (config != null ? TAG_DETAIL : (isSplitLayout ? null : TAG_LIST))); + // Show the current config as the title if the list of configurations is not visible. + setTitle(!isSplitLayout && config != null ? config.getName() : getString(R.string.app_name)); + // Show or hide the action bar back button if the back stack is not empty. + if (getActionBar() != null) { + getActionBar().setDisplayHomeAsUpEnabled(config != null && + (!isSplitLayout || shouldBeEditing)); + } + } @Override public void onBackPressed() { super.onBackPressed(); - if ((isEditing && isSplitLayout) || (!isEditing && !isSplitLayout)) { - if (getActionBar() != null) - getActionBar().setDisplayHomeAsUpEnabled(false); - } - // Ensure the current config is cleared when going back to the single-pane-layout list. + // The visible fragment is now the one that was on top of the back stack, if there was one. if (isEditing) - isEditing = false; - else - setCurrentConfig(null); + visibleFragmentTag = TAG_DETAIL; + else if (!isSplitLayout && TAG_DETAIL.equals(visibleFragmentTag)) + visibleFragmentTag = TAG_LIST; + // If the user went back from the detail screen to the list, clear the current config. + moveToState(isEditing ? getCurrentConfig() : null, false); } @Override public void onCreate(final Bundle savedInstanceState) { + super.onCreate(savedInstanceState); setContentView(R.layout.config_activity); isSplitLayout = findViewById(R.id.detail_fragment) != null; mainContainer = isSplitLayout ? R.id.detail_fragment : R.id.master_fragment; - Log.d(getClass().getSimpleName(), "onCreate isSplitLayout=" + isSplitLayout); - super.onCreate(savedInstanceState); + isLayoutFinished = true; + moveToState(getCurrentConfig(), isEditing); } @Override @@ -58,52 +155,28 @@ public class ConfigActivity extends BaseConfigActivity { @Override protected void onCurrentConfigChanged(final Config config) { - Log.d(getClass().getSimpleName(), "onCurrentConfigChanged config=" + (config != null ? - config.getName() : null) + " fragment=" + fm.findFragmentById(mainContainer) + - (!isServiceAvailable || isStateSaved ? " SKIPPING" : "")); - // Avoid performing fragment transactions when it would be illegal or the service is null. - if (!isServiceAvailable || isStateSaved) { - // Signal that updates need to be performed once the activity is resumed. - wasUpdateSkipped = true; - return; - } else { - // Now that an update is being performed, reset the flag. - wasUpdateSkipped = false; - } - // If the config change came from the intent or ConfigEditFragment, forward it to the list. - // listFragment is guaranteed not to be null at this point by onServiceAvailable(). - if (listFragment.getCurrentConfig() != config) - listFragment.setCurrentConfig(config); - // Update the activity's title if the list of configurations is not visible. - if (!isSplitLayout) - setTitle(config != null ? config.getName() : getString(R.string.app_name)); - // Update the fragment in the main container. - if (isEditing) - onBackPressed(); - if (config != null) { - final boolean shouldPush = !isSplitLayout && - fm.findFragmentById(mainContainer) instanceof ConfigListFragment; - switchToFragment(mainContainer, TAG_DETAIL, shouldPush); - } else if (isSplitLayout) { - switchToFragment(mainContainer, TAG_PLACEHOLDER, false); - } + Log.d(getClass().getSimpleName(), "onCurrentConfigChanged: config=" + + (config != null ? config.getName() : null)); + // Abandon editing a config when the current config changes. + moveToState(config, false); } @Override public boolean onOptionsItemSelected(final MenuItem item) { switch (item.getItemId()) { case android.R.id.home: + // The back arrow in the action bar should act the same as the back button. onBackPressed(); return true; case R.id.menu_action_add: startActivity(new Intent(this, ConfigAddActivity.class)); return true; case R.id.menu_action_edit: - isEditing = true; - switchToFragment(mainContainer, TAG_EDIT, true); + // Try to make the editing fragment visible. + moveToState(getCurrentConfig(), true); return true; case R.id.menu_action_save: - // This menu item is handled by the current fragment. + // This menu item is handled by the editing fragment. return false; case R.id.menu_settings: startActivity(new Intent(this, SettingsActivity.class)); @@ -116,26 +189,17 @@ public class ConfigActivity extends BaseConfigActivity { @Override public void onPostResume() { super.onPostResume(); - final boolean wasStateSaved = isStateSaved; + // Allow changes to fragments. isStateSaved = false; - if (wasStateSaved || wasUpdateSkipped) - onCurrentConfigChanged(getCurrentConfig()); + moveToState(getCurrentConfig(), isEditing); } @Override public void onSaveInstanceState(final Bundle outState) { // We cannot save fragments that might switch between containers if the layout changes. - if (fm.getBackStackEntryCount() > 0) { - final int bottomEntryId = fm.getBackStackEntryAt(0).getId(); - fm.popBackStackImmediate(bottomEntryId, FragmentManager.POP_BACK_STACK_INCLUSIVE); - } - if (isSplitLayout) { - final Fragment oldFragment = fm.findFragmentById(mainContainer); - if (oldFragment != null) { - fm.beginTransaction().remove(oldFragment).commit(); - fm.executePendingTransactions(); - } - } + if (isLayoutFinished && isServiceAvailable && !isStateSaved) + moveToFragment(null, isSplitLayout ? null : TAG_LIST); + // Prevent further changes to fragments. isStateSaved = true; super.onSaveInstanceState(outState); } @@ -143,55 +207,44 @@ public class ConfigActivity extends BaseConfigActivity { @Override protected void onServiceAvailable() { super.onServiceAvailable(); + // Allow creating fragments. isServiceAvailable = true; - // Create the initial fragment set. - final Fragment masterFragment = fm.findFragmentById(R.id.master_fragment); - if (masterFragment instanceof ConfigListFragment) - listFragment = (ConfigListFragment) masterFragment; - else - switchToFragment(R.id.master_fragment, TAG_LIST, false); - // This must run even if no update was skipped, so the restored config gets applied. - onCurrentConfigChanged(getCurrentConfig()); + moveToState(getCurrentConfig(), isEditing); } - private void switchToFragment(final int container, final String tag, final boolean push) { - if (tag.equals(TAG_PLACEHOLDER)) { - final Fragment oldFragment = fm.findFragmentById(container); - if (oldFragment != null) - fm.beginTransaction().remove(oldFragment).commit(); - return; - } - final BaseConfigFragment fragment; - switch (tag) { - case TAG_DETAIL: - if (detailFragment == null) - detailFragment = new ConfigDetailFragment(); - fragment = detailFragment; - break; - case TAG_EDIT: - if (editFragment == null) - editFragment = new ConfigEditFragment(); - fragment = editFragment; - break; - case TAG_LIST: - if (listFragment == null) - listFragment = new ConfigListFragment(); - fragment = listFragment; - break; - default: - throw new IllegalArgumentException(); + private static class FragmentCache { + private ConfigDetailFragment detailFragment; + private ConfigEditFragment editFragment; + private final FragmentManager fm; + private ConfigListFragment listFragment; + + private FragmentCache(final FragmentManager fm) { + this.fm = fm; } - if (fragment.getCurrentConfig() != getCurrentConfig()) - fragment.setCurrentConfig(getCurrentConfig()); - if (fm.findFragmentById(container) != fragment) { - final FragmentTransaction transaction = fm.beginTransaction(); - if (push) { - transaction.addToBackStack(null); - transaction.setTransition(FragmentTransaction.TRANSIT_FRAGMENT_FADE); - if (getActionBar() != null && (!isSplitLayout || isEditing)) - getActionBar().setDisplayHomeAsUpEnabled(true); + + private BaseConfigFragment get(final String tag) { + switch (tag) { + case TAG_DETAIL: + if (detailFragment == null) + detailFragment = (ConfigDetailFragment) fm.findFragmentByTag(tag); + if (detailFragment == null) + detailFragment = new ConfigDetailFragment(); + return detailFragment; + case TAG_EDIT: + if (editFragment == null) + editFragment = (ConfigEditFragment) fm.findFragmentByTag(tag); + if (editFragment == null) + editFragment = new ConfigEditFragment(); + return editFragment; + case TAG_LIST: + if (listFragment == null) + listFragment = (ConfigListFragment) fm.findFragmentByTag(tag); + if (listFragment == null) + listFragment = new ConfigListFragment(); + return listFragment; + default: + throw new IllegalArgumentException(); } - transaction.replace(container, fragment, null).commit(); } } } diff --git a/app/src/main/java/com/wireguard/android/ConfigListFragment.java b/app/src/main/java/com/wireguard/android/ConfigListFragment.java index bb146755..c086e40d 100644 --- a/app/src/main/java/com/wireguard/android/ConfigListFragment.java +++ b/app/src/main/java/com/wireguard/android/ConfigListFragment.java @@ -75,7 +75,7 @@ public class ConfigListFragment extends BaseConfigFragment { Log.d(getClass().getSimpleName(), "onCurrentConfigChanged config=" + (config != null ? config.getName() : null)); final BaseConfigActivity activity = ((BaseConfigActivity) getActivity()); - if (activity != null && activity.getCurrentConfig() != config) + if (activity != null) activity.setCurrentConfig(config); if (listView != null) setConfigChecked(config); -- cgit v1.2.3-59-g8ed1b