Extract multiple package entries#3508
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new context menu entry to extract all entries from a bundled package alongside refactoring the existing single‐entry extraction to support multiple selections and recursive folder structures.
- Introduce
ExtractAllPackageEntriesContextMenuEntryfor bulk extraction of package contents. - Refactor
ExtractPackageEntryContextMenuEntryto use pattern matching, unify save logic, and support nested folders viaGetFullPath. - Add new resource strings
ExtractAllPackageEntriesin both English and Simplified Chinese.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ILSpy/Properties/Resources.zh-Hans.resx | Added Chinese translation for “ExtractAllPackageEntries”. |
| ILSpy/Properties/Resources.resx | Added English string for “ExtractAllPackageEntries”. |
| ILSpy/Commands/ExtractPackageEntryContextMenuEntry.cs | Refactored extraction logic, added bulk‐extract class and recursion. |
Files not reviewed (1)
- ILSpy/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (3)
ILSpy/Commands/ExtractPackageEntryContextMenuEntry.cs:95
- [nitpick] The name 'GetFullPath' suggests an absolute path but it returns a relative package fragment. Consider renaming to 'GetRelativePackagePath' for clarity.
static string GetFullPath(SharpTreeNode node)
ILSpy/Commands/ExtractPackageEntryContextMenuEntry.cs:187
- [nitpick] Add XML doc comments for this new class to explain its purpose and usage in the context menu.
sealed class ExtractAllPackageEntriesContextMenuEntry(DockWorkspace dockWorkspace) : IContextMenuEntry
ILSpy/Commands/ExtractPackageEntryContextMenuEntry.cs:187
- [nitpick] There are no tests covering 'ExtractAllPackageEntriesContextMenuEntry'. Consider adding unit or integration tests for both single and nested package extraction.
sealed class ExtractAllPackageEntriesContextMenuEntry(DockWorkspace dockWorkspace) : IContextMenuEntry
| { | ||
| string name = node.Text + "\\"; | ||
| if (GetFullPath(node.Parent) is string parent) | ||
| return parent + "\\" + name; |
There was a problem hiding this comment.
GetFullPath concatenates segments with manual backslashes, which can produce duplicate separators. Consider using Path.Combine for reliable path construction.
| return parent + "\\" + name; | |
| return Path.Combine(parent, name); |
| public bool IsVisible(TextViewContext context) | ||
| { | ||
| var selectedNodes = context.SelectedTreeNodes?.OfType<AssemblyTreeNode>() | ||
| .Where(asm => asm.PackageEntry != null) ?? Enumerable.Empty<AssemblyTreeNode>(); | ||
| return selectedNodes.Any(); | ||
| if (context.SelectedTreeNodes is [AssemblyTreeNode { PackageEntry: null } asm]) | ||
| { | ||
| try | ||
| { | ||
| if (asm.LoadedAssembly.GetLoadResultAsync().GetAwaiter().GetResult().Package is { Kind: PackageKind.Bundle }) |
There was a problem hiding this comment.
[nitpick] Blocking on an async method with GetAwaiter().GetResult() can lead to deadlocks. Consider making this check asynchronous or prefetching the package kind.
@christophwille What are you doing? |
We are testing Copilot on select PRs to see what is good at / lacking / straightforward bad at to learn whether it can be useful for us in the future. (I prefer first hand experience to hearsay) |
|
Thank you for your contribution! Sorry for taking so long! |
|
@siegfriedpammer What are you doing sir? My PR work well, but your commit doesn't work at all.
WholeProjectDecompiler.SanitizeFileName(@"E:\test\my.dll") = "E.dll" // what??? Where to save? |
Fixed now, I misremembered how SanitizeFileName works, sorry for that! |
Close #3498
I see this file structure:
I don't know if following is also possible:
If it's impossible, recursion is unnecessary in GetFullPath