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.

…ShowOptionsCommand (icsharpcode#3641)

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

lextm force-pushed the wpf-isolation branch from 3baf745 to 1b833df Compare December 17, 2025 19:03

Refactor for cross platform port (icsharpcode#3641)

a0f38ad

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

lextm added 2 commits December 17, 2025 21:55

Add safe guard and clean up (icsharpcode#3641)

c1178a2

Add IList implementation for cross platform binding (icsharpcode#3641)

a1a89b1

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 added 3 commits December 18, 2025 11:20

Move option page interfaces and command out (icsharpcode#3641)

da9b0ef

Implement asynchronous dialog handling for cross-platform support in …

2ce2851

…ShowOptionsCommand (icsharpcode#3641)

Revert a few changes (icsharpcode#3641)

168eebe

-			((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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants