Introduce an ID provider for SVGs#774
Conversation
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
Signed-off-by: Florian Dupuy <florian.dupuy@rte-france.com>
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
…+ fix WireMetadata Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
|
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
…eDiagramTest` Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
|
github-project-automation
Bot
moved this from Waiting for review
to Approved
in Release 06/2026
# Conflicts: # single-line-diagram/single-line-diagram-core/src/main/java/com/powsybl/sld/SingleLineDiagram.java
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
|
|
|
||
| @Override | ||
| public String getOrCreateSvgId(String equipmentId, String subType) { | ||
| return getOrCreateId(String.format("equipmentId_%s_subType_%s", equipmentId, subType)); |
There was a problem hiding this comment.
Putting information into a String is bad design... And it could lead to conflicts. For instance, in this case you've got the problem of an equipmentId "ALDJF_subtype_AER" (leading to entry "equipmentId_ALDJF_subtype_AER") clashing with another equipment id "ALDJF" with subtype "AER" (leading again to entry "equipmentId_ALDJF_subtype_AER").
The problem already lies within the getOrCreateSvgId goal. When don't we know if it's a create or a get when we call it? Maybe the idProvider could be responsible for having the map allowing to get the id from any object, but isn't this information already in the metadata objects?
There was a problem hiding this comment.
Should the logic of the map be put in an AbstractIdProvider so that LegacyIdProvider and DefaultIdProvider extend from that ?
I'm not sure to understand what you mean by "When don't we know if it's a create or a get when we call it?"
How do you propose we ensure the unicity of the naming, because as it stands with the string we can't guarantee it.
There was a problem hiding this comment.
To ensure the unicity of the naming: just use a counter++ like in network-area-diagram, that's enough. And for the legacy id provider, we don't need to do anything as we cannot ensure anything without breaking it.
To me there should be no map. The IdProvider should be called only for creating id, not for getting an already created id. The nodes and edges should rather hold their id, hence no map is needed. The nodes already hold an internal id in fact, which is currently useless, so we could make good use of it. I think a good design would be to use the IdProvider at nodes and edges creation.
There was a problem hiding this comment.
(that was already discussed with @rolnico and we agreed that the underlying changes were too significant to finish this PR juste before the release)
github-project-automation
Bot
moved this from Approved
to Waiting for review
in Release 06/2026
So-Fras
changed the title
NathanDissoubray
left a comment
NathanDissoubray
left a comment
There was a problem hiding this comment.
There are some files in the SVG that look very similar and just have different svg class id (so no visual difference). I'm wondering what's the goal with those.
| /** | ||
| * @author Florian Dupuy {@literal <florian.dupuy at rte-france.com>} | ||
| */ | ||
| public class LegacyIdProvider implements IdProvider { |
There was a problem hiding this comment.
Would CountingIdProvider be better for the class name ? I think it's better to give information about the function of this class rather than the fact it's old.
There was a problem hiding this comment.
It's not based on counting.
I named it "legacy" as it gives the information that it replicates the previous behaviour and as it implies that you should rather not use it.
There was a problem hiding this comment.
Got confused with the default which has a count
| @@ -0,0 +1,534 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
There was a problem hiding this comment.
we can see that the text of the ids of B11_B12_1 and B11_B12_2 overlap with the line (or rather is hidden by it). I don't think it's an issue with this PR directly, but shouldn't the positioning of the label of the id account for the horizontal span of said ID ?
There was a problem hiding this comment.
we do not know the horizontal span of a text in a svg produced by single-line-diagram, as the users specify the font size with an external CSS
| @@ -0,0 +1,676 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
There was a problem hiding this comment.
appart from all the idLegacy becoming idTest (which is a difference in the text format of the svg), there is no visual difference between TestDrawS1PrefixLegacy.svg and TestDrawS1PrefixTest.svg. What is the goal of having those 2 files ?
There was a problem hiding this comment.
We probably need to change all the unit test references to use the default counting id provider once this PR is finished (maybe in a separate PR to have it reviewed more easily), hence the two files
5 participants
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
No
What kind of change does this PR introduce?
Feature
What is the current behavior?
In SLD, the ids used in the SVGs are created by prepending the network element ids with
idand by replacing non-alphanumeric characters with their unicode value.What is the new behavior (if this is a feature change)?
An
IdProvidercan now be given to thedrawmethods to specify how the ids should be generated.Default behavior is still the same, using
LegacyIdProvider. A new IdProvider is also available, it works as in NAD and generates the ids by counting the elements.Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information: