Refactor for cross platform port (#3641) #3642

lextm · 2025-12-17T12:59:19Z

Typical changes needed by the cross platform port are,

A trivial namespace rename (TextView was both a control name and a namespace).
A few types were moved to their own source files, either easy to be replaced in the cross platform project folder, or add as link.
Split a few key classes into multiple source files, so that their WPF specific parts can be easily replaced in the cross platform project.

Still a work-in-progress, but should be ready to review and merge as it doesn't change anything fundamental on the WPF side.

christophwille · 2025-12-17T14:56:44Z

Why that moving around? namespace ICSharpCode.ILSpy.TextView => namespace ICSharpCode.ILSpy.TextViewControl

lextm · 2025-12-17T16:08:12Z

Project Rover uses two compiler tricks to enable source file reuse without heavy modifications,

However, such tricks fail easily when a type has the same name as an existing namespace. The term TextView happens to be hitting this problem.

I will see how much effort it takes to avoid this namespace change though. Will provide an update once I have some results.

lextm · 2025-12-17T17:26:39Z

Reverted the namespace changes, but one more extra file to maintain in the cross platform port.

Copilot

Pull request overview

This pull request refactors the ILSpy codebase to support cross-platform porting by separating WPF-specific code from platform-independent logic. The refactoring maintains existing functionality while restructuring code to enable easier platform abstraction.

Key Changes

Split multiple classes into platform-specific (.wpf.cs) and shared (.cs) files to isolate WPF dependencies
Extracted nested classes and extension methods into separate files for better modularity and platform-specific replacement
Added IList interface implementation to SharpTreeNodeCollection for cross-platform binding compatibility

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
ILSpy/Views/OpenFromGacDialog.xaml.cs	Removed nested GacEntry class (extracted to separate file) and unused using statement
ILSpy/Views/GacEntry.cs	New file containing extracted GacEntry class nested within OpenFromGacDialog partial class
ILSpy/ViewModels/TabPageModelExtensions.wpf.cs	New file with WPF-specific Focus extension method for TabPageModel
ILSpy/ViewModels/TabPageModelExtensions.cs	New file with platform-independent TabPageModel extension methods
ILSpy/ViewModels/TabPageModel.cs	Removed TabPageModelExtensions static class (extracted to separate files) and unused using statements
ILSpy/ViewModels/PaneModel.cs	Removed Pane static class with WPF dependency properties (extracted to separate file) and unused using statement
ILSpy/ViewModels/Pane.cs	New file containing extracted Pane static class with WPF dependency properties
ILSpy/ViewModels/LegacyToolPaneModel.cs	Removed unused System.Windows using statement
ILSpy/ViewModels/DebugStepsPaneModel.cs	Removed unused System.Windows using statement
ILSpy/TreeNodes/AssemblyListTreeNode.wpf.cs	New file with WPF-specific drag-drop implementation for AssemblyListTreeNode
ILSpy/TreeNodes/AssemblyListTreeNode.cs	Made class partial, removed drag-drop methods (moved to .wpf.cs file) and unused using statements
ILSpy/TextView/VisualLineReferenceText.wpf.cs	New file with WPF-specific cursor handling for VisualLineReferenceText
ILSpy/TextView/VisualLineReferenceText.cs	New file with platform-independent VisualLineReferenceText implementation
ILSpy/TextView/ViewState.cs	New file containing extracted ViewState class from DecompilerTextView
ILSpy/TextView/ReferenceElementGenerator.cs	Removed nested VisualLineReferenceText class (extracted to separate files)
ILSpy/TextView/IBracketSearcher.cs	New file containing extracted IBracketSearcher interface
ILSpy/TextView/DefaultBracketSearcher.cs	New file containing extracted DefaultBracketSearcher class
ILSpy/TextView/DecompilerTextView.cs	Removed ViewState class (extracted to separate file)
ILSpy/TextView/BracketSearchResult.cs	New file containing extracted BracketSearchResult class
ILSpy/TextView/BracketHighlightRenderer.cs	Removed bracket-related classes (extracted to separate files) and added null-safety for resource loading
ILSpy/SmartTextOutputExtensions.cs	New file containing extracted SmartTextOutputExtensions static class
ILSpy/Search/ShowSearchCommand.cs	New file containing extracted ShowSearchCommand class with debug logging
ILSpy/Search/SearchPaneModel.cs	Added [Export] attribute and made class partial
ILSpy/Search/SearchPane.xaml.cs	Removed ShowSearchCommand class (extracted to separate file) and unused using statement
ILSpy/Options/DisplaySettings.cs	Added CROSS_PLATFORM conditional compilation for font handling
ILSpy/Languages/CSharpLanguage.wpf.cs	New file with WPF-specific AddWarningMessage implementation
ILSpy/Languages/CSharpLanguage.cs	Made class partial and removed AddWarningMessage method (moved to .wpf.cs file)
ILSpy/ISmartTextOutput.cs	Removed SmartTextOutputExtensions class (extracted to separate file) and unused using statements
ILSpy/Docking/DockWorkspace.wpf.cs	New file with WPF-specific docking and layout management implementation
ILSpy/Docking/DockWorkspace.cs	Made class partial, changed Export attribute to explicit type, removed WPF-specific methods and using statements
ILSpy/AssemblyTree/AssemblyTreeModel.wpf.cs	New file with WPF-specific constructor and initialization logic
ILSpy/AssemblyTree/AssemblyTreeModel.cs	Made class partial, removed constructor and LoadInitialAssemblies method, added TODO comments
ILSpy/AboutPage.cs	Added CROSS_PLATFORM conditional compilation for binding and image loading
ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs	Added IList interface implementation with collection manipulation methods
ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs	Added ViewChildren property and LoadingTreeNode class for cross-platform tree view binding

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-18T06:32:55Z

ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs

+		public int Add(object value)
+		{
+			return ((IList)list).Add(value);
+		}


The IList.Add implementation directly delegates to the underlying list without validation. This bypasses the parent node validation logic that exists in the generic Add method. Consider validating the node's parent before adding, similar to the generic Add(SharpTreeNode) method.

Copilot · 2025-12-18T06:32:56Z

ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs

+
+		public void Remove(object value)
+		{
+			((IList)list).Remove(value);


The IList.Remove implementation directly delegates to the underlying list without using the CollectionChanged event mechanism. This bypasses the notification logic that exists in the generic Remove method. Consider using the generic Remove method or ensuring proper event notification.

Suggested change

((IList)list).Remove(value);

// Route through the generic Remove to ensure proper notification and validation.

if (value == null || value is SharpTreeNode)

{

Remove((SharpTreeNode)value);

}

Copilot · 2025-12-18T06:32:56Z

ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs

+
+		public object SyncRoot => ((ICollection)list).SyncRoot;
+
+		object IList.this[int index] { get => ((IList)list)[index]; set => ((IList)list)[index] = value; }


The IList indexer setter directly delegates to the underlying list without validation. This bypasses the parent node validation logic that exists in the typed indexer. Consider validating the node's parent before setting, similar to the generic indexer setter.

Suggested change

object IList.this[int index] { get => ((IList)list)[index]; set => ((IList)list)[index] = value; }

object IList.this[int index]

{

get => this[index];

set

{

if (value is not SharpTreeNode node)

throw new ArgumentException("Value must be a SharpTreeNode.", nameof(value));

this[index] = node;

}

}

Copilot · 2025-12-18T06:32:56Z

ILSpy/Search/ShowSearchCommand.cs

+			Console.WriteLine($"ShowSearchCommand: Executing ShowToolPane for SearchPaneModel.PaneContentId using dockWorkspace type: {dockWorkspace?.GetType().FullName}");
+			var result = dockWorkspace.ShowToolPane(SearchPaneModel.PaneContentId);
+			Console.WriteLine($"ShowSearchCommand: ShowToolPane returned: {result}");


Debug console output statements should be removed before merging to production. These Console.WriteLine calls appear to be temporary debugging code.

Suggested change

Console.WriteLine($"ShowSearchCommand: Executing ShowToolPane for SearchPaneModel.PaneContentId using dockWorkspace type: {dockWorkspace?.GetType().FullName}");

var result = dockWorkspace.ShowToolPane(SearchPaneModel.PaneContentId);

Console.WriteLine($"ShowSearchCommand: ShowToolPane returned: {result}");

dockWorkspace.ShowToolPane(SearchPaneModel.PaneContentId);

Copilot · 2025-12-18T06:32:57Z

ILSpy/AssemblyTree/AssemblyTreeModel.cs

 				var oldSelection = selectedItems;
 				selectedItems = value;
 				OnPropertyChanged();
+				// TODO: OnPropertyChanged(nameof(SelectedItem));


TODO comment should be addressed or removed. If this is intentional work-in-progress, it should be tracked in an issue rather than left as an inline comment.

Suggested change

// TODO: OnPropertyChanged(nameof(SelectedItem));

Copilot · 2025-12-18T06:32:57Z

ILSpy/AssemblyTree/AssemblyTreeModel.cs

 			}
 			else
 			{
+				// TODO: ExpandAncestors(node);


TODO comment should be addressed or removed. If this is intentional work-in-progress, it should be tracked in an issue rather than left as an inline comment.

Suggested change

// TODO: ExpandAncestors(node);

// Ancestors are intentionally not expanded automatically when selecting a node.

Copilot · 2025-12-18T06:32:58Z

ILSpy/AssemblyTree/AssemblyTreeModel.cs

+		// TODO: void ExpandAncestors(SharpTreeNode node)
+		// {
+		// 	foreach (var ancestor in node.Ancestors().Reverse())
+		// 	{
+		// 		ancestor.EnsureLazyChildren();
+		// 		ancestor.IsExpanded = true;
+		// 	}
+		// }


TODO comment should be addressed or removed. If this represents incomplete functionality, it should be tracked in an issue or completed before merging.

Copilot · 2025-12-18T06:32:58Z

ILSpy/Search/SearchPaneModel.cs

 		public ImageSource Image { get; init; }
 	}

+	[Export]


Adding the [Export] attribute alongside [ExportToolPane] may create duplicate exports. Verify that this additional attribute is necessary and doesn't conflict with the existing export mechanism.

Suggested change

[Export]

Copilot · 2025-12-18T06:32:58Z

ILSpy/Docking/DockWorkspace.cs

 namespace ICSharpCode.ILSpy.Docking
 {
-	[Export]
+	[Export(typeof(DockWorkspace))]


The Export attribute has been changed from [Export] to [Export(typeof(DockWorkspace))]. Ensure that all import sites are compatible with this change and that the explicit type export is necessary for the cross-platform implementation.

Suggested change

[Export(typeof(DockWorkspace))]

[Export]

Copilot · 2025-12-18T06:32:59Z

ILSpy/SmartTextOutputExtensions.cs

+					if (icon != null)
+					{
+						button.Content = new StackPanel {
+							Orientation = Orientation.Horizontal,
+							Children = {
+								new Image { Width = 16, Height = 16, Source = icon, Margin = new Thickness(0, 0, 4, 0) },
+								new TextBlock { Text = text }
+							}
+						};
+					}
+					else
+					{
+						button.Content = text;
+					}


Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

Suggested change

if (icon != null)

{

button.Content = new StackPanel {

Orientation = Orientation.Horizontal,

Children = {

new Image { Width = 16, Height = 16, Source = icon, Margin = new Thickness(0, 0, 4, 0) },

new TextBlock { Text = text }

}

};

}

else

{

button.Content = text;

}

button.Content = icon != null

? (object)new StackPanel {

Orientation = Orientation.Horizontal,

Children = {

new Image { Width = 16, Height = 16, Source = icon, Margin = new Thickness(0, 0, 4, 0) },

new TextBlock { Text = text }

}

}

: text;

christophwille · 2025-12-18T06:38:19Z

Fyi, we run Copilot reviews on almost all PRs these days to see how it changes over time (quality-wise), so expect nuggets and duds.

lextm · 2025-12-18T06:51:49Z

@christophwille It actually provides some useful insights. I am still working on the repo reconfiguration, so will take care of them later.

christophwille · 2025-12-18T07:13:31Z

No need to rush anything. A well-thought-out & tested approach is appreciated.

Copilot

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-31T09:39:51Z

ILSpy/Metadata/Helper.wpf.cs

+
+namespace ICSharpCode.ILSpy.Metadata
+{
+	static partial class Helpers


The file name is 'Helper.wpf.cs' but the class name is 'Helpers' (plural). The file should be renamed to 'Helpers.wpf.cs' to match the class name convention and be consistent with other files in the project.

Copilot · 2025-12-31T09:39:52Z

ILSpy/Metadata/Helpers.cs

 				case "StartOffset":
 				case "Length":
+#if CROSS_PLATFORM
+					binding.Converter = HexFormatConverter.Instance;


The code references HexFormatConverter.Instance but this type does not appear to be defined in the changes. Ensure that HexFormatConverter is defined elsewhere in the codebase or add its implementation for the CROSS_PLATFORM build.

Copilot · 2025-12-31T09:39:52Z

ILSpy/AboutPage.cs

 					new Image {
 						Width = 16, Height = 16,
+#if CROSS_PLATFORM
+						Source = Images.LoadImage(Images.OK),


The call to Images.LoadImage(Images.OK) is incorrect. Images.OK is already an ImageSource, and LoadImage is a private method that expects (object part, string icon) parameters. This line should probably just use Images.OK directly, as it's already an ImageSource compatible with the Source property.

Suggested change

Source = Images.LoadImage(Images.OK),

Source = Images.OK,

Copilot · 2025-12-31T09:39:52Z

ILSpy/TreeNodes/AssemblyTreeNode.cs

 					if (metadata?.IsAssembly == true && metadata.TryGetFullAssemblyName(out var assemblyName))
 					{
+#if CROSS_PLATFORM
+						tooltip.Inlines.Add(new Bold().Add(new Run("Name: ")));


The syntax 'new Bold().Add(new Run(...))' appears incorrect. The Bold class does not have an Add method that returns the Bold instance. The correct pattern would be to either use the Bold(Inline) constructor (WPF) or to add to the Inlines collection separately. This code may not compile in the CROSS_PLATFORM configuration.

Copilot · 2025-12-31T09:39:53Z

ICSharpCode.ILSpyX/TreeView/SharpTreeNodeCollection.cs

+		public int Add(object value)
+		{
+			return ((IList)list).Add(value);
+		}
+
+		public bool Contains(object value)
+		{
+			return ((IList)list).Contains(value);
+		}
+
+		public int IndexOf(object value)
+		{
+			return ((IList)list).IndexOf(value);
+		}
+
+		public void Insert(int index, object value)
+		{
+			((IList)list).Insert(index, value);
+		}
+
+		public void Remove(object value)
+		{
+			((IList)list).Remove(value);
+		}


The non-generic IList methods (Add, Insert, Remove) delegate directly to the underlying List without going through the validation and parent tracking logic present in the generic counterparts. This could lead to broken parent-child relationships and missing change notifications when these methods are called. Consider implementing these methods properly to maintain invariants, similar to how the generic versions work.

Copilot · 2025-12-31T09:39:53Z

ILSpy/SmartTextOutputExtensions.cs

+					if (icon != null)
+					{
+						button.Content = new StackPanel {
+							Orientation = Orientation.Horizontal,
+							Children = {
+								new Image { Width = 16, Height = 16, Source = icon, Margin = new Thickness(0, 0, 4, 0) },
+								new TextBlock { Text = text }
+							}
+						};
+					}
+					else
+					{
+						button.Content = text;
+					}


Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

lextm force-pushed the wpf-isolation branch from 1679429 to 3baf745 Compare December 17, 2025 17:25

lextm force-pushed the wpf-isolation branch 2 times, most recently from 1b833df to a0f38ad Compare December 17, 2025 22:06

christophwille requested a review from Copilot December 18, 2025 06:25

Copilot started reviewing on behalf of christophwille December 18, 2025 06:25 View session

Copilot AI reviewed Dec 18, 2025

View reviewed changes

lextm force-pushed the wpf-isolation branch 3 times, most recently from d571676 to 175fd60 Compare December 31, 2025 04:58

Refactor for cross platform port (icsharpcode#3641)

9e74d29

lextm force-pushed the wpf-isolation branch from 175fd60 to 9e74d29 Compare December 31, 2025 07:42

Fix whitespace

d583014

christophwille requested a review from Copilot December 31, 2025 09:34

Copilot started reviewing on behalf of christophwille December 31, 2025 09:34 View session

Copilot AI reviewed Dec 31, 2025

View reviewed changes

-			((IList)list).Remove(value);
+			// Route through the generic Remove to ensure proper notification and validation.
+			if (value == null || value is SharpTreeNode)
+			{
+				Remove((SharpTreeNode)value);
+			}


		public object SyncRoot => ((ICollection)list).SyncRoot;

		object IList.this[int index] { get => ((IList)list)[index]; set => ((IList)list)[index] = value; }

-		object IList.this[int index] { get => ((IList)list)[index]; set => ((IList)list)[index] = value; }
+		object IList.this[int index]
+		{
+			get => this[index];
+			set
+			{
+				if (value is not SharpTreeNode node)
+					throw new ArgumentException("Value must be a SharpTreeNode.", nameof(value));
+				this[index] = node;
+			}
+		}

	// TODO: ExpandAncestors(node);
	// Ancestors are intentionally not expanded automatically when selecting a node.

Uh oh!

Refactor for cross platform port (#3641) #3642

Are you sure you want to change the base?

Refactor for cross platform port (#3641) #3642

Uh oh!

Conversation

lextm commented Dec 17, 2025

Uh oh!

christophwille commented Dec 17, 2025

Uh oh!

lextm commented Dec 17, 2025

Uh oh!

lextm commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

christophwille commented Dec 18, 2025

Uh oh!

lextm commented Dec 18, 2025

Uh oh!

christophwille commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees