-
Notifications
You must be signed in to change notification settings - Fork 31
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
[REFACTOR] use BpmnCanvas
in IconPainter
#404
Conversation
Move all methods that set icon origin to `BpmnCanvas` Remove `MxCanvasUtil` static utility `IconPainter` additional refactoring: method signatures and rename
Note: icons touched by this refactoring
|
this.scaleX = (parentSize / this.iconOriginalSize.width) * ratioFromShape; | ||
this.scaleY = (parentSize / this.iconOriginalSize.height) * ratioFromShape; | ||
} else { | ||
this.scaleX = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if the ratio from parent is calculated like I do in IconPainter.calculateIconSize(...).
But when the ratioFromParent parameter is equals to 1, I consider that the icon must have the same size as the shape (if height < width of shape, icon height = shape height and icon width is calculate proportionally with original icon height / shape height).
I don't know if it's really clear. We can refactor the scale calculation in another PR 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csouchet I plan to do "no more actual icon size computation in IconPainter" in the next refactoring as described in #247
The computation is currently not done in the same way in IconPainter and BpmnCanvas but it should be. This will be in a next refactoring to unify this.
The idea in this PR is to do the refactoring without behaviour changes and with small/baby steps. So for now, there are potentially duplications, several ways of doing things (like before). This will be managed progressively.
About your remark about the ratioFromParent = 1 (remember this is only temporary to make the icon painter uses the BPMN Canvas but the integration is not finished)
- we don't use
ratioFromParent = 1
here butratioFromParent = undefined
- the actual icon size is computed outside of the canvas, using the ratioFromParent
- the icon size passed to the canvas is the actual size so the canvas must not scale it (i.e. scaling = 1). For now, I use this trick (undefined ratio from parent/shape) to decide to not scale the icon dimensions. I add TSDoc in
IconConfiguration
to explain it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Move all methods that set icon origin to
BpmnCanvas
Remove
MxCanvasUtil
static utilityIconPainter
additional refactoring: method signatures and renameCovers #247