Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Hide the navigation nodes for unmodified nodes in diff #9015

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Hide the navigation nodes for unmodified nodes in diff #9015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/dotnet/APIView/APIView/Model/CodeFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ public void ConvertToTreeTokenModel()
List<ReviewToken> currentLineTokens = new List<ReviewToken>();
foreach(var oldToken in Tokens)
{
//Don't include documentation in converted code file due to incorrect documentation formatting used in previous model.
Copy link
Member

Choose a reason for hiding this comment

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

how did you find out this discrepancy? are there any automated tests we can run to keep this tool up to date with the design (as it changes)?

Copy link
Member

@chidozieononiwu chidozieononiwu Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also couldn't you convert the old documentation to new. Seem like that would be a straightforward mapping? Would be weird to just exclude documentation.

if (isDocumentation && oldToken.Kind != CodeFileTokenKind.DocumentRangeEnd)
continue;
ReviewToken token = null;
switch(oldToken.Kind)
{
Expand Down Expand Up @@ -270,6 +273,13 @@ public void ConvertToTreeTokenModel()
reviewLine.parentLine = parent;
}

//Handle specific cases for C++ line with 'public:' and '{' to mark related line
Copy link
Member

Choose a reason for hiding this comment

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

is there another way we can guarantee this is only for c++? my worry is that another parser might have something similar and we don't know about and they run this tool and get a behavior that is not accurate

if ((currentLineTokens.Count == 1 && currentLineTokens.First().Value == "{") ||
(currentLineTokens.Count == 2 && currentLineTokens.Any(t => t.Kind == TokenKind.Keyword && t.Value == "public")))
{
reviewLine.RelatedToLine = previousLine?.LineId;
}

if (currentLineTokens.Count == 0)
{
//Empty line. So just add previous line as related line
Expand Down
1 change: 1 addition & 0 deletions src/dotnet/APIView/ClientSPA/src/app/_models/navigationTreeModels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ export class NavigationTreeNode {
data: NavigationTreeNodeData = new NavigationTreeNodeData();
expanded: boolean = false;
children: NavigationTreeNode[] = [];
visible: boolean = true;
}
30 changes: 24 additions & 6 deletions src/dotnet/APIView/ClientSPA/src/app/_workers/apitree-builder.worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,22 @@ addEventListener('message', ({ data }) => {
if (!codePanelData?.hasDiff) {
apiTreeBuilderData!.diffStyle = FULL_DIFF_STYLE; // If there is no diff nodes and tree diff will not work
}

if (codePanelData?.navigationTreeNodes && codePanelData?.navigationTreeNodes.length > 0)
{
isNavigationTreeCreated = true;
navigationTree = codePanelData?.navigationTreeNodes;
}

buildCodePanelRows("root", navigationTree);
const codePanelRowDataMessage : InsertCodePanelRowDataMessage = {
directive: ReviewPageWorkerMessageDirective.UpdateCodePanelRowData,
payload: codePanelRowData
};

if (codePanelData?.navigationTreeNodes && codePanelData?.navigationTreeNodes.length > 0)
{
isNavigationTreeCreated = true;
navigationTree = codePanelData?.navigationTreeNodes;
//Remove navigation nodes for nodes that are not visible in diff style view
navigationTree.forEach(node => FilterVisibleNavigationNodes(node));
navigationTree = navigationTree.filter(n => n.visible);
}

postMessage(codePanelRowDataMessage);

const navigationTreeMessage : InsertCodePanelRowDataMessage = {
Expand Down Expand Up @@ -259,4 +263,18 @@ function shouldAppendIfRowIsHiddenAPI(row: CodePanelRowData) {
} else {
return true;
}
}

function FilterVisibleNavigationNodes(node: NavigationTreeNode) {
Copy link
Member

Choose a reason for hiding this comment

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

Should you make this private?

// Recursively perform a bottom up traversal and trim down any invisible nodes
if (node.children) {
for (let child of node.children) {
FilterVisibleNavigationNodes(child);
}
node.children = node.children.filter(c => c.visible);
}

if (visibleNodes.has(node.data.nodeIdHashed) || (node.children && node.children.some(c => c.visible))) {
node.visible = true;
}
}