mirror of
https://github.com/mRemoteNG/mRemoteNG.git
synced 2026-02-17 22:11:48 +08:00
Fix issue #2907: Options panel freezing, disposal, and performance issues
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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);
|
||||
|
||||
// ***
|
||||
|
||||
@@ -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);
|
||||
|
||||
// ***
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
|
||||
// ***
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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);
|
||||
|
||||
// ***
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -85,6 +85,26 @@ namespace mRemoteNG.UI.Forms
|
||||
private readonly FileBackupPruner _backupPruner = new();
|
||||
public static FrmOptions OptionsForm;
|
||||
|
||||
/// <summary>
|
||||
/// Recreates the OptionsForm if it has been disposed.
|
||||
/// This method should be called when OptionsForm is in an invalid state.
|
||||
/// </summary>
|
||||
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
|
||||
|
||||
@@ -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<OptionsPage> _optionPages = [];
|
||||
private string _pageName;
|
||||
private readonly DisplayProperties _display = new();
|
||||
private readonly List<string> _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");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
using System;
|
||||
using System.Drawing;
|
||||
using mRemoteNG.Tools;
|
||||
using NUnit.Framework;
|
||||
|
||||
@@ -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<Panel>("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<Panel>("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<Panel>("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<Panel>("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();
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user