From 100c2e30781bd058c325363609cdf2ab95f857eb Mon Sep 17 00:00:00 2001 From: Dawie Joubert Date: Wed, 5 Nov 2025 19:49:56 +0200 Subject: [PATCH] Fix issue #2907: Options panel freezing, disposal, and performance issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit resolves multiple related issues with the Options dialog that caused freezing, crashes, and slow performance: **Problem 1: Infinite Recursive Loop** - Symptom: Options dialog would freeze when navigating between pages - Cause: LstOptionPages_SelectedIndexChanged event handler triggering itself infinitely - Fix: Added _isHandlingSelectionChange guard flag to prevent recursive calls **Problem 2: Disposed Object Exception** - Symptom: "Cannot access a disposed object" error after SSH connection workflow - Cause: Static FrmOptions instance was disposed but still referenced - Fix: Enhanced OptionsWindow.LoadOptionsForm() to detect disposal before use - Fix: Added FrmMain.RecreateOptionsForm() to recreate disposed forms transparently **Problem 3: Index Out of Range** - Symptom: "index must be less than 0" when accessing empty lstOptionPages - Cause: SetActivatedPage() tried to access Items[0] when collection was empty - Fix: Added bounds checking before accessing lstOptionPages.Items **Problem 4: NullReferenceException in OptionsPages** - Symptom: NullReferenceException in LoadRegistrySettings() across all pages - Cause: pageRegSettingsInstance was null when registry settings didn't exist - Fix: Added null checks and default instance creation in 8 OptionsPages **Problem 5: Slow Page Loading on Recreation** - Symptom: Second Options dialog open showed staggered page loading (~2.2 seconds) - Cause: Application.Idle async pattern loaded pages one-by-one - Fix: Replaced async loading with synchronous batch loading using BeginUpdate/EndUpdate **Files Modified:** - mRemoteNG/UI/Forms/frmOptions.cs - mRemoteNG/UI/Window/OptionsWindow.cs - mRemoteNG/UI/Forms/frmMain.cs - mRemoteNG/UI/Forms/OptionsPages/StartupExitPage.cs - mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs - mRemoteNG/UI/Forms/OptionsPages/AppearancePage.cs - mRemoteNG/UI/Forms/OptionsPages/SecurityPage.cs - mRemoteNG/UI/Forms/OptionsPages/ConnectionsPage.cs - mRemoteNG/UI/Forms/OptionsPages/CredentialsPage.cs - mRemoteNG/UI/Forms/OptionsPages/TabsPanelsPage.cs - mRemoteNG/UI/Forms/OptionsPages/UpdatesPage.cs **Additional Changes:** - Replaced all Debug.WriteLine with Logger.Instance.Log for consistent logging - Added comprehensive debug logging throughout Options form lifecycle - Improved defensive programming with guard flags and validation checks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../UI/Forms/OptionsPages/AppearancePage.cs | 7 + .../UI/Forms/OptionsPages/ConnectionsPage.cs | 8 + .../UI/Forms/OptionsPages/CredentialsPage.cs | 7 + .../Forms/OptionsPages/NotificationsPage.cs | 7 + .../UI/Forms/OptionsPages/SecurityPage.cs | 7 + .../UI/Forms/OptionsPages/StartupExitPage.cs | 8 + .../UI/Forms/OptionsPages/TabsPanelsPage.cs | 10 +- .../UI/Forms/OptionsPages/UpdatesPage.cs | 7 + mRemoteNG/UI/Forms/frmMain.cs | 20 ++ mRemoteNG/UI/Forms/frmOptions.cs | 196 ++++++++++++++---- mRemoteNG/UI/Window/OptionsWindow.cs | 96 ++++++++- .../Tools/TabColorConverterTests.cs | 1 + mRemoteNGTests/UI/Forms/OptionsFormTests.cs | 112 ++++++++++ 13 files changed, 444 insertions(+), 42 deletions(-) diff --git a/mRemoteNG/UI/Forms/OptionsPages/AppearancePage.cs b/mRemoteNG/UI/Forms/OptionsPages/AppearancePage.cs index 5d9e18df..22905bae 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/AppearancePage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/AppearancePage.cs @@ -112,6 +112,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistryAppearancePage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistryAppearancePage(); + Logger.Instance.Log?.Debug("[AppearancePage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); // *** diff --git a/mRemoteNG/UI/Forms/OptionsPages/ConnectionsPage.cs b/mRemoteNG/UI/Forms/OptionsPages/ConnectionsPage.cs index f185c1fb..7dacb773 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/ConnectionsPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/ConnectionsPage.cs @@ -1,4 +1,5 @@ using System; +using mRemoteNG.App; using mRemoteNG.Config; using mRemoteNG.Config.Connections; using mRemoteNG.Properties; @@ -174,6 +175,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistryConnectionsPage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistryConnectionsPage(); + Logger.Instance.Log?.Debug("[ConnectionsPage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); // *** diff --git a/mRemoteNG/UI/Forms/OptionsPages/CredentialsPage.cs b/mRemoteNG/UI/Forms/OptionsPages/CredentialsPage.cs index 4d7054f5..eb3f75f2 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/CredentialsPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/CredentialsPage.cs @@ -89,6 +89,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistryCredentialsPage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistryCredentialsPage(); + Logger.Instance.Log?.Debug("[CredentialsPage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); // Show registry settings info if any common setting is used. diff --git a/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs b/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs index 0fdcf6b4..a2701cf7 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs @@ -150,6 +150,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistryNotificationsPage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistryNotificationsPage(); + Logger.Instance.Log?.Debug("[NotificationsPage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); LoadRegistryNotificationPanelSettings(); diff --git a/mRemoteNG/UI/Forms/OptionsPages/SecurityPage.cs b/mRemoteNG/UI/Forms/OptionsPages/SecurityPage.cs index e07b03c5..fb47937b 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/SecurityPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/SecurityPage.cs @@ -77,6 +77,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistrySecurityPage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistrySecurityPage(); + Logger.Instance.Log?.Debug("[SecurityPage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); // *** diff --git a/mRemoteNG/UI/Forms/OptionsPages/StartupExitPage.cs b/mRemoteNG/UI/Forms/OptionsPages/StartupExitPage.cs index e1f1311f..af178699 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/StartupExitPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/StartupExitPage.cs @@ -1,5 +1,6 @@ using System; using System.Runtime.Versioning; +using mRemoteNG.App; using mRemoteNG.Config.Settings.Registry; using mRemoteNG.Properties; using mRemoteNG.Resources.Language; @@ -55,6 +56,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistryStartupExitPage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistryStartupExitPage(); + Logger.Instance.Log?.Debug("[StartupExitPage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); // Disable Controls depending on the value ("None", "Minimized", or "FullScreen") diff --git a/mRemoteNG/UI/Forms/OptionsPages/TabsPanelsPage.cs b/mRemoteNG/UI/Forms/OptionsPages/TabsPanelsPage.cs index 9c97553b..c7f52e36 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/TabsPanelsPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/TabsPanelsPage.cs @@ -1,4 +1,5 @@ -using mRemoteNG.Config.Settings.Registry; +using mRemoteNG.App; +using mRemoteNG.Config.Settings.Registry; using mRemoteNG.Properties; using mRemoteNG.Resources.Language; using System; @@ -118,6 +119,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistryTabsPanelsPage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistryTabsPanelsPage(); + Logger.Instance.Log?.Debug("[TabsPanelsPage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); // *** diff --git a/mRemoteNG/UI/Forms/OptionsPages/UpdatesPage.cs b/mRemoteNG/UI/Forms/OptionsPages/UpdatesPage.cs index 43ff2c81..89d73c90 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/UpdatesPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/UpdatesPage.cs @@ -135,6 +135,13 @@ namespace mRemoteNG.UI.Forms.OptionsPages RegistryLoader.RegistrySettings.TryGetValue(settingsType, out var settings); pageRegSettingsInstance = settings as OptRegistryUpdatesPage; + // If registry settings don't exist, create a default instance to prevent null reference exceptions + if (pageRegSettingsInstance == null) + { + pageRegSettingsInstance = new OptRegistryUpdatesPage(); + Logger.Instance.Log?.Debug("[UpdatesPage.LoadRegistrySettings] pageRegSettingsInstance was null, created default instance"); + } + RegistryLoader.Cleanup(settingsType); // Checks updates are generaly disallowed. diff --git a/mRemoteNG/UI/Forms/frmMain.cs b/mRemoteNG/UI/Forms/frmMain.cs index e17f2f32..a9f61096 100644 --- a/mRemoteNG/UI/Forms/frmMain.cs +++ b/mRemoteNG/UI/Forms/frmMain.cs @@ -85,6 +85,26 @@ namespace mRemoteNG.UI.Forms private readonly FileBackupPruner _backupPruner = new(); public static FrmOptions OptionsForm; + /// + /// Recreates the OptionsForm if it has been disposed. + /// This method should be called when OptionsForm is in an invalid state. + /// + public static void RecreateOptionsForm() + { + Logger.Instance.Log?.Debug("[FrmMain.RecreateOptionsForm] Recreating OptionsForm"); + + // Dispose the old form if it exists + if (OptionsForm != null && !OptionsForm.IsDisposed) + { + Logger.Instance.Log?.Debug("[FrmMain.RecreateOptionsForm] Disposing old OptionsForm"); + OptionsForm.Dispose(); + } + + // Create a new instance + OptionsForm = new FrmOptions(); + Logger.Instance.Log?.Debug("[FrmMain.RecreateOptionsForm] New OptionsForm created"); + } + internal FullscreenHandler Fullscreen { get; set; } //Added theming support diff --git a/mRemoteNG/UI/Forms/frmOptions.cs b/mRemoteNG/UI/Forms/frmOptions.cs index cdcf0d36..6ec7e24c 100644 --- a/mRemoteNG/UI/Forms/frmOptions.cs +++ b/mRemoteNG/UI/Forms/frmOptions.cs @@ -1,4 +1,5 @@ #region Usings +using mRemoteNG.App; using mRemoteNG.UI.Forms.OptionsPages; using System; using System.Collections.Generic; @@ -17,13 +18,13 @@ namespace mRemoteNG.UI.Forms [SupportedOSPlatform("windows")] public partial class FrmOptions : Form { - private int _currentIndex = 0; private readonly List _optionPages = []; private string _pageName; private readonly DisplayProperties _display = new(); private readonly List _optionPageObjectNames; private bool _isLoading = true; - private bool _isInitialized = false; // Add this field to the FrmOptions class + private bool _isInitialized = false; + private bool _isHandlingSelectionChange = false; // Guard flag to prevent recursive event handling public FrmOptions() : this(Language.StartupExit) { @@ -59,13 +60,20 @@ namespace mRemoteNG.UI.Forms private void FrmOptions_Load(object sender, EventArgs e) { + Logger.Instance.Log?.Debug($"[FrmOptions_Load] START - IsInitialized: {_isInitialized}, Visible: {this.Visible}, Handle: {this.Handle}"); + // Only initialize once to prevent multiple event subscriptions and page reloading if (_isInitialized) { + Logger.Instance.Log?.Debug($"[FrmOptions_Load] Already initialized - calling ValidateControlState"); + // On subsequent loads, validate and recover control state if needed + ValidateControlState(); this.Visible = true; + Logger.Instance.Log?.Debug($"[FrmOptions_Load] END (already initialized)"); return; } + Logger.Instance.Log?.Debug($"[FrmOptions_Load] First initialization"); this.Visible = true; FontOverrider.FontOverride(this); SetActivatedPage(); @@ -80,26 +88,11 @@ namespace mRemoteNG.UI.Forms //ThemeManager.getInstance().ThemeChanged += ApplyTheme; lstOptionPages.SelectedIndexChanged += LstOptionPages_SelectedIndexChanged; lstOptionPages.SelectedIndex = 0; - - // Handle visibility changes to ensure panel is populated when form is shown - this.VisibleChanged += FrmOptions_VisibleChanged; + Logger.Instance.Log?.Debug($"[FrmOptions_Load] Selected index set to 0"); // Mark as initialized _isInitialized = true; - } - - private void FrmOptions_VisibleChanged(object sender, EventArgs e) - { - // When the form becomes visible after initial load, ensure the panel is populated with the selected page - // Skip this during initial load to avoid interfering with the normal initialization - if (this.Visible && _isInitialized) - { - // Clear and re-add the selected page to ensure it's properly displayed - pnlMain.Controls.Clear(); - OptionsPage page = (OptionsPage)lstOptionPages.SelectedObject; - if (page != null) - pnlMain.Controls.Add(page); - } + Logger.Instance.Log?.Debug($"[FrmOptions_Load] END (first initialization complete)"); } private void ApplyTheme() @@ -122,25 +115,33 @@ namespace mRemoteNG.UI.Forms private void InitOptionsPagesToListView() { + Logger.Instance.Log?.Debug($"[InitOptionsPagesToListView] START - Loading {_optionPageObjectNames.Count} pages"); + lstOptionPages.RowHeight = _display.ScaleHeight(lstOptionPages.RowHeight); lstOptionPages.AllColumns.First().ImageGetter = ImageGetter; - InitOptionsPage(_optionPageObjectNames[_currentIndex++]); - Application.Idle += new EventHandler(Application_Idle); - } - - private void Application_Idle(object sender, EventArgs e) - { - if (_currentIndex >= _optionPageObjectNames.Count) + // Suspend layout to prevent flickering during batch loading + lstOptionPages.BeginUpdate(); + try { - Application.Idle -= new EventHandler(Application_Idle); + // Load all pages synchronously for faster, more responsive loading + // This is especially important when the form is recreated (second+ open) + foreach (var pageName in _optionPageObjectNames) + { + Logger.Instance.Log?.Debug($"[InitOptionsPagesToListView] Loading page: {pageName}"); + InitOptionsPage(pageName); + } + // All pages loaded, now start tracking changes _isLoading = false; + Logger.Instance.Log?.Debug($"[InitOptionsPagesToListView] All {_optionPageObjectNames.Count} pages loaded"); } - else + finally { - InitOptionsPage(_optionPageObjectNames[_currentIndex++]); + lstOptionPages.EndUpdate(); } + + Logger.Instance.Log?.Debug($"[InitOptionsPagesToListView] END"); } private void InitOptionsPage(string pageName) @@ -256,6 +257,13 @@ namespace mRemoteNG.UI.Forms { _pageName = pageName ?? Language.StartupExit; + // Ensure we have items loaded before trying to access them + if (lstOptionPages.Items.Count == 0) + { + Logger.Instance.Log?.Warn($"[SetActivatedPage] No items in lstOptionPages, cannot set active page to '{_pageName}'"); + return; + } + bool isSet = false; for (int i = 0; i < lstOptionPages.Items.Count; i++) { @@ -265,58 +273,110 @@ namespace mRemoteNG.UI.Forms break; } - if (!isSet) + if (!isSet && lstOptionPages.Items.Count > 0) lstOptionPages.Items[0].Selected = true; } private void BtnOK_Click(object sender, EventArgs e) { + Logger.Instance.Log?.Debug($"[BtnOK_Click] START"); SaveOptions(); // Clear change flags after saving ClearChangeFlags(); this.Visible = false; + Logger.Instance.Log?.Debug($"[BtnOK_Click] END - Visible set to false"); } private void BtnApply_Click(object sender, EventArgs e) { + Logger.Instance.Log?.Debug($"[BtnApply_Click] START"); SaveOptions(); // Clear change flags after applying ClearChangeFlags(); + Logger.Instance.Log?.Debug($"[BtnApply_Click] END"); } private void SaveOptions() { foreach (OptionsPage page in _optionPages) { - Debug.WriteLine(page.PageName); + Logger.Instance.Log?.Debug($"[SaveOptions] Saving page: {page.PageName}"); page.SaveSettings(); } - Debug.WriteLine((ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None)).FilePath); + Logger.Instance.Log?.Debug($"[SaveOptions] Configuration file: {(ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None)).FilePath}"); Settings.Default.Save(); } private void LstOptionPages_SelectedIndexChanged(object sender, EventArgs e) { - pnlMain.Controls.Clear(); + // Guard against recursive calls that can cause infinite loops + if (_isHandlingSelectionChange) + { + Logger.Instance.Log?.Warn($"[LstOptionPages_SelectedIndexChanged] RECURSIVE CALL BLOCKED - Preventing infinite loop"); + return; + } - OptionsPage page = (OptionsPage)lstOptionPages.SelectedObject; - if (page != null) + _isHandlingSelectionChange = true; + try + { + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] START - IsLoading: {_isLoading}, SelectedIndex: {lstOptionPages.SelectedIndex}, Items.Count: {lstOptionPages.Items.Count}"); + + pnlMain.Controls.Clear(); + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] pnlMain.Controls cleared"); + + OptionsPage page = lstOptionPages.SelectedObject as OptionsPage; + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] SelectedObject: {(page != null ? page.PageName : "NULL")}"); + + if (page == null) + { + Logger.Instance.Log?.Warn($"[LstOptionPages_SelectedIndexChanged] Page is NULL - cannot display. This may indicate a selection issue."); + return; + } + + if (page.IsDisposed) + { + Logger.Instance.Log?.Error($"[LstOptionPages_SelectedIndexChanged] Page '{page.PageName}' is disposed - cannot display"); + return; + } + + // Ensure the page has a valid window handle + if (!page.IsHandleCreated) + { + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] Page '{page.PageName}' has no handle - creating handle"); + var handle = page.Handle; // This creates the handle + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] Handle created: {handle}"); + } + + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] Adding page '{page.PageName}' to pnlMain"); pnlMain.Controls.Add(page); + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] Page added successfully. pnlMain.Controls.Count: {pnlMain.Controls.Count}"); + + Logger.Instance.Log?.Debug($"[LstOptionPages_SelectedIndexChanged] END"); + } + finally + { + _isHandlingSelectionChange = false; + } } private void BtnCancel_Click(object sender, EventArgs e) { + Logger.Instance.Log?.Debug($"[BtnCancel_Click] START"); // When Cancel is clicked, we don't check for changes // The user explicitly wants to cancel this.Visible = false; + Logger.Instance.Log?.Debug($"[BtnCancel_Click] END - Visible set to false"); } private void FrmOptions_FormClosing(object sender, FormClosingEventArgs e) { + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] START - CloseReason: {e.CloseReason}, Cancel: {e.Cancel}"); + // Check if any page has unsaved changes bool hasChanges = _optionPages.Any(page => page.HasChanges); - + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] HasChanges: {hasChanges}"); + if (hasChanges) { DialogResult result = MessageBox.Show( @@ -324,7 +384,9 @@ namespace mRemoteNG.UI.Forms Language.Options, MessageBoxButtons.YesNoCancel, MessageBoxIcon.Question); - + + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] User choice: {result}"); + switch (result) { case DialogResult.Yes: @@ -332,16 +394,19 @@ namespace mRemoteNG.UI.Forms ClearChangeFlags(); e.Cancel = true; this.Visible = false; + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] Saved and hiding"); break; case DialogResult.No: // Discard changes ClearChangeFlags(); e.Cancel = true; this.Visible = false; + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] Discarded and hiding"); break; case DialogResult.Cancel: // Cancel closing - keep the dialog open e.Cancel = true; + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] User cancelled close"); break; } } @@ -349,7 +414,10 @@ namespace mRemoteNG.UI.Forms { e.Cancel = true; this.Visible = false; + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] No changes - hiding"); } + + Logger.Instance.Log?.Debug($"[FrmOptions_FormClosing] END - Cancel: {e.Cancel}, Visible: {this.Visible}"); } private void TrackChangesInControls(Control control) @@ -415,5 +483,59 @@ namespace mRemoteNG.UI.Forms page.HasChanges = false; } } + + private void ValidateControlState() + { + Logger.Instance.Log?.Debug($"[ValidateControlState] START - Items.Count: {lstOptionPages.Items.Count}, pnlMain.Controls.Count: {pnlMain.Controls.Count}"); + + // Ensure we have pages loaded + if (lstOptionPages.Items.Count == 0) + { + Logger.Instance.Log?.Debug($"[ValidateControlState] No items loaded - returning"); + return; + } + + OptionsPage currentPage = lstOptionPages.SelectedObject as OptionsPage; + Logger.Instance.Log?.Debug($"[ValidateControlState] Current page: {(currentPage != null ? currentPage.PageName : "NULL")}"); + + if (currentPage == null) + { + Logger.Instance.Log?.Warn($"[ValidateControlState] SelectedObject is NULL - this may indicate a selection issue"); + // Don't try to fix this - let the normal selection handling deal with it + return; + } + + if (currentPage.IsDisposed) + { + Logger.Instance.Log?.Warn($"[ValidateControlState] Page '{currentPage.PageName}' is disposed"); + return; + } + + Logger.Instance.Log?.Debug($"[ValidateControlState] Page '{currentPage.PageName}' is valid - IsHandleCreated: {currentPage.IsHandleCreated}"); + + // Ensure the page has a valid window handle + if (!currentPage.IsHandleCreated) + { + Logger.Instance.Log?.Debug($"[ValidateControlState] Creating handle for page '{currentPage.PageName}'"); + // Force handle creation + var handle = currentPage.Handle; + Logger.Instance.Log?.Debug($"[ValidateControlState] Handle created: {handle}"); + } + + // Ensure the page is displayed in the panel + if (!pnlMain.Controls.Contains(currentPage)) + { + Logger.Instance.Log?.Debug($"[ValidateControlState] Page '{currentPage.PageName}' not in pnlMain - adding it now"); + pnlMain.Controls.Clear(); + pnlMain.Controls.Add(currentPage); + Logger.Instance.Log?.Debug($"[ValidateControlState] Page added. pnlMain.Controls.Count: {pnlMain.Controls.Count}"); + } + else + { + Logger.Instance.Log?.Debug($"[ValidateControlState] Page '{currentPage.PageName}' already in pnlMain - OK"); + } + + Logger.Instance.Log?.Debug($"[ValidateControlState] END"); + } } } \ No newline at end of file diff --git a/mRemoteNG/UI/Window/OptionsWindow.cs b/mRemoteNG/UI/Window/OptionsWindow.cs index 3f3931bb..0b9714c5 100644 --- a/mRemoteNG/UI/Window/OptionsWindow.cs +++ b/mRemoteNG/UI/Window/OptionsWindow.cs @@ -1,6 +1,7 @@ using System; using System.Runtime.Versioning; using System.Windows.Forms; +using mRemoteNG.App; using mRemoteNG.Themes; using mRemoteNG.UI.Forms; using WeifenLuo.WinFormsUI.Docking; @@ -35,16 +36,39 @@ namespace mRemoteNG.UI.Window private void Options_Load(object sender, EventArgs e) { + Logger.Instance.Log?.Debug($"[OptionsWindow.Options_Load] START - IsInitialized: {_isInitialized}, Visible: {this.Visible}"); + // Only subscribe to ThemeChanged once to prevent multiple subscriptions if (!_isInitialized) { + Logger.Instance.Log?.Debug($"[OptionsWindow.Options_Load] First initialization - subscribing to ThemeChanged"); ThemeManager.getInstance().ThemeChanged += ApplyTheme; _isInitialized = true; } - + ApplyTheme(); ApplyLanguage(); LoadOptionsForm(); + + // Ensure all pages are loaded and form is ready + EnsureOptionsFormReady(); + + Logger.Instance.Log?.Debug($"[OptionsWindow.Options_Load] END"); + } + + private void EnsureOptionsFormReady() + { + Logger.Instance.Log?.Debug($"[OptionsWindow.EnsureOptionsFormReady] START - OptionsForm: {(_optionsForm != null ? "EXISTS" : "NULL")}, IsDisposed: {_optionsForm?.IsDisposed}"); + + if (_optionsForm != null && !_optionsForm.IsDisposed) + { + Logger.Instance.Log?.Debug($"[OptionsWindow.EnsureOptionsFormReady] Processing Application.DoEvents"); + // Process any pending Application.Idle events to ensure all pages are loaded + // This prevents race conditions with async page loading + Application.DoEvents(); + } + + Logger.Instance.Log?.Debug($"[OptionsWindow.EnsureOptionsFormReady] END"); } private new void ApplyTheme() @@ -61,43 +85,107 @@ namespace mRemoteNG.UI.Window private void LoadOptionsForm() { - if (_optionsForm == null || _optionsForm.IsDisposed) + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] START - _optionsForm: {(_optionsForm != null ? "EXISTS" : "NULL")}, IsDisposed: {_optionsForm?.IsDisposed}"); + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] FrmMain.OptionsForm: {(FrmMain.OptionsForm != null ? "EXISTS" : "NULL")}, IsDisposed: {FrmMain.OptionsForm?.IsDisposed}"); + + // Check if FrmMain.OptionsForm is disposed FIRST (this is the source of truth) + if (FrmMain.OptionsForm != null && FrmMain.OptionsForm.IsDisposed) { + Logger.Instance.Log?.Warn($"[OptionsWindow.LoadOptionsForm] FrmMain.OptionsForm is DISPOSED - recreating"); + // Force FrmMain to recreate the OptionsForm + FrmMain.RecreateOptionsForm(); + } + + // If the local reference is disposed, we need to clean up + if (_optionsForm != null && _optionsForm.IsDisposed) + { + Logger.Instance.Log?.Warn($"[OptionsWindow.LoadOptionsForm] Local _optionsForm is DISPOSED - cleaning up"); + if (Controls.Contains(_optionsForm)) + { + Controls.Remove(_optionsForm); + } + _optionsForm.VisibleChanged -= OptionsForm_VisibleChanged; + _optionsForm = null; + } + + // Get fresh reference if needed + if (_optionsForm == null) + { + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] Getting fresh OptionsForm from FrmMain"); _optionsForm = FrmMain.OptionsForm; + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] OptionsForm retrieved from FrmMain: {(_optionsForm != null ? "SUCCESS" : "FAILED")}"); + + if (_optionsForm == null) + { + Logger.Instance.Log?.Error($"[OptionsWindow.LoadOptionsForm] CRITICAL: Failed to get OptionsForm from FrmMain"); + return; + } + + // Double-check that the form we just got is not disposed + if (_optionsForm.IsDisposed) + { + Logger.Instance.Log?.Error($"[OptionsWindow.LoadOptionsForm] CRITICAL: FrmMain.OptionsForm is STILL disposed after recreation attempt!"); + return; + } + _optionsForm.TopLevel = false; _optionsForm.FormBorderStyle = FormBorderStyle.None; _optionsForm.Dock = DockStyle.Fill; _optionsForm.VisibleChanged += OptionsForm_VisibleChanged; Controls.Add(_optionsForm); + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] OptionsForm added to Controls"); } - _optionsForm.Show(); + + // Only show if not already visible to prevent redundant event cascades + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] OptionsForm.Visible: {_optionsForm.Visible}, IsDisposed: {_optionsForm.IsDisposed}"); + if (!_optionsForm.Visible) + { + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] Calling Show()"); + _optionsForm.Show(); + } + else + { + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] Already visible - skipping Show()"); + } + + Logger.Instance.Log?.Debug($"[OptionsWindow.LoadOptionsForm] END"); } private void OptionsForm_VisibleChanged(object sender, EventArgs e) { + Logger.Instance.Log?.Debug($"[OptionsWindow.OptionsForm_VisibleChanged] OptionsForm.Visible: {_optionsForm?.Visible}"); + // When the embedded FrmOptions is hidden (OK/Cancel clicked), close this window if (_optionsForm != null && !_optionsForm.Visible) { + Logger.Instance.Log?.Debug($"[OptionsWindow.OptionsForm_VisibleChanged] OptionsForm hidden - closing OptionsWindow"); this.Close(); } } protected override void OnFormClosing(FormClosingEventArgs e) { + Logger.Instance.Log?.Debug($"[OptionsWindow.OnFormClosing] START - CloseReason: {e.CloseReason}"); // With HideOnClose = true, we don't dispose the window // so we keep the embedded form in Controls for reuse base.OnFormClosing(e); + Logger.Instance.Log?.Debug($"[OptionsWindow.OnFormClosing] END"); } protected override void OnVisibleChanged(EventArgs e) { + Logger.Instance.Log?.Debug($"[OptionsWindow.OnVisibleChanged] START - Visible: {Visible}, OptionsForm: {(_optionsForm != null ? "EXISTS" : "NULL")}"); + base.OnVisibleChanged(e); - + // When the window becomes visible, ensure the embedded form is also shown if (Visible && _optionsForm != null && !_optionsForm.Visible) { + Logger.Instance.Log?.Debug($"[OptionsWindow.OnVisibleChanged] Window visible but OptionsForm hidden - calling Show()"); _optionsForm.Show(); } + + Logger.Instance.Log?.Debug($"[OptionsWindow.OnVisibleChanged] END"); } public void SetActivatedPage(string pageName) diff --git a/mRemoteNGTests/Tools/TabColorConverterTests.cs b/mRemoteNGTests/Tools/TabColorConverterTests.cs index 40a1ecec..ffdd94c2 100644 --- a/mRemoteNGTests/Tools/TabColorConverterTests.cs +++ b/mRemoteNGTests/Tools/TabColorConverterTests.cs @@ -1,3 +1,4 @@ +using System; using System.Drawing; using mRemoteNG.Tools; using NUnit.Framework; diff --git a/mRemoteNGTests/UI/Forms/OptionsFormTests.cs b/mRemoteNGTests/UI/Forms/OptionsFormTests.cs index bd3b8048..f6381724 100644 --- a/mRemoteNGTests/UI/Forms/OptionsFormTests.cs +++ b/mRemoteNGTests/UI/Forms/OptionsFormTests.cs @@ -66,5 +66,117 @@ namespace mRemoteNGTests.UI.Forms } } } + + [Test] + public void MultipleOpenCloseWithoutConnectionsDoesNotFreeze() + { + // Test for issue #2907: Options panel should not freeze after multiple open/close cycles + for (int i = 0; i < 25; i++) + { + // Show the form + _optionsForm.Show(); + Application.DoEvents(); + System.Threading.Thread.Sleep(50); + + // Verify panel has content + var pnlMain = _optionsForm.FindControl("pnlMain"); + Assert.That(pnlMain, Is.Not.Null, $"pnlMain is null on iteration {i}"); + Assert.That(pnlMain.Controls.Count, Is.GreaterThan(0), $"pnlMain has no controls on iteration {i}"); + + // Hide the form (simulating OK/Cancel) + _optionsForm.Visible = false; + Application.DoEvents(); + } + + // Final check - form should still be responsive + _optionsForm.Show(); + Application.DoEvents(); + var finalPanel = _optionsForm.FindControl("pnlMain"); + Assert.That(finalPanel.Controls.Count, Is.GreaterThan(0), "Final pnlMain has no controls after 25 cycles"); + } + + [Test] + public void OptionsFormHasValidSelectedPageAfterMultipleShows() + { + // Test for issue #2907: lstOptionPages.SelectedObject should remain valid + for (int i = 0; i < 10; i++) + { + _optionsForm.Show(); + Application.DoEvents(); + + // Use reflection to check lstOptionPages.SelectedObject + var lstOptionPages = _optionsForm.GetType() + .GetField("lstOptionPages", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance) + ?.GetValue(_optionsForm); + + Assert.That(lstOptionPages, Is.Not.Null, $"lstOptionPages is null on iteration {i}"); + + var selectedObject = lstOptionPages.GetType() + .GetProperty("SelectedObject") + ?.GetValue(lstOptionPages); + + Assert.That(selectedObject, Is.Not.Null, $"SelectedObject is null on iteration {i}"); + + _optionsForm.Visible = false; + Application.DoEvents(); + } + } + + [Test] + public void OptionsFormControlStateRemainsValidAfterHideShow() + { + // Test for issue #2907: Control handles should remain valid after hide/show + _optionsForm.Show(); + Application.DoEvents(); + System.Threading.Thread.Sleep(500); // Wait for all pages to load + + var pnlMain = _optionsForm.FindControl("pnlMain"); + Assert.That(pnlMain.Controls.Count, Is.GreaterThan(0)); + + var firstPage = pnlMain.Controls[0]; + Assert.That(firstPage.IsHandleCreated, Is.True, "Page handle should be created initially"); + + // Hide and show multiple times + for (int i = 0; i < 5; i++) + { + _optionsForm.Visible = false; + Application.DoEvents(); + _optionsForm.Show(); + Application.DoEvents(); + + var currentPanel = _optionsForm.FindControl("pnlMain"); + Assert.That(currentPanel.Controls.Count, Is.GreaterThan(0), $"Panel should have controls on iteration {i}"); + + var currentPage = currentPanel.Controls[0]; + Assert.That(currentPage.IsHandleCreated, Is.True, $"Page handle should remain valid on iteration {i}"); + Assert.That(currentPage.IsDisposed, Is.False, $"Page should not be disposed on iteration {i}"); + } + } + + [Test] + public void RapidOpenCloseDoesNotCauseNullReference() + { + // Test for issue #2907: Rapid open/close should not cause null reference exceptions + for (int i = 0; i < 50; i++) + { + try + { + _optionsForm.Show(); + _optionsForm.Visible = false; + Application.DoEvents(); + } + catch (System.NullReferenceException ex) + { + Assert.Fail($"NullReferenceException on iteration {i}: {ex.Message}"); + } + } + + // Should be able to show normally after rapid cycles + Assert.DoesNotThrow(() => + { + _optionsForm.Show(); + Application.DoEvents(); + }); + } } } \ No newline at end of file