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

Modify diagram apis #522

Merged
merged 47 commits into from
Sep 28, 2023
Merged

Modify diagram apis #522

merged 47 commits into from
Sep 28, 2023

Conversation

So-Fras
Copy link
Member

@So-Fras So-Fras commented Jun 5, 2023

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)

Does this PR already have an issue describing the problem?
#469
There is another PR on the same topic (#475). This old PR has diverged too much from main and it was easier to start afresh than to rebase or merge.
Developments on the old PR were added to this new PR.
New developments have been added afterwards on the new branch:

  • Config and ConfigBuilder have been renamed in Param and ParamBuilder
  • Param and ParamBuilder have become network agnostic, both for SLD and NAD (whereas it was only the case for NAD on the old PR)

What kind of change does this PR introduce?
Modification and standardization of SLD and NAD APIs

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

  • The Breaking Change or Deprecated label has been added
  • The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)

So-Fras and others added 7 commits April 6, 2023 16:20
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
* Remove executions goals from sld-cgmes-dl-iidm-extensions

* Add new module powsybl-single-line-diagram-cgmes-iidm-extensions-test

* Import Networks class from powsybl-single-line-diagram-cgmes-iidm-extensions-test

* Use new module powsybl-single-line-diagram-cgmes-iidm-extensions-test instead of using test-jars

* Rename & move module powsybl-single-line-diagram-cgmes-iidm-extensions-test to diagram-test

* Fix sonar bug

* Use Networks class instead of duplication code

* Merge NetworkTestFactory into Networks class

* Fix code smells on Networks class

* Replace CreateNetworksUtil & NetworkFactory by Networks class

* Fix code smells on Netorks

* Taking into account PR comments

* Fix code smells on Networks class

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
@@ -36,11 +39,12 @@ public abstract class AbstractTestCaseIidm extends AbstractTestCase {

@Override
public String toSVG(Graph g, String filename) {
return toSVG(g, filename, getDefaultDiagramLabelProvider(), getDefaultDiagramStyleProvider());
DefaultSVGWriter defaultSVGWriter = new DefaultSVGWriter(componentLibrary, layoutParameters, svgParameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't the SingleLineDiagram class create the SvgWriter like it used to be?

Comment on lines -283 to -284
public static void draw(Graph graph, Writer writerForSvg, Writer metadataWriter, LayoutParameters layoutParameters, ComponentLibrary componentLibrary,
DiagramLabelProvider initProvider, StyleProvider styleProvider, String prefixId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed this method, but it was used in unit tests. Removing it forced you to instantiate a DefaultSvgWriter in all the unit tests. We don't want this, as this leads to always the same code in all the tests.

With the new design the equivalent would be a draw(graph, writerForSvg, metadataWriter, param) method.

}

public static void draw(Graph graph, Writer writerForSvg, Writer metadataWriter, DefaultSVGWriter svgWriter,
DiagramLabelProvider initProvider, StyleProvider styleProvider, String prefixId) {
public static void draw(Graph graph, Writer writerForSvg, Writer metadataWriter, SVGWriter svgWriter, LabelProvider labelProvider, StyleProvider styleProvider, String prefixId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should hide the svgWriter and therefore remove this method. Besides, exposing the providers is against the new design.

This a utility class, hence we want to keep methods simple (simpler than they were!), an advanced user who wants to use a custom SvgWriter can still do it, without using this class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not wish to expose the providers but the LabelProvider and the StyleProvider cannot be retrieved using an SldParameters object only. I need a Network object too. That is why I had to "build" those providers earlier, when I could still have access to the Network object.

Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
…orship on recently created factory interfaces

Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
So-Fras and others added 21 commits July 21, 2023 13:48
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <93923177+So-Fras@users.noreply.github.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
…s in a different way

Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <93923177+So-Fras@users.noreply.github.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <93923177+So-Fras@users.noreply.github.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Sophie Frasnedo <sophie.frasnedo@rte-france.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@gmail.com>
Signed-off-by: Geoffroy Jamgotchian <geoffroy.jamgotchian@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 13 Code Smells

92.6% 92.6% Coverage
0.9% 0.9% Duplication

@So-Fras So-Fras merged commit 07edea0 into main Sep 28, 2023
6 checks passed
@So-Fras So-Fras deleted the modify_diagram_apis branch September 28, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants