Skip to content

Conversation

@sonyps5201314
Copy link
Contributor

Added support for multi-select batch PDB generation. When multiple assemblies are selected, the user is prompted to choose a target folder, and files are generated as {ShortName}.pdb based on the assembly's short name.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for batch PDB generation in ILSpy, allowing users to select multiple assemblies and generate PDB files for all of them at once. When multiple assemblies are selected, users are prompted to choose a target folder, and PDB files are generated with filenames based on each assembly's short name.

Key Changes:

  • Extended context menu and main menu entries to support multiple assembly selection
  • Added new GeneratePdbForAssemblies method for batch processing
  • Added localized resource strings for batch operation feedback

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.

File Description
ILSpy/Commands/GeneratePdbContextMenuEntry.cs Implements batch PDB generation logic with multi-select support for both context menu and main menu entries
ILSpy/Properties/Resources.resx Adds English resource strings for batch generation status messages
ILSpy/Properties/Resources.zh-Hans.resx Adds Chinese translations for new resource strings
ILSpy/Properties/Resources.Designer.cs Auto-generated designer code for accessing new resource strings
Files not reviewed (1)
  • ILSpy/Properties/Resources.Designer.cs: Language not supported

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

if (options.Progress != null)
{
options.Progress.Report(new DecompilationProgress {
Title = "Generating portable PDB...",
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The progress title "Generating portable PDB..." is hardcoded and not localized. Consider adding this string to the resource files for proper internationalization, similar to other user-facing messages in this file.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sonyps5201314 if you want, you can add this. There are other locations where this same string is used, that might need updating as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

71cfce3 resolved.

output.WriteLine();
output.WriteLine(Resources.GenerationCompleteInSeconds, totalWatch.Elapsed.TotalSeconds.ToString("F1"));
output.WriteLine();
output.AddButton(null, Resources.OpenExplorer, delegate { Process.Start("explorer", "/select,\"" + targetFolder + "\""); });
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using /select with a folder path (targetFolder) will not work as intended. The /select parameter expects a file path to select in Explorer. To open the folder directly, use:

Process.Start("explorer", "\"" + targetFolder + "\"");

This will open the target folder itself, which is more appropriate for a batch operation where multiple files were generated.

Suggested change
output.AddButton(null, Resources.OpenExplorer, delegate { Process.Start("explorer", "/select,\"" + targetFolder + "\""); });
output.AddButton(null, Resources.OpenExplorer, delegate { Process.Start("explorer", "\"" + targetFolder + "\""); });

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sonyps5201314 Yes, this a legitimate bug: explorer C:\path\to\folder is the syntax for folders.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1f13b80 resolved.
This also incidentally resolves another small 'unmodern' issue in ILSpy: every time 'Open or Select File/Folder' was executed, a new explorer.exe process was launched, and these processes would not automatically exit even after closing the opened windows.
image

@siegfriedpammer
Copy link
Member

Looks good to me! Thank you very much for contributing this feature!

@sonyps5201314
Copy link
Contributor Author

@siegfriedpammer I believe I have addressed all the points you mentioned. The functionality of GeneratePdbForAssembly is now fully covered by GeneratePdbForAssemblies, so GeneratePdbForAssembly can be removed to simplify maintenance.

Additionally, I intentionally did not set an initial directory for OpenFolderDialog because I prefer to use the user's last selected directory. If we default to the target DLL's directory, there is a risk of overwriting an existing official PDB, which could break the software's integrity verification.

@@ -0,0 +1,152 @@
using System;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs license header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e027a1a resolved.

}
}
if (itemPidl != IntPtr.Zero)
CoTaskMemFree(itemPidl);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this needs try-finally too?

}
catch
{
// ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what kind of exceptions are we ignoring here? "catch all" is considered bad pratice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3619 (comment) answered.

}
catch
{
// ignore
Copy link
Member

@siegfriedpammer siegfriedpammer Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what kind of exceptions are we ignoring here? "catch all" is considered bad practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike C, which typically uses return values to signal errors, C# is a language that relies heavily on exceptions. Since almost any method call has the potential to throw, adding a try-catch block is certainly an effective measure to prevent the application from crashing.

These potential exceptions include Process.Start failures, DllImport("shell32.dll") errors (such as failing to load the DLL or finding the specific entry point), and even managed exceptions thrown from within P/Invoke during extreme scenarios (e.g., when hooked by tools like EasyHook).

Given that the consequence of a failure in ShellHelper's public API is merely that a folder fails to open—which is a minor impact—I believe adding a general exception handler is acceptable here. (Actually, I didn't add these catch blocks myself; I simply retained them because I agree they are necessary.)

using System.Runtime.InteropServices;
using System.Linq;
using System.Collections.Generic;

Copy link
Member

@siegfriedpammer siegfriedpammer Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#pragma warning disable CA1060 // Move pinvokes to native methods class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't actually get this warning when I compiled, but I still added it as you suggested.
b631b55 resolved.

{
if (context.SelectedTreeNodes == null)
return;
var paths = new List<string>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it would make sense to use a HashSet here? Note that this would need a StringComparer.OrdinalIgnoreCase on Windows :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a change is strictly necessary here. If multiple instances of the same file appear in the Assembly List, it implies that the insertion logic (AssemblyList.OpenAssembly) isn't handling things correctly. The restriction should be enforced at that layer, rather than becoming a burden for internal calls.

Given that AssemblyList defines:
readonly Dictionary<string, LoadedAssembly> byFilename = new Dictionary<string, LoadedAssembly>(StringComparer.OrdinalIgnoreCase);
we can infer that ILSpy doesn't suffer from this issue. However, I did notice a few days ago that dnSpyEx indeed has a problem with duplicate file entries.

That being said, your point is valid. Since relying on external restrictions increases the cognitive load on the caller (requiring the user to pay special attention), I went ahead and handled it inside OpenFolderAndSelectItems.
752e6ed resolved.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 17 comments.

Files not reviewed (1)
  • ILSpy/Properties/Resources.Designer.cs: Language not supported

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

internal static void GeneratePdbForAssemblies(IEnumerable<LoadedAssembly> assemblies, LanguageService languageService, DockWorkspace dockWorkspace)
{
var assemblyArray = assemblies?.Where(a => a != null).ToArray() ?? [];
if (assemblyArray == null || assemblyArray.Length == 0)
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redundant null check can be simplified. Line 123 already filters out null values with assemblies?.Where(a => a != null).ToArray() ?? [], so the subsequent check if (assemblyArray == null || assemblyArray.Length == 0) will never have a null assemblyArray.

Suggested change
if (assemblyArray == null || assemblyArray.Length == 0)
if (assemblyArray.Length == 0)

Copilot uses AI. Check for mistakes.
Comment on lines +148 to +150
string msg = string.Format(Resources.CannotCreatePDBFile, ":" + Environment.NewLine +
string.Join(Environment.NewLine, unsupported.Select(u => Path.GetFileName(u.FileName)))
+ Environment.NewLine);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message formatting is incorrect. The first parameter to string.Format should be just the assembly filename, not a formatted string with additional content. This will result in a malformed error message like "Cannot create PDB file for :
Assembly1.dll
Assembly2.dll
because the PE debug directory type 'CodeView' is missing."

Consider changing this to properly format the list of unsupported assemblies, for example:

string msg = Resources.CannotCreatePDBFile + Environment.NewLine +
    string.Join(Environment.NewLine, unsupported.Select(u => "  - " + Path.GetFileName(u.FileName)));
MessageBox.Show(msg);
Suggested change
string msg = string.Format(Resources.CannotCreatePDBFile, ":" + Environment.NewLine +
string.Join(Environment.NewLine, unsupported.Select(u => Path.GetFileName(u.FileName)))
+ Environment.NewLine);
string msg = Resources.CannotCreatePDBFile + Environment.NewLine +
string.Join(Environment.NewLine, unsupported.Select(u => " - " + Path.GetFileName(u.FileName)));

Copilot uses AI. Check for mistakes.
Comment on lines 33 to 45
IntPtr folderPidl = IntPtr.Zero;
uint attrs;
int hr = SHParseDisplayName(folderPath, IntPtr.Zero, out folderPidl, 0, out attrs);
if (hr == 0 && folderPidl != IntPtr.Zero)
{
SHOpenFolderAndSelectItems(folderPidl, 0, null, 0);
CoTaskMemFree(folderPidl);
}
else
{
// fallback
Process.Start(new ProcessStartInfo { FileName = folderPath, UseShellExecute = true });
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this call with a call to managed code if possible.

Suggested change
IntPtr folderPidl = IntPtr.Zero;
uint attrs;
int hr = SHParseDisplayName(folderPath, IntPtr.Zero, out folderPidl, 0, out attrs);
if (hr == 0 && folderPidl != IntPtr.Zero)
{
SHOpenFolderAndSelectItems(folderPidl, 0, null, 0);
CoTaskMemFree(folderPidl);
}
else
{
// fallback
Process.Start(new ProcessStartInfo { FileName = folderPath, UseShellExecute = true });
}
Process.Start(new ProcessStartInfo { FileName = folderPath, UseShellExecute = true });

Copilot uses AI. Check for mistakes.
Comment on lines 33 to 45
IntPtr folderPidl = IntPtr.Zero;
uint attrs;
int hr = SHParseDisplayName(folderPath, IntPtr.Zero, out folderPidl, 0, out attrs);
if (hr == 0 && folderPidl != IntPtr.Zero)
{
SHOpenFolderAndSelectItems(folderPidl, 0, null, 0);
CoTaskMemFree(folderPidl);
}
else
{
// fallback
Process.Start(new ProcessStartInfo { FileName = folderPath, UseShellExecute = true });
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this call with a call to managed code if possible.

Suggested change
IntPtr folderPidl = IntPtr.Zero;
uint attrs;
int hr = SHParseDisplayName(folderPath, IntPtr.Zero, out folderPidl, 0, out attrs);
if (hr == 0 && folderPidl != IntPtr.Zero)
{
SHOpenFolderAndSelectItems(folderPidl, 0, null, 0);
CoTaskMemFree(folderPidl);
}
else
{
// fallback
Process.Start(new ProcessStartInfo { FileName = folderPath, UseShellExecute = true });
}
// Use managed code to open the folder in Explorer
Process.Start(new ProcessStartInfo { FileName = folderPath, UseShellExecute = true });

Copilot uses AI. Check for mistakes.
if (hr == 0 && folderPidl != IntPtr.Zero)
{
SHOpenFolderAndSelectItems(folderPidl, 0, null, 0);
CoTaskMemFree(folderPidl);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this call with a call to managed code if possible.

Copilot uses AI. Check for mistakes.
static class ShellHelper
{
[DllImport("shell32.dll", CharSet = CharSet.Unicode)]
static extern int SHParseDisplayName([MarshalAs(UnmanagedType.LPWStr)] string pszName, IntPtr pbc, out IntPtr ppidl, uint sfgaoIn, out uint psfgaoOut);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimise the use of unmanaged code.

Copilot uses AI. Check for mistakes.
static extern int SHParseDisplayName([MarshalAs(UnmanagedType.LPWStr)] string pszName, IntPtr pbc, out IntPtr ppidl, uint sfgaoIn, out uint psfgaoOut);

[DllImport("shell32.dll")]
static extern int SHOpenFolderAndSelectItems(IntPtr pidlFolder, uint cidl, [MarshalAs(UnmanagedType.LPArray)] IntPtr[] apidl, uint dwFlags);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimise the use of unmanaged code.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +19
[DllImport("shell32.dll")]
static extern IntPtr ILFindLastID(IntPtr pidl);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimise the use of unmanaged code.

Suggested change
[DllImport("shell32.dll")]
static extern IntPtr ILFindLastID(IntPtr pidl);

Copilot uses AI. Check for mistakes.
static extern IntPtr ILFindLastID(IntPtr pidl);

[DllImport("ole32.dll")]
static extern void CoTaskMemFree(IntPtr pv);
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimise the use of unmanaged code.

Copilot uses AI. Check for mistakes.
// Ask for target folder
var dlg = new OpenFolderDialog();
dlg.Title = Resources.SelectPDBOutputFolder;
if (dlg.ShowDialog() != true || string.IsNullOrWhiteSpace(dlg.FolderName))
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression 'A != true' can be simplified to '!A'.

Suggested change
if (dlg.ShowDialog() != true || string.IsNullOrWhiteSpace(dlg.FolderName))
if (!dlg.ShowDialog() || string.IsNullOrWhiteSpace(dlg.FolderName))

Copilot uses AI. Check for mistakes.
@sonyps5201314
Copy link
Contributor Author

@siegfriedpammer Thank you for taking the time to review the code again. I have addressed all your questions and made the necessary changes. Is there anything else that needs modification?

@siegfriedpammer siegfriedpammer merged commit 193a463 into icsharpcode:master Nov 25, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants