Skip to content

Fix the "Use nested namespace structure" option#3879

Merged
siegfriedpammer merged 1 commit into
icsharpcode:masterfrom
ds5678:fix-nested-namespace-nodes
Jul 17, 2026
Merged

Fix the "Use nested namespace structure" option#3879
siegfriedpammer merged 1 commit into
icsharpcode:masterfrom
ds5678:fix-nested-namespace-nodes

Conversation

@ds5678

@ds5678 ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Problem

image

Not all the namespaces are showing when "Use nested namespace structure" is enabled. I think the problem started around the time that the UI was shifted to Avalonia, but I'm not really sure.

Solution

Previously, IsHidden was being calculated as true because a namespace node momentarily had no children. This change reverses the order of adding to the tree from Top-Down to Bottom-Up.

Follow-up issue

I'm investigating why the namespaces are misordered, or more specifically the best way to fix the ordering.

image

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes missing namespace nodes when “Use nested namespace structure” is enabled by changing how the nested namespace tree is constructed so that intermediate namespace nodes don’t get incorrectly marked hidden during the filter cascade.

Changes:

  • Reworked nested namespace construction from a recursive top-down approach to an iterative bottom-up approach.
  • Ensured child namespace nodes are attached before their parent is inserted into the next level up, preventing transient “no children” states that could set IsHidden = true.

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

@ds5678

ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

I resolved the follow-up issue I mentioned.

image image

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread ILSpy/TreeNodes/AssemblyTreeNode.cs Outdated
Comment thread ILSpy/TreeNodes/NamespaceTreeNode.cs Outdated
Comment thread ILSpy/TreeNodes/NamespaceTreeNode.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread ILSpy/TreeNodes/NamespaceTreeNode.cs Outdated
Comment on lines +84 to +88
var metadata = module.Metadata;
var types = metadata.TypeDefinitions
.Where(t => {
var td = metadata.GetTypeDefinition(t);
return td.GetDeclaringType().IsNil
&& metadata.GetString(td.Namespace) == fullName;
})
.OrderBy(t => metadata.GetString(metadata.GetTypeDefinition(t).Name), NaturalStringComparer.Instance);

foreach (var t in types)
Children.Add(new TypeTreeNode(t, module));
var newChildren = new List<(string, ILSpyTreeNode)>();
var childNamespaces = new HashSet<string>(StringComparer.Ordinal);
var useNestedNamespaceNodes = TryGetUseNestedNamespaceNodes();
foreach (var t in metadata.TypeDefinitions)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ds5678 I think this is a valid concern... if I remember correctly, the old WPF code handled the namespaces once in AssemblyTreeNode and cached them. Maybe we could get a lazy+cached behavior by moving the logic to AssemblyTreeNode?

Comment thread ILSpy/TreeNodes/NamespaceTreeNode.cs Outdated
@siegfriedpammer

Copy link
Copy Markdown
Member

The display fix looks correct: top-level nodes plus lazy per-node sub-namespace discovery avoids the transient empty-children → IsHidden latch in ApplyFilterToChild (Children.All(...) is true on an empty set), and the resulting tree shape matches the pre-Avalonia release/10.1 structure. The namefullName fix in Decompile is also right.

One issue that isn't in this diff but that this change surfaces: type and namespace navigation is broken in nested mode. TreeNodeLocator.FindTreeNode — used by hyperlink clicks in the decompiler view, search-result activation, and JumpToType — resolves entities against the assembly node's direct children by full namespace name, and never descends the hierarchy:

  • FindTypeNode (TreeNodeLocator.cs:180): n.Name == top.Namespace. In nested mode the direct children are first-segment nodes ("System"), while top.Namespace is the full string ("System.Collections.Generic"), so it returns null; and even on a match it reads ns.Children.OfType<TypeTreeNode>() without walking System → Collections → Generic.
  • FindNamespaceNode (TreeNodeLocator.cs:163) and AssemblyTreeNode.FindNamespaceNode (used by --navigateto N:…) match direct children by full name too.

Repro: with nested mode on, click a hyperlink to System.Collections.Generic.List<T> (or activate a search result, or use a Base/Derived-type jump) — nothing gets selected. Flat mode works, and one-segment namespaces work. This is pre-existing (release/10.1 resolved from a full-name dictionary at any depth), but it was masked while nested display was broken, so completing the display makes it reachable. The added test covers display only, not navigation.

Not necessarily a blocker for the display fix itself, but worth a decision: fix navigation here so nested mode is fully functional, or track it as an explicit follow-up?

