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

JBPM-7162: Stunner - Make canvas size configurable (via workbench preferences) #1714

Merged
merged 1 commit into from May 4, 2018

Conversation

wmedvede
Copy link
Member

No description provided.

@wmedvede
Copy link
Member Author

@romartin @hasys Hi, would you mind review this PR?
Thanks.

@romartin I've set the maximum accepted values for the canvas size two times the one we already had. This was more or less what we talked about. But since you have more background/experience around the performance issues please take a look, and let me know if we need an adjustment, or if this max values can even be increased.

@wmedvede
Copy link
Member Author

Jenkins execute full downstream build

@romartin romartin requested review from hasys and romartin April 30, 2018 17:27
@wmedvede
Copy link
Member Author

wmedvede commented May 2, 2018

Jenkins execute full downstream build

2 similar comments
@wmedvede
Copy link
Member Author

wmedvede commented May 3, 2018

Jenkins execute full downstream build

@wmedvede
Copy link
Member Author

wmedvede commented May 3, 2018

Jenkins execute full downstream build

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

Some comments for discussion from my site, otherwise looks very well.

StunnerDiagramEditorPreferences.AutoHidePalettePanel.Help=Enables the automatically hiding of the toolbar panel associated with a Category. E.g. when a category item is selected, or when the mouse leaves the panel.
StunnerDiagramEditorPreferences.CanvasWidth.Label=Drawing area width
StunnerDiagramEditorPreferences.CanvasWidth.Help=Sets the width of the editor drawing area. It must be an integer number between 2800 and 5600.
Copy link

Choose a reason for hiding this comment

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

Would be possoble to replace constants 2800 ... with something like {0} ... and then during construction of the real string use the constants from CanvasWidthValidator and CanvasHeightValidator ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jomarko The preferences API doesn't have the ability of accepting/processing {xxx} parameters in the validators, so it's not possible at this point.

intValue = Integer.parseInt(value);
} catch (NumberFormatException e) {
return new ValidationResult(false,
messagesBundleKeys());
Copy link

Choose a reason for hiding this comment

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

Just minor thing, but I think NumberFormatException doesn't mean out of the range.

Copy link
Member Author

@wmedvede wmedvede May 4, 2018

Choose a reason for hiding this comment

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

@jomarko I see the point, but see that validations are chained and the preferences api first checks if the value is a valid number, and then "transfers the execution" to the next validator. So if a wrong number is typed, this validator is not executed, and a proper message is automatically set. Since I still need to parse the value, I need to do something in case the exception is throw, but this case will probably never happen. I believe we can live with that. We'll also move to infinite canvases as asap, so this canvas size preferences will be removed asap too.

@@ -31,6 +31,8 @@
@Override
public StunnerPreferences defaultValue(final StunnerPreferences defaultValue) {
defaultValue.diagramEditorPreferences.setAutoHidePalettePanel(false);
defaultValue.diagramEditorPreferences.setCanvasWidth(2800);
Copy link

Choose a reason for hiding this comment

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

I think we could use CanvasWidthValidator.MIN_CANVAS_WIDTH

Copy link
Member Author

Choose a reason for hiding this comment

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

@jomarko I don't think so. The MIN_VALUE is the lower bound. i.e., we don't want users to try to set the value lower than this minimum. But instead the by default value is what we "believe" is "the best value" to start working with. Then the user has the freedom of changing.

Copy link

Choose a reason for hiding this comment

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

Thanks for explanation.

@@ -31,6 +31,8 @@
@Override
public StunnerPreferences defaultValue(final StunnerPreferences defaultValue) {
defaultValue.diagramEditorPreferences.setAutoHidePalettePanel(false);
defaultValue.diagramEditorPreferences.setCanvasWidth(2800);
defaultValue.diagramEditorPreferences.setCanvasHeight(1400);
Copy link

Choose a reason for hiding this comment

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

I think we could use CanvasHeightValidator.MIN_CANVAS_HEIGHT

Copy link
Member Author

Choose a reason for hiding this comment

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

@jomarko same as above.

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

Approving in advance, I didn't check manualy, leaving final check for @hasys , thanks for response @wmedvede .

@LuboTerifaj
Copy link
Contributor

LuboTerifaj commented May 4, 2018

Hi @wmedvede @jomarko @hasys
I have checked it also manually and it seems that it works properly.
Thanks!

@romartin romartin merged commit ccb3da7 into kiegroup:master May 4, 2018
wmedvede added a commit to wmedvede/kie-wb-common that referenced this pull request May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants