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

Up to N FeederInfos #341

Merged
merged 7 commits into from
Jan 27, 2022
Merged

Up to N FeederInfos #341

merged 7 commits into from
Jan 27, 2022

Conversation

tadam50
Copy link
Contributor

@tadam50 tadam50 commented Jan 25, 2022

Signed-off-by: Thomas ADAM tadam@silicom.fr

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • 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 ? If so, link to this issue using '#XXX' and skip the rest
Fixes #303

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature

What is the current behavior? (You can also link to an open issue here)
Only 2 FeederInfos expected

What is the new behavior (if this is a feature change)?
Add more that 2 FeederInfos

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:

  • 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?)

Other information:

(if any of the questions/checkboxes don't apply, please delete them entirely)

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
@tadam50 tadam50 requested a review from flo-dup January 25, 2022 09:04
@tadam50 tadam50 self-assigned this Jan 25, 2022
@flo-dup flo-dup changed the title Up to N FeederInfos (arrows) #303 Up to N FeederInfos Jan 25, 2022
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
…IntraMargin parameter

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Comment on lines 788 to 796
// If not enough space to have layoutParameters.getArrowDistance() at both sides of the 2 feeder infos,
// we compute the distance between feeder anchor and first feeder info so that the two feeder infos are centered.
double distFeederAnchorToFirstFeederInfoCenter =
distancePoints >= 2 * layoutParameters.getFeederInfosOuterMargin() + 2 * componentSize.getHeight()
? layoutParameters.getFeederInfosOuterMargin()
: (distancePoints - 2 * componentSize.getHeight()) / 2;
double x = pointA.getX() + cosAngle * (distFeederAnchorToFirstArrowCenter + shift);
double y = pointA.getY() + sinAngle * (distFeederAnchorToFirstArrowCenter + shift);
double x = pointA.getX() + cosAngle * (distFeederAnchorToFirstFeederInfoCenter + shift);
double y = pointA.getY() + sinAngle * (distFeederAnchorToFirstFeederInfoCenter + shift);

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you delete this code? I think there's no use to keep it now that the user is responsible for giving enough space for the feederInfos

Copy link
Contributor Author

@tadam50 tadam50 Jan 26, 2022

Choose a reason for hiding this comment

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

I've changed as following :
double distFeederAnchorToFirstFeederInfoCenter = layoutParameters.getFeederInfosOuterMargin()

@@ -851,15 +851,15 @@ protected void insertFeederInfos(String prefixId,
points.add(new Point(feederNode.getDiagramCoordinates()));
}

int iArrow = 0;
double shiftFeederInfo2 = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why the 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake !

@flo-dup flo-dup marked this pull request as ready for review January 25, 2022 16:46
Signed-off-by: Thomas ADAM <tadam@silicom.fr>
@sonarqubecloud
Copy link

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 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@flo-dup flo-dup merged commit 5085be0 into main Jan 27, 2022
@flo-dup flo-dup deleted the issue_303_up_to_N_feaderinfos branch January 27, 2022 10:33
MarinPinchinatLoth pushed a commit that referenced this pull request Feb 8, 2022
* Adding feederInfosIntraMargin parameter in LayoutParameters
* Rename feederArrowSymmetry to feederInfoSymmetry in LayoutParameters
* Rename arrowDistance to feederInfosOuterMargin in LayoutParameters
* Rename minSpaceForFeederArrow to spaceForFeederInfos in LayoutParameters
* Rename Arrow to FeederInfo in DefaultSVGWriter
* Rename Arrow to FeederInfo in LayoutParameters
* Add new test with 5 feeder info

Signed-off-by: Thomas ADAM <tadam@silicom.fr>
Signed-off-by: Marin Pinchinat-Loth <marin.pinchinat@rte-france.com>
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.

Up to N FeederInfos (arrows)
2 participants