Refactoring the Config Form interactive code

Add new hints and fixed some old interaction bugs

shadowsocks-csharp/Model/Configuration.cs

@@ -1,9 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
-
-using Shadowsocks.Controller;
 using Newtonsoft.Json;
+using Shadowsocks.Controller;
 
 namespace Shadowsocks.Model
 {
@@ -34,7 +33,7 @@ namespace Shadowsocks.Model
         public ProxyConfig proxy;
         public HotkeyConfig hotkey;
 
-        private static string CONFIG_FILE = "gui-config.json";
+        private static readonly string CONFIG_FILE = "gui-config.json";
 
         public Server GetCurrentServer()
         {
@@ -46,12 +45,25 @@ namespace Shadowsocks.Model
 
         public static void CheckServer(Server server)
         {
+            CheckServer(server.server);
             CheckPort(server.server_port);
             CheckPassword(server.password);
-            CheckServer(server.server);
             CheckTimeout(server.timeout, Server.MaxServerTimeoutSec);
         }
 
+        public static bool ChecksServer(Server server)
+        {
+            try
+            {
+                CheckServer(server);
+                return true;
+            }
+            catch (Exception)
+            {
+                return false;
+            }
+        }
+
         public static Configuration Load()
         {
             try
@@ -125,12 +137,18 @@ namespace Shadowsocks.Model
             }
         }
 
-        public static Server AddDefaultServerOrServer(Configuration config, Server server = null)
+        public static Server AddDefaultServerOrServer(Configuration config, Server server = null, int? index = null)
         {
             if (config != null && config.configs != null)
             {
                 server = (server ?? GetDefaultServer());
-                config.configs.Add(server);
+
+                config.configs.Insert(index.GetValueOrDefault(config.configs.Count), server);
+
+                //if (index.HasValue)
+                //    config.configs.Insert(index.Value, server);
+                //else
+                //    config.configs.Add(server);
             }
             return server;
         }
@@ -174,8 +192,7 @@ namespace Shadowsocks.Model
         public static void CheckTimeout(int timeout, int maxTimeout)
         {
             if (timeout <= 0 || timeout > maxTimeout)
-                throw new ArgumentException(
-                    I18N.GetString("Timeout is invalid, it should not exceed {0}", maxTimeout));
+                throw new ArgumentException(I18N.GetString("Timeout is invalid, it should not exceed {0}", maxTimeout));
         }
 
         public static void CheckProxyAuthUser(string user)

shadowsocks-csharp/View/ConfigForm.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Drawing;
+using System.Runtime.CompilerServices;
 using System.Windows.Forms;
 using Shadowsocks.Controller;
 using Shadowsocks.Model;
@@ -17,20 +18,20 @@ namespace Shadowsocks.View
 
         public ConfigForm(ShadowsocksController controller)
         {
-            this.Font = SystemFonts.MessageBoxFont;
+            Font = SystemFonts.MessageBoxFont;
             InitializeComponent();
 
             // a dirty hack
-            this.ServersListBox.Dock = DockStyle.Fill;
-            this.tableLayoutPanel5.Dock = DockStyle.Fill;
-            this.PerformLayout();
+            ServersListBox.Dock = DockStyle.Fill;
+            tableLayoutPanel5.Dock = DockStyle.Fill;
+            PerformLayout();
 
             UpdateTexts();
             SetupValueChangedListeners();
-            this.Icon = Icon.FromHandle(Resources.ssw128.GetHicon());
+            Icon = Icon.FromHandle(Resources.ssw128.GetHicon());
 
             this.controller = controller;
-            controller.ConfigChanged += controller_ConfigChanged;
+            controller.ConfigChanged += Controller_ConfigChanged;
 
             LoadCurrentConfiguration();
         }
@@ -51,7 +52,7 @@ namespace Shadowsocks.View
             NeedPluginArgCheckBox.Text = I18N.GetString("Need Plugin Argument");
             ProxyPortLabel.Text = I18N.GetString("Proxy Port");
             PortableModeCheckBox.Text = I18N.GetString("Portable Mode");
-            toolTip1.SetToolTip(this.PortableModeCheckBox, I18N.GetString("Restart required"));
+            toolTip1.SetToolTip(PortableModeCheckBox, I18N.GetString("Restart required"));
             RemarksLabel.Text = I18N.GetString("Remarks");
             TimeoutLabel.Text = I18N.GetString("Timeout(Sec)");
             ServerGroupBox.Text = I18N.GetString("Server");
@@ -60,7 +61,7 @@ namespace Shadowsocks.View
             ApplyButton.Text = I18N.GetString("Apply");
             MoveUpButton.Text = I18N.GetString("Move &Up");
             MoveDownButton.Text = I18N.GetString("Move D&own");
-            this.Text = I18N.GetString("Edit Servers");
+            Text = I18N.GetString("Edit Servers");
         }
 
         private void SetupValueChangedListeners()
@@ -78,7 +79,7 @@ namespace Shadowsocks.View
             ServerPortTextBox.TextChanged += ConfigValueChanged;
         }
 
-        private void controller_ConfigChanged(object sender, EventArgs e)
+        private void Controller_ConfigChanged(object sender, EventArgs e)
         {
             LoadCurrentConfiguration();
         }
@@ -88,7 +89,13 @@ namespace Shadowsocks.View
             ApplyButton.Enabled = true;
         }
 
-        private bool ValidateAndSaveSelectedServerDetails()
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private void UpdateIndexToEnd()
+        {
+            _lastSelectedIndex = (ServersListBox.SelectedIndex = (_modifiedConfiguration.configs.Count - 1));
+        }
+
+        private bool ValidateAndSaveSelectedServerDetails(bool isSave = false, bool isCopy = false)
         {
             try
             {
@@ -96,15 +103,17 @@ namespace Shadowsocks.View
                 {
                     return true;
                 }
-                Server server = GetServerDetailsFromUI();
-                if (server == null)
+
+                bool verify = GetServerDetailsFromUI(out Server server, isSave, isCopy);
+
+                if (server != null)
                 {
-                    return false;
-                }
-                Configuration.CheckServer(server);
-                _modifiedConfiguration.configs[_lastSelectedIndex] = server;
+                    if (isSave || isCopy)
+                        Configuration.CheckServer(server);
 
-                return true;
+                    _modifiedConfiguration.configs[_lastSelectedIndex] = server;
+                }
+                return verify;
             }
             catch (Exception ex)
             {
@@ -113,36 +122,209 @@ namespace Shadowsocks.View
             return false;
         }
 
-        private Server GetServerDetailsFromUI()
+        private bool GetServerDetailsFromUI(out Server server, bool isSave = false, bool isCopy = false)
+        {
+            server = null;
+
+            bool? checkIP = false;
+            bool? checkPort = false;
+            bool? checkPassword = false;
+            bool? checkTimeout = false;
+
+            if ((checkIP = CheckIPTextBox(out string address, isSave, isCopy)).GetValueOrDefault(false) && address != null
+                    && (checkPort = CheckServerPortTextBox(out int? addressPort, isSave, isCopy)).GetValueOrDefault(false) && addressPort.HasValue
+                        && (checkPassword = CheckPasswordTextBox(out string serverPassword, isSave, isCopy)).GetValueOrDefault(false) && serverPassword != null
+                            && (checkTimeout = CheckTimeoutTextBox(out int? timeout, isSave, isCopy)).GetValueOrDefault(false) && timeout.HasValue)
+            {
+                server = new Server()
+                {
+                    server = address,
+                    server_port = addressPort.Value,
+                    password = serverPassword,
+                    method = EncryptionSelect.Text,
+                    plugin = PluginTextBox.Text,
+                    plugin_opts = PluginOptionsTextBox.Text,
+                    plugin_args = PluginArgumentsTextBox.Text,
+                    remarks = RemarksTextBox.Text,
+                    timeout = timeout.Value,
+                };
+
+                return true;
+            }
+
+            if (checkIP == null || checkPort == null || checkTimeout == null)
+            {
+                _modifiedConfiguration.configs.RemoveAt(_lastSelectedIndex);
+                ServersListBox.SelectedIndexChanged -= ServersListBox_SelectedIndexChanged;
+
+                LoadServerNameListToUI(_modifiedConfiguration);
+                UpdateIndexToEnd();
+
+                ServersListBox.SelectedIndexChanged += ServersListBox_SelectedIndexChanged;
+                return true;
+            }
+            else
+                return false;
+        }
+
+        #region GetServerDetailsFromUI Check
+
+        private bool? CheckIPTextBox(out string address, bool isSave, bool isCopy)
+        {
+            address = null;
+            string outAddress;
+            if (Uri.CheckHostName(outAddress = IPTextBox.Text.Trim()) == UriHostNameType.Unknown)
+            {
+                if (!isSave && !isCopy && ServersListBox.Items.Count > 1 && I18N.GetString("New server").Equals(ServersListBox.Items[_lastSelectedIndex].ToString()))
+                {
+                    DialogResult result = MessageBox.Show(I18N.GetString("Whether to discard unconfigured servers"), I18N.GetString("Operation failure"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.OK)
+                        return null;
+                }
+                else if (ApplyButton.Enabled && !isSave && !isCopy)
+                {
+                    var result = MessageBox.Show(I18N.GetString("Invalid server address, Cannot automatically save or discard changes"), I18N.GetString("Auto save failed"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.Cancel)
+                        return false;
+                    else
+                    {
+                        address = _modifiedConfiguration.configs[_lastSelectedIndex].server;
+                        return true;
+                    }
+                }
+                else
+                {
+                    MessageBox.Show(I18N.GetString("Invalid server address"), I18N.GetString("Operation failure"));
+                    IPTextBox.Focus();
+                }
+                return false;
+            }
+            else
+            {
+                address = outAddress;
+            }
+            return true;
+        }
+
+        private bool? CheckServerPortTextBox(out int? addressPort, bool isSave, bool isCopy)
+        {
+            addressPort = null;
+            if (!int.TryParse(ServerPortTextBox.Text, out int outaddressPort))
+            {
+                if (!isSave && !isCopy && ServersListBox.Items.Count > 1 && I18N.GetString("New server").Equals(ServersListBox.Items[_lastSelectedIndex].ToString()))
+                {
+                    DialogResult result = MessageBox.Show(I18N.GetString("Whether to discard unconfigured servers"), I18N.GetString("Operation failure"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.OK)
+                        return null;
+                }
+                else if (!isSave && !isCopy)
+                {
+                    var result = MessageBox.Show(I18N.GetString("Illegal port number format, Cannot automatically save or discard changes"), I18N.GetString("Auto save failed"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.Cancel)
+                        return false;
+                    else
+                    {
+                        addressPort = _modifiedConfiguration.configs[_lastSelectedIndex].server_port;
+                        return true;
+                    }
+                }
+                else
+                {
+                    MessageBox.Show(I18N.GetString("Illegal port number format"), I18N.GetString("Operation failure"));
+                    ServerPortTextBox.Focus();
+                }
+                return false;
+            }
+            else
+            {
+                addressPort = outaddressPort;
+            }
+            return true;
+        }
+
+        private bool? CheckPasswordTextBox(out string password, bool isSave, bool isCopy)
         {
-            Server server = new Server();
-            if (Uri.CheckHostName(server.server = IPTextBox.Text.Trim()) == UriHostNameType.Unknown)
+            password = null;
+            string outPassword;
+            if ((outPassword = PasswordTextBox.Text).IsNullOrWhiteSpace())
             {
-                MessageBox.Show(I18N.GetString("Invalid server address"));
-                IPTextBox.Focus();
-                return null;
+                if (!isSave && !isCopy && ServersListBox.Items.Count > 1 && I18N.GetString("New server").Equals(ServersListBox.Items[_lastSelectedIndex].ToString()))
+                {
+                    DialogResult result = MessageBox.Show(I18N.GetString("Whether to discard unconfigured servers"), I18N.GetString("Operation failure"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.OK)
+                        return null;
+                }
+                else if (ApplyButton.Enabled && !isSave && !isCopy)
+                {
+                    var result = MessageBox.Show(I18N.GetString("Password can not be blank, Cannot automatically save or discard changes"), I18N.GetString("Auto save failed"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.Cancel)
+                        return false;
+                    else
+                    {
+                        password = _modifiedConfiguration.configs[_lastSelectedIndex].password;
+                        return true;
+                    }
+                }
+                else
+                {
+                    MessageBox.Show(I18N.GetString("Password can not be blank"), I18N.GetString("Operation failure"));
+                    PasswordTextBox.Focus();
+                }
+                return false;
             }
-            if (!int.TryParse(ServerPortTextBox.Text, out server.server_port))
+            else
             {
-                MessageBox.Show(I18N.GetString("Illegal port number format"));
-                ServerPortTextBox.Focus();
-                return null;
+                password = outPassword;
             }
-            server.password = PasswordTextBox.Text;
-            server.method = EncryptionSelect.Text;
-            server.plugin = PluginTextBox.Text;
-            server.plugin_opts = PluginOptionsTextBox.Text;
-            server.plugin_args = PluginArgumentsTextBox.Text;
-            server.remarks = RemarksTextBox.Text;
-            if (!int.TryParse(TimeoutTextBox.Text, out server.timeout))
+            return true;
+        }
+
+        private bool? CheckTimeoutTextBox(out int? timeout, bool isSave, bool isCopy)
+        {
+            timeout = null;
+            if (!int.TryParse(TimeoutTextBox.Text, out int outTimeout))
+            {
+                if (!isSave && !isCopy && ServersListBox.Items.Count > 1 && I18N.GetString("New server").Equals(ServersListBox.Items[_lastSelectedIndex].ToString()))
+                {
+                    DialogResult result = MessageBox.Show(I18N.GetString("Whether to discard unconfigured servers"), I18N.GetString("Operation failure"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.OK)
+                        return null;
+                }
+                else if (ApplyButton.Enabled && !isSave && !isCopy)
+                {
+                    var result = MessageBox.Show(I18N.GetString("Illegal timeout format, Cannot automatically save or discard changes"), I18N.GetString("Auto save failed"), MessageBoxButtons.OKCancel);
+
+                    if (result == DialogResult.Cancel)
+                        return false;
+                    else
+                    {
+                        timeout = _modifiedConfiguration.configs[_lastSelectedIndex].timeout;
+                        return true;
+                    }
+                }
+                else
+                {
+                    MessageBox.Show(I18N.GetString("Illegal timeout format"), I18N.GetString("Operation failure"));
+                    TimeoutTextBox.Focus();
+                }
+                return false;
+            }
+            else
             {
-                MessageBox.Show(I18N.GetString("Illegal timeout format"));
-                TimeoutTextBox.Focus();
-                return null;
+                timeout = outTimeout;
             }
-            return server;
+            return true;
         }
 
+        #endregion
+
         private void LoadSelectedServerDetails()
         {
             if (ServersListBox.SelectedIndex >= 0 && ServersListBox.SelectedIndex < _modifiedConfiguration.configs.Count)
@@ -168,6 +350,8 @@ namespace Shadowsocks.View
 
             RemarksTextBox.Text = server.remarks;
             TimeoutTextBox.Text = server.timeout.ToString();
+
+            ApplyButton.Enabled = false;
         }
 
         private void ShowHidePluginArgInput(bool show)
@@ -176,7 +360,6 @@ namespace Shadowsocks.View
             PluginArgumentsLabel.Visible = show;
         }
 
-
         private void LoadServerNameListToUI(Configuration configuration)
         {
             ServersListBox.Items.Clear();
@@ -200,20 +383,17 @@ namespace Shadowsocks.View
             ServersListBox.SelectedIndex = _lastSelectedIndex;
             UpdateButtons();
             LoadSelectedServerDetails();
+
             ProxyPortTextBox.Text = _modifiedConfiguration.localPort.ToString();
             PortableModeCheckBox.Checked = _modifiedConfiguration.portableMode;
+
             ApplyButton.Enabled = false;
         }
 
         private bool SaveValidConfiguration()
         {
-            if (!ValidateAndSaveSelectedServerDetails())
-            {
-                return false;
-            }
-            if (_modifiedConfiguration.configs.Count == 0)
+            if (!ValidateAndSaveSelectedServerDetails(isSave: true))
             {
-                MessageBox.Show(I18N.GetString("Please add at least one server"));
                 return false;
             }
 
@@ -269,81 +449,62 @@ namespace Shadowsocks.View
 
         private void AddButton_Click(object sender, EventArgs e)
         {
-            if (!ValidateAndSaveSelectedServerDetails())
+            if (ValidateAndSaveSelectedServerDetails(isSave: true))
             {
-                return;
+                Configuration.AddDefaultServerOrServer(_modifiedConfiguration);
+                LoadServerNameListToUI(_modifiedConfiguration);
+                UpdateIndexToEnd();
             }
-            Configuration.AddDefaultServerOrServer(_modifiedConfiguration);
-            LoadServerNameListToUI(_modifiedConfiguration);
-            ServersListBox.SelectedIndex = _modifiedConfiguration.configs.Count - 1;
-            _lastSelectedIndex = ServersListBox.SelectedIndex;
         }
 
         private void DuplicateButton_Click(object sender, EventArgs e)
         {
-            if (_lastSelectedIndex == -1 || _lastSelectedIndex > _modifiedConfiguration.configs.Count
-                || !ValidateAndSaveSelectedServerDetails())
+            if (ValidateAndSaveSelectedServerDetails(isCopy: true))
             {
-                return;
+                Server currServer = _modifiedConfiguration.configs[_lastSelectedIndex];
+                Configuration.AddDefaultServerOrServer(_modifiedConfiguration, currServer);
+                LoadServerNameListToUI(_modifiedConfiguration);
+                UpdateIndexToEnd();
             }
-            Server currServer = _modifiedConfiguration.configs[_lastSelectedIndex];
-            var currIndex = _modifiedConfiguration.configs.IndexOf(currServer);
-            _modifiedConfiguration.configs.Insert(currIndex + 1, currServer);
-            LoadServerNameListToUI(_modifiedConfiguration);
-            ServersListBox.SelectedIndex = currIndex + 1;
-            _lastSelectedIndex = ServersListBox.SelectedIndex;
         }
 
         private void DeleteButton_Click(object sender, EventArgs e)
         {
-            _lastSelectedIndex = ServersListBox.SelectedIndex;
-            if (_lastSelectedIndex >= 0 && _lastSelectedIndex < _modifiedConfiguration.configs.Count)
-            {
-                _modifiedConfiguration.configs.RemoveAt(_lastSelectedIndex);
-            }
+            _modifiedConfiguration.configs.RemoveAt(_lastSelectedIndex);
+
             if (_modifiedConfiguration.configs.Count == 0)
             {
                 Configuration.AddDefaultServerOrServer(_modifiedConfiguration);
             }
-            if (_lastSelectedIndex >= _modifiedConfiguration.configs.Count)
-            {
-                // can be -1
-                _lastSelectedIndex = _modifiedConfiguration.configs.Count - 1;
-            }
-            ServersListBox.SelectedIndex = _lastSelectedIndex;
+
             LoadServerNameListToUI(_modifiedConfiguration);
-            ServersListBox.SelectedIndex = _lastSelectedIndex;
+            UpdateIndexToEnd();
             LoadSelectedServerDetails();
 
             UpdateButtons();
         }
 
-        private void OKButton_Click(object sender, EventArgs e)
-        {
-            if (SaveValidConfiguration())
-            {
-                this.Close();
-            }
-        }
-
-        private void CancelButton_Click(object sender, EventArgs e)
-        {
-            this.Close();
-        }
-
-        private void ApplyButton_Click(object sender, EventArgs e)
+        private void UpdateButtons()
         {
-            SaveValidConfiguration();
+            DeleteButton.Enabled = (ServersListBox.Items.Count > 0);
+            MoveUpButton.Enabled = (ServersListBox.SelectedIndex > 0);
+            MoveDownButton.Enabled = (ServersListBox.SelectedIndex < ServersListBox.Items.Count - 1);
         }
 
-        private void ConfigForm_Shown(object sender, EventArgs e)
+        private void MoveUpButton_Click(object sender, EventArgs e)
         {
-            IPTextBox.Focus();
+            if (ServersListBox.SelectedIndex > 0)
+            {
+                MoveConfigItem(-1);  // -1 means move backward
+            }
         }
 
-        private void ConfigForm_FormClosed(object sender, FormClosedEventArgs e)
+        private void MoveDownButton_Click(object sender, EventArgs e)
         {
-            controller.ConfigChanged -= controller_ConfigChanged;
+            if (ServersListBox.SelectedIndex < ServersListBox.Items.Count - 1)
+            {
+                MoveConfigItem(+1);  // +1 means move forward
+            }
         }
 
         private void MoveConfigItem(int step)
@@ -368,45 +529,42 @@ namespace Shadowsocks.View
             UpdateButtons();
         }
 
-        private void UpdateButtons()
+        private void OKButton_Click(object sender, EventArgs e)
         {
-            DeleteButton.Enabled = (ServersListBox.Items.Count > 0);
-            MoveUpButton.Enabled = (ServersListBox.SelectedIndex > 0);
-            MoveDownButton.Enabled = (ServersListBox.SelectedIndex < ServersListBox.Items.Count - 1);
+            if (SaveValidConfiguration())
+            {
+                Close();
+            }
         }
 
-        private void MoveUpButton_Click(object sender, EventArgs e)
+        private void CancelButton_Click(object sender, EventArgs e)
         {
-            if (!ValidateAndSaveSelectedServerDetails())
-            {
-                return;
-            }
-            if (ServersListBox.SelectedIndex > 0)
-            {
-                MoveConfigItem(-1);  // -1 means move backward
-            }
+            Close();
         }
 
-        private void MoveDownButton_Click(object sender, EventArgs e)
+        private void ApplyButton_Click(object sender, EventArgs e)
         {
-            if (!ValidateAndSaveSelectedServerDetails())
-            {
-                return;
-            }
-            if (ServersListBox.SelectedIndex < ServersListBox.Items.Count - 1)
-            {
-                MoveConfigItem(+1);  // +1 means move forward
-            }
+            SaveValidConfiguration();
+        }
+
+        private void ConfigForm_Shown(object sender, EventArgs e)
+        {
+            IPTextBox.Focus();
+        }
+
+        private void ConfigForm_FormClosed(object sender, FormClosedEventArgs e)
+        {
+            controller.ConfigChanged -= Controller_ConfigChanged;
         }
 
         private void ShowPasswdCheckBox_CheckedChanged(object sender, EventArgs e)
         {
-            this.PasswordTextBox.UseSystemPasswordChar = !this.ShowPasswdCheckBox.Checked;
+            PasswordTextBox.UseSystemPasswordChar = !ShowPasswdCheckBox.Checked;
         }
 
         private void UsePluginArgCheckBox_CheckedChanged(object sender, EventArgs e)
         {
-            ShowHidePluginArgInput(this.NeedPluginArgCheckBox.Checked);
+            ShowHidePluginArgInput(NeedPluginArgCheckBox.Checked);
         }
     }
 }

shadowsocks-csharp/View/MenuViewController.cs

@@ -438,11 +438,14 @@ namespace Shadowsocks.View
             Configuration configuration = controller.GetConfigurationCopy();
             foreach (var server in configuration.configs)
             {
-                MenuItem item = new MenuItem(server.FriendlyName());
-                item.Tag = i - strategyCount;
-                item.Click += AServerItem_Click;
-                items.Add(i, item);
-                i++;
+                if (Configuration.ChecksServer(server))
+                {
+                    MenuItem item = new MenuItem(server.FriendlyName());
+                    item.Tag = i - strategyCount;
+                    item.Click += AServerItem_Click;
+                    items.Add(i, item);
+                    i++;
+                }
             }
 
             foreach (MenuItem item in items)