-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Navigate visible history #3591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Navigate visible history #3591
Conversation
|
Why do we need a |
|
Because it differs from what is in the tree, it shows full types, members including type, and assembly name for some of the folders (screenshot first item) |
|
I can also imagine it could be a method where you put current state and the text would differ on where you are (like same namespace or same assembly) |
|
I think I am fine with that. Does need to be in the |
|
It's also important to use the settings-aware implementation for the node text that is used everywhere else. |
|
It has a default implementation returning You can see in the changed files that it follows what With that said, I don't mind using |
|
Maybe put the folder in parentheses? I am not sure. I can have a closer look when I get home. Reviewing code on the phone is not ideal. 😅 |
|
I put the folder first and where it belongs in parentheses, looks better, see the updated screenshot. |
|
I also tried to include module name in the metadata nodes but feel free to skip that commit. Done working on this PR until there is feedback to address. |
|
One thought I had: is it possible to get an actual dropdown toolbar button in WPF? Is this only included in some community library? |
|
There is no built-in dropdown button in WPF. I considered creating a control for it (or retemplating combobox) but since the theme and styles are external it didn't seem worth it. We could also just have it as context menu on the normal buttons and that's it. |
| : metadata.GetString(metadata.GetModuleDefinition().Name); | ||
| if (metadata.IsAssembly) | ||
| value = metadata.GetString(metadata.GetAssemblyDefinition().Name); | ||
| else if (metadata.DebugMetadataHeader == null) // standalone debug metadata does not contain module table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should check Kind instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GetModuleDefinition throws on non-null DebugMetadataHeader, so that felt like an appropriate check before invoking it.
I don't mind changing it to Kind check, but I would need some hint about which Kinds guarantee DebugMetadataHeader, as I don't know how to produce e.g. WebCIL to check.
|
Yeah I am aware of that. I thought it is a rare if not adverse scenario where you would jump through the same nodes in different assemblies. I thought about adding the top-level info, but then all the items become cluttered with full assembly name for minimal benefit in rare cases. Next thing you come with will be same assembly names using different file locations, so I think the line needs to be drawn somewhere. This is not intended to replace user's memory. You still know what you have been doing recently so you will have an idea where the items come from. With that said, with the I could also do some analysis when building the history list, and if the previous item "looks the same" but has different top level, append the top level. |
|
You are right. It's probably fine the way it's now. |
There was a problem hiding this 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 implements feature #3582 to add visible navigation history to the Back/Forward toolbar buttons. The implementation adds dropdown menus to these buttons that display a list of recently visited navigation states, allowing users to jump directly to a specific point in their navigation history rather than clicking through each step sequentially.
Key Changes:
- Introduces
NavigationTextproperty to tree nodes for context-rich display in navigation history - Adds dropdown menu support to toolbar commands via the new
IProvideParameterListinterface - Enhances navigation commands (Back/Forward) with parameterized history navigation
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| ICSharpCode.ILSpyX/TreeView/SharpTreeNode.cs | Adds base NavigationText property that defaults to Text |
| ILSpy/NavigationState.cs | Implements NavigationText property to format navigation history display |
| ILSpy/NavigationHistory.cs | Exposes BackList and ForwardList properties for UI consumption |
| ILSpy/AssemblyTree/AssemblyTreeModel.cs | Updates NavigateHistory to support jumping to specific state |
| ILSpy/Commands/SimpleCommand.cs | Adds IProvideParameterList interface for toolbar dropdown support |
| ILSpy/Commands/BrowseBackCommand.cs | Implements IProvideParameterList to provide back history list |
| ILSpy/Commands/BrowseForwardCommand.cs | Implements IProvideParameterList to provide forward history list |
| ILSpy/Controls/MainToolBar.xaml.cs | Implements dropdown menu UI for commands with parameter lists |
| ILSpy/Themes/ThemeManager.cs | Adds CreateToolBarToggleButtonStyle method for toggle button styling |
| ILSpy/TreeNodes/*.cs (multiple files) | Overrides NavigationText to include contextual information (type names, parent assembly, etc.) |
| ILSpy/Metadata/*.cs (multiple files) | Overrides NavigationText to include module/file name context |
| ILSpy/Properties/Resources.* | Adds "Referenced Types" localized resource string |
| ICSharpCode.Decompiler/Metadata/MetadataFile.cs | Handles name retrieval for standalone debug metadata without module table |
Files not reviewed (1)
- ILSpy/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
ILSpy/NavigationState.cs:78
- Default 'ToString()': NavigationState inherits 'ToString()' from 'Object', and so is not suitable for printing.
return ToString();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| static void PrepareParameterList(ContextMenu menu) | ||
| { | ||
| const int maximumParameterListCount = 20; |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded limit of 20 items should be extracted as a named constant at the class level for better maintainability. This makes it easier to adjust the limit and makes the purpose clearer.
|
|
||
| public interface IProvideParameterList | ||
| { | ||
| IEnumerable ParameterList { get; } |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IProvideParameterList interface is missing XML documentation comments. Consider adding documentation to explain the purpose of the interface, when it should be implemented, and what each member returns. This would help future developers understand how to use this interface for toolbar dropdown menus.
| public interface IProvideParameterList | |
| { | |
| IEnumerable ParameterList { get; } | |
| /// <summary> | |
| /// Provides a list of parameters for use in toolbar dropdown menus. | |
| /// Implement this interface when you want to supply a list of selectable parameters | |
| /// (e.g., for a toolbar dropdown) and provide display text for each parameter. | |
| /// </summary> | |
| public interface IProvideParameterList | |
| { | |
| /// <summary> | |
| /// Gets the list of parameters to be displayed in the dropdown menu. | |
| /// </summary> | |
| IEnumerable ParameterList { get; } | |
| /// <summary> | |
| /// Returns the display text for the specified parameter. | |
| /// </summary> | |
| /// <param name="parameter">The parameter for which to get the display text.</param> | |
| /// <returns>A string representing the display text for the parameter.</returns> |
| parameterBinding.ParameterBinding); | ||
| } | ||
|
|
||
| if (command is IProvideParameterList parameterList) |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to parameterList is useless, since its value is never read.
| if (command is IProvideParameterList parameterList) | |
| if (command is IProvideParameterList) |
Co-authored-by: Copilot <[email protected]>
|
I addressed what I intend to. |
|
I ran another quick test and I think we can merge it like this. Thanky you very much for your contribution! |

Implements #3582 to show history items:
@aybe