(Review written by an agent on Siegfried's behalf.)

@ds5678

ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Thoughts on my latest commit? It functionally changes whether or not namespaces get hidden, which is why I'm not sure if it's okay or if we need to solve the problem a different way.

@ds5678

ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Not necessarily a blocker for the display fix itself, but worth a decision: fix navigation here so nested mode is fully functional, or track it as an explicit follow-up?

I strongly care about navigation and want to help fix it.

@siegfriedpammer

Copy link
Copy Markdown
Member

Not necessarily a blocker for the display fix itself, but worth a decision: fix navigation here so nested mode is fully functional, or track it as an explicit follow-up?

I strongly care about navigation and want to help fix it.

@ds5678 sorry for my AI generated comment above, didn't want to be disrespectful.

Yes, please, if you could contribute this it would be very nice. Unfortunately, there are still divergencies between 10.1 and master no matter how many times I go over it/or let Claude cross check...

IIRC the old behavior was to eagerly construct the namespaces on load...

@ds5678

ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

IIRC the old behavior was to eagerly construct the namespaces on load...

Is this still desirable? Without 0df02ac, I get a significant lag the first time I expand the tree view.

@ds5678

ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Not necessarily a blocker for the display fix itself, but worth a decision: fix navigation here so nested mode is fully functional, or track it as an explicit follow-up?

I strongly care about navigation and want to help fix it.

Yes, please, if you could contribute this it would be very nice.

I think I already did in 6ea5920, ea2bebb, and 89684d6

@ds5678

ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

IIRC the old behavior was to eagerly construct the namespaces on load...

Is this still desirable? Without 0df02ac, I get a significant lag the first time I expand the tree view.

Presumably because the current method of populating namespace node children takes N^2 time, where N is the number of types in the module.

Note: my module has roughly 17 thousand types.

@siegfriedpammer

Copy link
Copy Markdown
Member

IIRC the old behavior was to eagerly construct the namespaces on load...

Is this still desirable? Without 0df02ac, I get a significant lag the first time I expand the tree view.

Presumably because the current method of populating namespace node children takes N^2 time, where N is the number of types in the module.

Note: my module has roughly 17 thousand types.

Is the performance in 10.1 better?

@ds5678

ds5678 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

IIRC the old behavior was to eagerly construct the namespaces on load...

Is this still desirable? Without 0df02ac, I get a significant lag the first time I expand the tree view.

Presumably because the current method of populating namespace node children takes N^2 time, where N is the number of types in the module.
Note: my module has roughly 17 thousand types.

Is the performance in 10.1 better?

Yes, I do not notice any lag in 10.1

@siegfriedpammer

Copy link
Copy Markdown
Member

I was referring to 10.1 as the "old behavior". So, if we were going to copy the exact source code in 10.1, would all your problems be solved or did you find more bugs? I guess, AssemblyTreeNode.LoadChildren is not too inefficient on 10.1?

@siegfriedpammer

siegfriedpammer commented Jul 16, 2026

Copy link
Copy Markdown
Member

Hey ... I hope you don't mind, that I will restore the structure of the WPF version (release/10.1) where the main handling code is in AssemblyTreeNode and then apply your additional fixes on top of that. So, I'd rebase your PR later...

Also, thank you very much for finding and investigating this performance regression. Very much appreciated!

@ds5678

ds5678 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

I don't mind at all. Thank you for helping with this.

In nested-namespace mode a NamespaceTreeNode's display label is only its
last segment ("Generic"), while the full dotted path
("System.Collections.Generic") is what identifies the namespace in
metadata and to the docs site. Three call sites read the label where they
need the full path, so in nested mode each targets the wrong namespace:
decompiling a namespace node queries an empty one and titles the output
after the last segment; the MSDN URL points at the wrong page; and
scope-search-to-namespace scopes to the wrong name.

These are ds5678's fixes from icsharpcode#3879, reintegrated on top of the eager
namespace rebuild. icsharpcode#3879's other Name -> FullName corrections, in
AssemblyTreeNode.FindNamespaceNode and TreeNodeLocator, are already
covered here by the full-namespace-name and type-handle indexes, so only
the Decompile and search-entry cases carry over.

Assisted-by: Claude:claude-opus-4-8:Claude Code
@siegfriedpammer
siegfriedpammer force-pushed the fix-nested-namespace-nodes branch from 89684d6 to 217fc00 Compare July 17, 2026 16:33
@ds5678

ds5678 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Is there anything you need from me?

@siegfriedpammer
siegfriedpammer merged commit c40c299 into icsharpcode:master Jul 17, 2026
7 checks passed
@siegfriedpammer

Copy link
Copy Markdown
Member

Is there anything you need from me?

No, thank you for reporting this and drafting a fix!

@ds5678
ds5678 deleted the fix-nested-namespace-nodes branch July 17, 2026 18:53
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