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.

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

SDFG Diff viewer #161

Merged
merged 20 commits into from
Sep 5, 2024
Merged

SDFG Diff viewer #161

merged 20 commits into from
Sep 5, 2024

Conversation

phschaad
Copy link
Collaborator

@phschaad phschaad commented Aug 28, 2024

This PR allows SDFGs with GUIDs on graph elements to be viewed in a diff view:

localhost_3000_

The diff view can be entered by opening an SDFG, and then selecting Compare With Other SDFG in the top tools panel. The second SDFG can be loaded, and then the diff view is created.

image

In the diff view, the first opened SDFG is considered to be the "new" graph, and is shown on the right side. The second opened SDFG is considered the "old" graph and is shown on the right.
Differences are highlighted with colors over individual graph elements. Red (in the old graph) represents elements that were removed in the new graph. Green (in the new graph) highlights newly added elements, and orange (shown in both graphs) represents elements where some property or attribute has been modified.

Opening the graph outline further highlights the changes in a linear fashion, similar to a text diff, and allows jumping to individual elements. Here, too, the new graph's outline is shown on the right, and the old graph's outline appears on the left.

localhost_3000_ (1)

Search functionality is provided similar to the regular viewer. The search query is run on both graphs, and both graphs' results are shown in the resulting search result panel, with duplicates (i.e., everything except added and removed elements) removed. Clicking results zooms to them in the respective graph, or in both graphs if a result is an unchanged element or one where properties or attributes were changed (orange).

localhost_3000_ (2)

Finally, the renderer's UI is adjusted to disallow any graph modifications, placing the SDFG(s) in a read-only mode, until the diff view is exited again through the corresponding option in the top tools panel. Changes to the renderer options carry over into and can be further adjusted in the diff view, similar to the regular SDFG view.

image

Note: the exact color hues are not finalized and will be adjusted over time based on user feedback and testing with various color schemes in embedded scenarios, such as VSCode.

@phschaad phschaad marked this pull request as ready for review September 3, 2024 09:49
@phschaad phschaad changed the title Add first prototype of SDFG Diff viewer SDFG Diff viewer Sep 3, 2024
@phschaad phschaad requested a review from tbennun September 4, 2024 09:02
Copy link
Contributor

@tbennun tbennun left a comment

Choose a reason for hiding this comment

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

The diff looks good. I would update the SDFGElement.guid() method to reflect new guids if they exist, and try to make use of the method if it's available.

@@ -254,6 +254,7 @@ export class LViewRenderer {
}

private initLocalViewSidebar(): void {
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

:(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,724 @@
// Copyright 2019-2024 ETH Zurich and the DaCe authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

File name is "viewr"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

public shadeElem(
elem: Edge | SDFGNode, ctx: CanvasRenderingContext2D
): void {
if (this.diffMap?.addedKeys.has(elem.attributes().guid))
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use the guid() method of SDFGElement here instead (and update it to prioritize the guid attribute if exists).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

Comment on lines +30 to +32
private readonly CHANGED_COLOR = 'orange';
private readonly ADDED_COLOR = 'green';
private readonly REMOVED_COLOR = 'red';
Copy link
Contributor

Choose a reason for hiding this comment

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

should this not point to the CSS so that we can modify it?

Copy link
Collaborator Author

@phschaad phschaad Sep 5, 2024

Choose a reason for hiding this comment

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

Fixed, this is now taken from the stylesheet (I revamped it a bit overall)

src/sdfv.ts Outdated Show resolved Hide resolved
Comment on lines 4104 to 4105
//reload_file(this.sdfv_instance);
// TODO: exit correctly.
Copy link
Contributor

Choose a reason for hiding this comment

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

:(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

public static async diff(
graphA: JsonSDFG, graphB: JsonSDFG
): Promise<DiffMap> {
if (!graphA.attributes.guid || !graphB.attributes.guid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use the SDFGElement's guid() method below, and when diff is called on graphs without guid here (i.e., this branch should not change) I would alert the user that diffs are not supported for graphs older than version 0.16.2 (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

That also means we have to add guid() to JsonSDFGElement? :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Found a way around it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a warning

src/renderer/renderer.ts Show resolved Hide resolved
@phschaad phschaad requested a review from tbennun September 5, 2024 11:45
@phschaad phschaad merged commit f3c39fa into master Sep 5, 2024
4 checks passed
@phschaad phschaad deleted the diff_viewer branch September 5, 2024 14:45
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.

2 participants