TunnelEditorFragment: rewrite and simplify

This should remove some null pointer dereferences and overall make the
thing more robust.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

app/src/main/java/com/wireguard/android/fragment/TunnelEditorFragment.java

@@ -35,67 +35,52 @@ public class TunnelEditorFragment extends BaseFragment {
     private static final String TAG = "WireGuard/" + TunnelEditorFragment.class.getSimpleName();
 
     private TunnelEditorFragmentBinding binding;
-    private boolean isViewStateRestored;
+    private Tunnel tunnel;
-    private Config localConfig = new Config();
-    private Tunnel localTunnel;
-    private String originalName;
 
-    private static <T extends Parcelable> T copyParcelable(final T original) {
+    private void onConfigLoaded(final String name, final Config config) {
-        if (original == null)
+        binding.setConfig(new Config.Observable(config, name));
-            return null;
-        final Parcel parcel = Parcel.obtain();
-        parcel.writeParcelable(original, 0);
-        parcel.setDataPosition(0);
-        final T copy = parcel.readParcelable(original.getClass().getClassLoader());
-        parcel.recycle();
-        return copy;
-    }
-
-    private void onConfigLoaded(final Config config) {
-        localConfig = copyParcelable(config);
-        if (binding != null && isViewStateRestored)
-            binding.setConfig(new Config.Observable(localConfig, originalName));
-    }
-
-    private void onConfigSaved(@SuppressWarnings("unused") final Config config,
-                               final Throwable throwable) {
-        final String message;
-        if (throwable == null) {
-            message = getString(R.string.config_save_success, localTunnel.getName());
-            Log.d(TAG, message);
-            onFinished();
-        } else {
-            final String error = ExceptionLoggers.unwrap(throwable).getMessage();
-            message = getString(R.string.config_save_error, localTunnel.getName(), error);
-            Log.e(TAG, message, throwable);
-            if (binding != null) {
-                final CoordinatorLayout container = binding.mainContainer;
-                Snackbar.make(container, message, Snackbar.LENGTH_LONG).show();
-            }
-        }
     }
 
     @Override
     public void onCreate(final Bundle savedInstanceState) {
         super.onCreate(savedInstanceState);
-        if (savedInstanceState != null) {
-            localConfig = savedInstanceState.getParcelable(KEY_LOCAL_CONFIG);
-            originalName = savedInstanceState.getString(KEY_ORIGINAL_NAME);
-        }
-        // Erase the remains of creating or editing a different tunnel.
-        if (getSelectedTunnel() != null && !getSelectedTunnel().getName().equals(originalName)) {
-            // The config must be loaded asynchronously since it's not an observable property.
-            localConfig = null;
-            originalName = getSelectedTunnel().getName();
-            getSelectedTunnel().getConfigAsync().thenAccept(this::onConfigLoaded);
-        } else if (getSelectedTunnel() == null && originalName != null) {
-            localConfig = new Config();
-            originalName = null;
-        }
-        localTunnel = getSelectedTunnel();
         setHasOptionsMenu(true);
     }
 
+    @Override
+    public void onSelectedTunnelChanged(final Tunnel oldTunnel, final Tunnel newTunnel) {
+        tunnel = newTunnel;
+        if (binding == null)
+            return;
+        binding.setConfig(new Config.Observable(null, null));
+        if (tunnel != null)
+            tunnel.getConfigAsync().thenAccept(a -> onConfigLoaded(tunnel.getName(), a));
+    }
+
+    @Override
+    public void onSaveInstanceState(@NonNull final Bundle outState) {
+        outState.putParcelable(KEY_LOCAL_CONFIG, binding.getConfig());
+        outState.putString(KEY_ORIGINAL_NAME, tunnel == null ? null : tunnel.getName());
+        super.onSaveInstanceState(outState);
+    }
+
+    @Override
+    public void onViewStateRestored(final Bundle savedInstanceState) {
+        if (savedInstanceState == null) {
+            onSelectedTunnelChanged(null, getSelectedTunnel());
+        } else {
+            tunnel = getSelectedTunnel();
+            Config.Observable config = savedInstanceState.getParcelable(KEY_LOCAL_CONFIG);
+            String originalName = savedInstanceState.getString(KEY_ORIGINAL_NAME);
+            if (tunnel != null && !tunnel.getName().equals(originalName))
+                onSelectedTunnelChanged(null, tunnel);
+            else
+                binding.setConfig(config);
+        }
+
+        super.onViewStateRestored(savedInstanceState);
+    }
+
     @Override
     public void onCreateOptionsMenu(final Menu menu, final MenuInflater inflater) {
         inflater.inflate(R.menu.config_editor, menu);
@@ -131,7 +116,7 @@ public class TunnelEditorFragment extends BaseFragment {
         // Tell the activity to finish itself or go back to the detail view.
         getActivity().runOnUiThread(() -> {
             // The selected tunnel has to actually change, but we have to remember this one.
-            final Tunnel savedTunnel = localTunnel;
+            final Tunnel savedTunnel = tunnel;
             if (savedTunnel == getSelectedTunnel())
                 setSelectedTunnel(null);
             setSelectedTunnel(savedTunnel);
@@ -142,32 +127,31 @@ public class TunnelEditorFragment extends BaseFragment {
     public boolean onOptionsItemSelected(final MenuItem item) {
         switch (item.getItemId()) {
             case R.id.menu_action_save:
-                final Tunnel selectedTunnel = getSelectedTunnel();
+                Config newConfig = new Config();
-                if (localConfig != null) {
                 try {
-                        binding.getConfig().commitData(localConfig);
+                    binding.getConfig().commitData(newConfig);
                 } catch (Exception e) {
                     final String error = ExceptionLoggers.unwrap(e).getMessage();
-                        final String message = getString(R.string.config_save_error, localTunnel.getName(), error);
+                    final String tunnelName = tunnel == null ? binding.getConfig().getName() : tunnel.getName();
+                    final String message = getString(R.string.config_save_error, tunnelName, error);
                     Log.e(TAG, message, e);
                     final CoordinatorLayout container = binding.mainContainer;
                     Snackbar.make(container, error, Snackbar.LENGTH_LONG).show();
                     return false;
                 }
-                }
+                if (tunnel == null) {
-                if (selectedTunnel == null) {
                     Log.d(TAG, "Attempting to create new tunnel " + binding.getConfig().getName());
                     final TunnelManager manager = Application.getComponent().getTunnelManager();
-                    manager.create(binding.getConfig().getName(), localConfig)
+                    manager.create(binding.getConfig().getName(), newConfig)
                             .whenComplete(this::onTunnelCreated);
-                } else if (!selectedTunnel.getName().equals(binding.getConfig().getName())) {
+                } else if (!tunnel.getName().equals(binding.getConfig().getName())) {
                     Log.d(TAG, "Attempting to rename tunnel to " + binding.getConfig().getName());
-                    selectedTunnel.setName(binding.getConfig().getName())
+                    tunnel.setName(binding.getConfig().getName())
-                            .whenComplete(this::onTunnelRenamed);
+                            .whenComplete((a, b) -> onTunnelRenamed(tunnel, newConfig, a, b));
-                } else if (localConfig != null) {
+                } else {
-                    Log.d(TAG, "Attempting to save config of " + selectedTunnel.getName());
+                    Log.d(TAG, "Attempting to save config of " + tunnel.getName());
-                    selectedTunnel.setConfig(localConfig)
+                    tunnel.setConfig(newConfig)
-                            .whenComplete(this::onConfigSaved);
+                            .whenComplete((a, b) -> onConfigSaved(tunnel, a, b));
                 }
                 return true;
             default:
@@ -175,36 +159,30 @@ public class TunnelEditorFragment extends BaseFragment {
         }
     }
 
-    @Override
+    private void onConfigSaved(final Tunnel savedTunnel, final Config config,
-    public void onSaveInstanceState(@NonNull final Bundle outState) {
+                               final Throwable throwable) {
-        outState.putParcelable(KEY_LOCAL_CONFIG, localConfig);
-        outState.putString(KEY_ORIGINAL_NAME, originalName);
-        super.onSaveInstanceState(outState);
-    }
-
-    @Override
-    public void onSelectedTunnelChanged(final Tunnel oldTunnel, final Tunnel newTunnel) {
-        // Erase the remains of creating or editing a different tunnel.
-        if (newTunnel != null) {
-            // The config must be loaded asynchronously since it's not an observable property.
-            localConfig = null;
-            originalName = newTunnel.getName();
-            newTunnel.getConfigAsync().thenAccept(this::onConfigLoaded);
-        } else {
-            localConfig = new Config();
-            if (binding != null && isViewStateRestored)
-                binding.setConfig(new Config.Observable(localConfig, ""));
-            originalName = null;
-        }
-        localTunnel = newTunnel;
-    }
-
-    private void onTunnelCreated(final Tunnel tunnel, final Throwable throwable) {
         final String message;
         if (throwable == null) {
+            message = getString(R.string.config_save_success, savedTunnel.getName());
+            Log.d(TAG, message);
+            onFinished();
+        } else {
+            final String error = ExceptionLoggers.unwrap(throwable).getMessage();
+            message = getString(R.string.config_save_error, savedTunnel.getName(), error);
+            Log.e(TAG, message, throwable);
+            if (binding != null) {
+                final CoordinatorLayout container = binding.mainContainer;
+                Snackbar.make(container, message, Snackbar.LENGTH_LONG).show();
+            }
+        }
+    }
+
+    private void onTunnelCreated(final Tunnel newTunnel, final Throwable throwable) {
+        final String message;
+        if (throwable == null) {
+            tunnel = newTunnel;
             message = getString(R.string.tunnel_create_success, tunnel.getName());
             Log.d(TAG, message);
-            localTunnel = tunnel;
             onFinished();
         } else {
             final String error = ExceptionLoggers.unwrap(throwable).getMessage();
@@ -217,14 +195,15 @@ public class TunnelEditorFragment extends BaseFragment {
         }
     }
 
-    private void onTunnelRenamed(final String name, final Throwable throwable) {
+    private void onTunnelRenamed(final Tunnel renamedTunnel, final Config newConfig,
+                                 final String name, final Throwable throwable) {
         final String message;
         if (throwable == null) {
-            message = getString(R.string.tunnel_rename_success, localTunnel.getName(), name);
+            message = getString(R.string.tunnel_rename_success, renamedTunnel.getName());
             Log.d(TAG, message);
             // Now save the rest of configuration changes.
-            Log.d(TAG, "Attempting to save config of renamed tunnel " + localTunnel.getName());
+            Log.d(TAG, "Attempting to save config of renamed tunnel " + tunnel.getName());
-            localTunnel.setConfig(localConfig).whenComplete(this::onConfigSaved);
+            renamedTunnel.setConfig(newConfig).whenComplete((a, b) -> onConfigSaved(renamedTunnel, a, b));
         } else {
             final String error = ExceptionLoggers.unwrap(throwable).getMessage();
             message = getString(R.string.tunnel_rename_error, error);
@@ -235,13 +214,4 @@ public class TunnelEditorFragment extends BaseFragment {
             }
         }
     }
-
-    @Override
-    public void onViewStateRestored(final Bundle savedInstanceState) {
-        super.onViewStateRestored(savedInstanceState);
-        if (localConfig == null)
-            localConfig = new Config();
-        binding.setConfig(new Config.Observable(localConfig, originalName));
-        isViewStateRestored = true;
-    }
 }

app/src/main/java/com/wireguard/android/fragment/TunnelListFragment.java

@@ -25,6 +25,7 @@ import android.widget.AbsListView.MultiChoiceModeListener;
 import android.widget.AdapterView;
 import android.widget.AdapterView.OnItemClickListener;
 import android.widget.AdapterView.OnItemLongClickListener;
+import android.widget.TextView;
 
 import com.commonsware.cwac.crossport.design.widget.CoordinatorLayout;
 import com.commonsware.cwac.crossport.design.widget.Snackbar;

app/src/main/java/com/wireguard/config/Config.java

@@ -21,36 +21,25 @@ import java.util.List;
  * Represents a wg-quick configuration file, its name, and its connection state.
  */
 
-public class Config implements Parcelable {
+public class Config {
-    public static final Creator<Config> CREATOR = new Creator<Config>() {
+    public static class Observable extends BaseObservable implements Parcelable {
-        @Override
-        public Config createFromParcel(final Parcel in) {
-            return new Config(in);
-        }
-
-        @Override
-        public Config[] newArray(final int size) {
-            return new Config[size];
-        }
-    };
-
-    public static class Observable extends BaseObservable {
         private String name;
         private Interface.Observable observableInterface;
         private ObservableList<Peer.Observable> observablePeers;
 
-
         public Observable(Config parent, String name) {
             this.name = name;
             loadData(parent);
         }
 
         public void loadData(Config parent) {
-            this.observableInterface = new Interface.Observable(parent.interfaceSection);
+            this.observableInterface = new Interface.Observable(parent == null ? null : parent.interfaceSection);
             this.observablePeers = new ObservableArrayList<>();
+            if (parent != null) {
                 for (Peer peer : parent.getPeers())
                     this.observablePeers.add(new Peer.Observable(peer));
             }
+        }
 
         public void commitData(Config parent) {
             this.observableInterface.commitData(parent.interfaceSection);
@@ -66,7 +55,7 @@ public class Config implements Parcelable {
 
         @Bindable
         public String getName() {
-            return name;
+            return name == null ? "" : name;
         }
 
         public void setName(String name) {
@@ -83,20 +72,43 @@ public class Config implements Parcelable {
         public ObservableList<Peer.Observable> getPeers() {
             return observablePeers;
         }
+
+
+        public static final Creator<Observable> CREATOR = new Creator<Observable>() {
+            @Override
+            public Observable createFromParcel(final Parcel in) {
+                return new Observable(in);
             }
 
-    private final Interface interfaceSection;
+            @Override
+            public Observable[] newArray(final int size) {
+                return new Observable[size];
+            }
+        };
+
+        @Override
+        public int describeContents() {
+            return 0;
+        }
+
+        @Override
+        public void writeToParcel(final Parcel dest, final int flags) {
+            dest.writeString(name);
+            dest.writeParcelable(observableInterface, flags);
+            dest.writeTypedList(observablePeers);
+        }
+
+        private Observable(final Parcel in) {
+            name = in.readString();
+            observableInterface = in.readParcelable(Interface.Observable.class.getClassLoader());
+            observablePeers = new ObservableArrayList<>();
+            in.readTypedList(observablePeers, Peer.Observable.CREATOR);
+        }
+    }
+
+    private final Interface interfaceSection = new Interface();
     private List<Peer> peers = new ArrayList<>();
 
-    public Config() {
-        interfaceSection = new Interface();
-    }
-
-    private Config(final Parcel in) {
-        interfaceSection = in.readParcelable(Interface.class.getClassLoader());
-        in.readTypedList(peers, Peer.CREATOR);
-    }
-
     public static Config from(final InputStream stream) throws IOException {
         return from(new BufferedReader(new InputStreamReader(stream, StandardCharsets.UTF_8)));
     }
@@ -130,11 +142,6 @@ public class Config implements Parcelable {
         return config;
     }
 
-    @Override
-    public int describeContents() {
-        return 0;
-    }
-
     public Interface getInterface() {
         return interfaceSection;
     }
@@ -150,10 +157,4 @@ public class Config implements Parcelable {
             sb.append('\n').append(peer);
         return sb.toString();
     }
-
-    @Override
-    public void writeToParcel(final Parcel dest, final int flags) {
-        dest.writeParcelable(interfaceSection, flags);
-        dest.writeTypedList(peers);
-    }
 }

app/src/main/java/com/wireguard/config/IPCidr.java

@@ -1,35 +1,15 @@
 package com.wireguard.config;
 
-import android.os.Parcel;
-import android.os.Parcelable;
-
 import java.net.Inet4Address;
 import java.net.Inet6Address;
 import java.net.InetAddress;
 import java.util.Locale;
 
-public class IPCidr implements Parcelable {
+public class IPCidr {
     private InetAddress address;
     private int cidr;
 
-
+    public IPCidr(String in) {
-    public static final Parcelable.Creator<IPCidr> CREATOR = new Parcelable.Creator<IPCidr>() {
-        @Override
-        public IPCidr createFromParcel(final Parcel in) {
-            return new IPCidr(in);
-        }
-
-        @Override
-        public IPCidr[] newArray(final int size) {
-            return new IPCidr[size];
-        }
-    };
-
-    IPCidr(String in) {
-        parse(in);
-    }
-
-    private void parse(String in) {
         cidr = -1;
         int slash = in.lastIndexOf('/');
         if (slash != -1 && slash < in.length() - 1) {
@@ -58,22 +38,4 @@ public class IPCidr implements Parcelable {
     public String toString() {
         return String.format(Locale.getDefault(), "%s/%d", address.getHostAddress(), cidr);
     }
-
-    @Override
-    public void writeToParcel(final Parcel dest, final int flags) {
-        dest.writeString(this.toString());
-    }
-
-    @Override
-    public int describeContents() {
-        return 0;
-    }
-
-    private IPCidr(final Parcel in) {
-        try {
-            parse(in.readString());
-        } catch (Exception ignored) {
-        }
-    }
-
 }

app/src/main/java/com/wireguard/config/Interface.java

@@ -6,7 +6,6 @@ import android.os.Parcel;
 import android.os.Parcelable;
 
 import com.wireguard.android.BR;
-import com.wireguard.crypto.KeyEncoding;
 import com.wireguard.crypto.Keypair;
 
 import java.net.InetAddress;
@@ -17,20 +16,8 @@ import java.util.List;
  * Represents the configuration for a WireGuard interface (an [Interface] block).
  */
 
-public class Interface implements Parcelable {
+public class Interface {
-    public static final Creator<Interface> CREATOR = new Creator<Interface>() {
+    public static class Observable extends BaseObservable implements Parcelable {
-        @Override
-        public Interface createFromParcel(final Parcel in) {
-            return new Interface(in);
-        }
-
-        @Override
-        public Interface[] newArray(final int size) {
-            return new Interface[size];
-        }
-    };
-
-    public static class Observable extends BaseObservable {
         private String addresses;
         private String dnses;
         private String publicKey;
@@ -39,6 +26,7 @@ public class Interface implements Parcelable {
         private String mtu;
 
         public Observable(Interface parent) {
+            if (parent != null)
                 loadData(parent);
         }
 
@@ -131,6 +119,43 @@ public class Interface implements Parcelable {
             this.mtu = mtu;
             notifyPropertyChanged(BR.mtu);
         }
+
+
+        public static final Creator<Observable> CREATOR = new Creator<Observable>() {
+            @Override
+            public Observable createFromParcel(final Parcel in) {
+                return new Observable(in);
+            }
+
+            @Override
+            public Observable[] newArray(final int size) {
+                return new Observable[size];
+            }
+        };
+
+        @Override
+        public int describeContents() {
+            return 0;
+        }
+
+        @Override
+        public void writeToParcel(final Parcel dest, final int flags) {
+            dest.writeString(addresses);
+            dest.writeString(dnses);
+            dest.writeString(publicKey);
+            dest.writeString(privateKey);
+            dest.writeString(listenPort);
+            dest.writeString(mtu);
+        }
+
+        private Observable(final Parcel in) {
+            addresses = in.readString();
+            dnses = in.readString();
+            publicKey = in.readString();
+            privateKey = in.readString();
+            listenPort = in.readString();
+            mtu = in.readString();
+        }
     }
 
     private List<IPCidr> addressList;
@@ -144,26 +169,6 @@ public class Interface implements Parcelable {
         dnsList = new LinkedList<>();
     }
 
-    private Interface(final Parcel in) {
-        addressList = in.createTypedArrayList(IPCidr.CREATOR);
-        int dnsItems = in.readInt();
-        dnsList = new LinkedList<>();
-        for (int i = 0; i < dnsItems; ++i) {
-            try {
-                dnsList.add(InetAddress.getByAddress(in.createByteArray()));
-            } catch (Exception ignored) {
-            }
-        }
-        listenPort = in.readInt();
-        mtu = in.readInt();
-        setPrivateKey(in.readString());
-    }
-
-    @Override
-    public int describeContents() {
-        return 0;
-    }
-
     private String getAddressString() {
         if (addressList.isEmpty())
             return null;
@@ -313,15 +318,4 @@ public class Interface implements Parcelable {
             sb.append(Attribute.PRIVATE_KEY.composeWith(keypair.getPrivateKey()));
         return sb.toString();
     }
-
-    @Override
-    public void writeToParcel(final Parcel dest, final int flags) {
-        dest.writeTypedList(addressList);
-        dest.writeInt(dnsList.size());
-        for (final InetAddress addr : dnsList)
-            dest.writeByteArray(addr.getAddress());
-        dest.writeInt(listenPort);
-        dest.writeInt(mtu);
-        dest.writeString(keypair == null ? "" : keypair.getPrivateKey());
-    }
 }

app/src/main/java/com/wireguard/config/Peer.java

@@ -21,27 +21,14 @@ import java.util.Locale;
  * Represents the configuration for a WireGuard peer (a [Peer] block).
  */
 
-public class Peer implements Parcelable {
+public class Peer {
-    public static final Creator<Peer> CREATOR = new Creator<Peer>() {
+    public static class Observable extends BaseObservable implements Parcelable {
-        @Override
-        public Peer createFromParcel(final Parcel in) {
-            return new Peer(in);
-        }
-
-        @Override
-        public Peer[] newArray(final int size) {
-            return new Peer[size];
-        }
-    };
-
-    public static class Observable extends BaseObservable {
         private String allowedIPs;
         private String endpoint;
         private String persistentKeepalive;
         private String preSharedKey;
         private String publicKey;
 
-
         public Observable(Peer parent) {
             loadData(parent);
         }
@@ -118,6 +105,41 @@ public class Peer implements Parcelable {
             this.publicKey = publicKey;
             notifyPropertyChanged(BR.publicKey);
         }
+
+
+        public static final Creator<Observable> CREATOR = new Creator<Observable>() {
+            @Override
+            public Observable createFromParcel(final Parcel in) {
+                return new Observable(in);
+            }
+
+            @Override
+            public Observable[] newArray(final int size) {
+                return new Observable[size];
+            }
+        };
+
+        @Override
+        public int describeContents() {
+            return 0;
+        }
+
+        @Override
+        public void writeToParcel(final Parcel dest, final int flags) {
+            dest.writeString(allowedIPs);
+            dest.writeString(endpoint);
+            dest.writeString(persistentKeepalive);
+            dest.writeString(preSharedKey);
+            dest.writeString(publicKey);
+        }
+
+        private Observable(final Parcel in) {
+            allowedIPs = in.readString();
+            endpoint = in.readString();
+            persistentKeepalive = in.readString();
+            preSharedKey = in.readString();
+            publicKey = in.readString();
+        }
     }
 
     private List<IPCidr> allowedIPsList;
@@ -130,26 +152,6 @@ public class Peer implements Parcelable {
         allowedIPsList = new LinkedList<>();
     }
 
-    private Peer(final Parcel in) {
-        allowedIPsList = in.createTypedArrayList(IPCidr.CREATOR);
-        String host = in.readString();
-        int port = in.readInt();
-        if (host != null && !host.isEmpty() && port > 0)
-            endpoint = InetSocketAddress.createUnresolved(host, port);
-        persistentKeepalive = in.readInt();
-        preSharedKey = in.readString();
-        if (preSharedKey != null && preSharedKey.isEmpty())
-            preSharedKey = null;
-        publicKey = in.readString();
-        if (publicKey != null && publicKey.isEmpty())
-            publicKey = null;
-    }
-
-    @Override
-    public int describeContents() {
-        return 0;
-    }
-
 
     private String getAllowedIPsString() {
         if (allowedIPsList.isEmpty())
@@ -299,14 +301,4 @@ public class Peer implements Parcelable {
             sb.append(Attribute.PUBLIC_KEY.composeWith(publicKey));
         return sb.toString();
     }
-
-    @Override
-    public void writeToParcel(final Parcel dest, final int flags) {
-        dest.writeTypedList(allowedIPsList);
-        dest.writeString(endpoint == null ? null : endpoint.getHostString());
-        dest.writeInt(endpoint == null ? 0 : endpoint.getPort());
-        dest.writeInt(persistentKeepalive);
-        dest.writeString(preSharedKey);
-        dest.writeString(publicKey);
-    }
 }

app/src/main/res/values/strings.xml

@@ -65,5 +65,5 @@
     <string name="tunnel_create_error">Unable to create tunnel: %s</string>
     <string name="tunnel_create_success">Successfully created tunnel “%s”</string>
     <string name="tunnel_rename_error">Unable to rename tunnel: %s</string>
-    <string name="tunnel_rename_success">Successfully renamed tunnel “%s” to “%s”</string>
+    <string name="tunnel_rename_success">Successfully renamed tunnel to “%s”</string>
 </resources>