Skip to content

nad-viewer: Drop nad-edge-infos from the SVG in adaptiveTextZoom mode at init#424

Open
sBouzols wants to merge 5 commits into
mainfrom
fix_adaptiveTextZoom_incompatibility_svg_with_edgeInfos_at_init
Open

nad-viewer: Drop nad-edge-infos from the SVG in adaptiveTextZoom mode at init#424
sBouzols wants to merge 5 commits into
mainfrom
fix_adaptiveTextZoom_incompatibility_svg_with_edgeInfos_at_init

Conversation

@sBouzols

@sBouzols sBouzols commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • A PR or issue has been opened in all impacted repositories (if any)

Does this PR already have an issue describing the problem?

No

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

If we use a SVG file containing nad-edge-infos at initialisation with the adaptiveTextZoom mode then edgeInfos aren't written from metadata nor updated from it (no classes added etc...).
Note: This is not the common case, but could happen in the demo app for example

What is the new behavior (if this is a feature change)?
Drop nad-edge-infos from the SVG in adaptiveTextZoom mode it will be created from metadata even at init.
Add a console warning if this kind of svg file is used with this mode

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

… mode it will be created from metadata even at init.

Add a console warning if this kind of svg file is used with this mode

Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>

@CBiasuzzi CBiasuzzi 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.

Apparently, if the SVGhas already a nad-edge-infos, this fix creates a second nad-edge-infos (empty).
You can verify it, for example, if you reload the demo, without pan/zoom or anything, and look at the svg for the "Partial network with adaptive zoom" using some browser's dev tools

@massimo-ferraro massimo-ferraro 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.

Dropping the edge info section at the init is done because there could be some inconsistencies in the input SVG w.r.t. the metadata, or it's a first step toward having the adaptive zoom functionality create what is needed in the svg also when loading the page, instead of possibly getting it from the input svg content?

@sBouzols

Copy link
Copy Markdown
Contributor Author

Apparently, if the SVGhas already a nad-edge-infos, this fix creates a second nad-edge-infos (empty). You can verify it, for example, if you reload the demo, without pan/zoom or anything, and look at the svg for the "Partial network with adaptive zoom" using some browser's dev tools

Oupsy you right. I would like to remove the existing nad-edge-infos section and create an empty one. IT will be populated from metadata at init if adaptiveTextZoom mode is activated.

Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>

@CBiasuzzi CBiasuzzi 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.

with the latest update in init, the section is not duplicated, anymore.
However, a comment about this change: first, it removes the existing nad-edge-infos; then, it recreates the section, appending it after the other existing sections. Still using the "Partial network with adaptive zoom" as an example:

these are the sections in the SVG file, in order:

  • nad-vl-nodes
  • nad-branch-edges
  • nad-edge-infos
  • nad-text-edges
  • nad-text-nodes

these are the sections, after the updated init ran:

  • nad-vl-nodes
  • nad-branch-edges
  • nad-text-edges
  • nad-text-nodes
  • nad-edge-infos

The effect is that with the original SVG, the text boxes are displayed above the other sections, while with the new init it's the edge-infos to be apparently at the top.

So far the nad-edge-infos section has always been in that position in the SVGs, and the adaptive zoom code has never actually removed and recreated it (it simply emptied it).

@sonarqubecloud

Copy link
Copy Markdown

@sBouzols

Copy link
Copy Markdown
Contributor Author

with the latest update in init, the section is not duplicated, anymore. However, a comment about this change: first, it removes the existing nad-edge-infos; then, it recreates the section, appending it after the other existing sections. Still using the "Partial network with adaptive zoom" as an example:

these are the sections in the SVG file, in order:

* nad-vl-nodes

* nad-branch-edges

* nad-edge-infos

* nad-text-edges

* nad-text-nodes

these are the sections, after the updated init ran:

* nad-vl-nodes

* nad-branch-edges

* nad-text-edges

* nad-text-nodes

* nad-edge-infos

The effect is that with the original SVG, the text boxes are displayed above the other sections, while with the new init it's the edge-infos to be apparently at the top.

So far the nad-edge-infos section has always been in that position in the SVGs, and the adaptive zoom code has never actually removed and recreated it (it simply emptied it).

But the previous getOrCreateEdgeInfosSection worked the same, creating a nad-edge-infos node and appending it at the end.
Would you prefer a call to .replaceChildren() then and keep the same order ?
In current version with adaptativeZoomMode and no nad-edge-infos then it will be appended at the end, then do we need to fix this as well ?

@CBiasuzzi

Copy link
Copy Markdown
Contributor

I just wanted to point out the potential issue of having different ordering for the edge-infos. The question is: is it actually a problem for a client using the viewer?

@CBiasuzzi

Copy link
Copy Markdown
Contributor

This version below should keep the order of the three sections as it was intended.
Here, getOrCreateTextEdgesSection and getOrCreateTextNodesSection are called in the correct order.
Besides, the two insertBefore should take care of e.g., the case where nad-edge-infos is not already available and created in the else branch.
What do you think?

this.textEdgesSection = this.getOrCreateTextEdgesSection();
this.textNodesSection = this.getOrCreateTextNodesSection();

//make sure that text-edges precedes text-nodes, to match the order used by powsybl-diagram's SvgWriter
this.innerSvg?.insertBefore(this.textEdgesSection, this.textNodesSection ?? null);

// Ignore nad-edge-infos from the SVG in the AdaptiveTextZoom mode at init
// It will be fullfilled later in the updateAdaptiveEdgeInfos
if (this.nadViewerParameters.getAdaptiveTextZoom().enabled) {
	// if the nad-edge-infos section exists in the SVG, empty it so that the position is preserved
	const existingEdgeInfos = <SVGElement>this.innerSvg?.querySelector(':scope > g.nad-edge-infos');
	if (existingEdgeInfos) {
		existingEdgeInfos.replaceChildren();
		console.warn(
			'AdaptiveTextZoom mode activated: creating nad-edge-infos from metadata and ignoring it from the SVG'
		);
		this.edgeInfosSection = existingEdgeInfos;
	} else {
		this.edgeInfosSection = this.createEmptyEdgeInfosSection();
	}
} else {
	this.edgeInfosSection = this.getOrCreateEdgeInfosSection();
}

//make sure that edge-infos precedes text-edges (and consequently text-nodes), to match the order used by powsybl-diagram's SvgWriter
this.innerSvg?.insertBefore(this.edgeInfosSection, this.textEdgesSection ?? null);

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