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

Issue 86 VoltageLevelLayoutFactory customization #90

Merged
merged 8 commits into from
Feb 13, 2024

Conversation

tadam50
Copy link
Contributor

@tadam50 tadam50 commented Jan 22, 2024

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?
'Fixes #86'

Other information:
Need issue #85 to be merged before this PR merge

@tadam50 tadam50 requested a review from So-Fras January 22, 2024 15:39
@tadam50 tadam50 self-assigned this Jan 22, 2024
@tadam50 tadam50 changed the base branch from main to issue_85_update_powsybl-dependencies January 22, 2024 15:40
Base automatically changed from issue_85_update_powsybl-dependencies to main February 5, 2024 09:54
@flo-dup flo-dup force-pushed the 86-voltage-level-layout-factory-customization branch from e6a53a1 to 9c8273e Compare February 5, 2024 11:53
@tadam50 tadam50 force-pushed the 86-voltage-level-layout-factory-customization branch from 9c8273e to 666db86 Compare February 6, 2024 13:37
flo-dup
flo-dup previously requested changes Feb 9, 2024
Copy link
Contributor

@flo-dup flo-dup left a comment

Choose a reason for hiding this comment

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

Nice, even simpler! 👍 Just a couple of minor comments

<ComboBox fx:id="voltageLevelLayoutComboBox"/>
<CheckBox fx:id="stackFeedersCheckBox" text="Stack feeders"/>
<EnumChoiceBox enumType="com.powsybl.diagram.viewer.sld.SingleLineDiagramModel$VoltageLevelLayoutFactoryType" fx:id="voltageLevelLayoutComboBox" initialValue="SMART"/>
<CheckBox fx:id="stackFeedersCheckBox" text="Stack feeders" selected="true"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

there's also removeUnnecessaryFictitiousNodes and substituteSingularFictitiousByFeederNode which are supposed to be true by default

@Override
public String toString() {
return switch (this) {
case SMART -> "Smart";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "'Smart' choice", to highlight the choice behind? because the SmartSelector chooses position with extensions if the vl contains at least one position extension, if not it chooses cgmes diagram layout if there's at least one cgmes extension, if not it chooses clustering.

@So-Fras So-Fras force-pushed the 86-voltage-level-layout-factory-customization branch from 7c3e873 to efaae20 Compare February 13, 2024 15:25
Copy link

@So-Fras So-Fras dismissed flo-dup’s stale review February 13, 2024 15:28

Comments taken into account by @tadam50 + discussed with @flo-dup before he left on holiday

@So-Fras So-Fras merged commit 2356338 into main Feb 13, 2024
4 checks passed
@So-Fras So-Fras deleted the 86-voltage-level-layout-factory-customization branch February 13, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VoltageLevelLayoutFactory customization
3 participants