-
Notifications
You must be signed in to change notification settings - Fork 219
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
DROOLS-3243: [DMN Designer] Move the Data Type dialog to a tab in the DMN editor #2314
Conversation
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.
Some comments to code
...mn-client/src/main/java/org/kie/workbench/common/dmn/client/editors/types/DataTypesPage.java
Outdated
Show resolved
Hide resolved
...lient/src/test/java/org/kie/workbench/common/dmn/client/editors/types/DataTypesPageTest.java
Show resolved
Hide resolved
Save of whole diagram doesn't shown FlashMessages. Please follow my steps.
|
Global save doesn't cancel edit operation of particular item definitions, please follow my steps:
I think we should cancel edit mode of item definitions if global save was clicked |
@karreiro thanks for PR, really nice coverage and performance seems to be really better! However I would like to ask you to check comments above before approval. |
@jomarko Thank you for the review. I'm already working in your comments. :-) |
...nt/src/main/java/org/kie/workbench/common/dmn/client/editors/types/DataTypePickerWidget.java
Outdated
Show resolved
Hide resolved
...mn-client/src/main/java/org/kie/workbench/common/dmn/client/editors/types/DataTypesPage.java
Outdated
Show resolved
Hide resolved
...mn-client/src/main/java/org/kie/workbench/common/dmn/client/editors/types/DataTypesPage.java
Outdated
Show resolved
Hide resolved
.../org/kie/workbench/common/dmn/showcase/client/screens/editor/SessionDiagramEditorScreen.java
Show resolved
Hide resolved
960e744
to
77b47e7
Compare
Thank you for the review @manstis @jomarko. Changes applied. Regarding the issues, you mentioned above. 1) Scroll bars issue 2) Some data type definitions are not visible 3) The 4) About these two topics: I don't see both as issues. They look like the expected behavior. I think we could raise a JIRA (involving Liz) to discuss more them, and eventually, implement the discussed approach in a future PR. Wdyt? |
@karreiro thank you, I am going to re-review quickly today, could you please add point 4) to our weekly meeting agenda? |
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.
Manual check pending
.../org/kie/workbench/common/dmn/showcase/client/screens/editor/SessionDiagramEditorScreen.java
Outdated
Show resolved
Hide resolved
...lient/src/main/java/org/kie/workbench/common/dmn/project/client/editor/DMNDiagramEditor.java
Outdated
Show resolved
Hide resolved
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.
@karreiro I can confirm 1) 2) and 3) works great now. Approving regardless of answers on my questions.
…e view items take ~100ms to be instantiated * DROOLS-3243: [DMN Designer] Move the Data Type dialog to a tab in the DMN editor
f1484ab
to
1be980a
Compare
@jomarko Thank you. I've extracted the constants for the page indexes as suggested. |
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.
Just a couple of follow up questions.
Liking what you did to DataTypeListItemView
-- I guess the Errai binding takes a ton of time!
...common-dmn-backend/src/test/java/org/kie/workbench/common/dmn/backend/DMNMarshallerTest.java
Outdated
Show resolved
Hide resolved
...common-dmn-backend/src/test/java/org/kie/workbench/common/dmn/backend/DMNMarshallerTest.java
Outdated
Show resolved
Hide resolved
...common-dmn-backend/src/test/java/org/kie/workbench/common/dmn/backend/DMNMarshallerTest.java
Outdated
Show resolved
Hide resolved
...ent/src/main/java/org/kie/workbench/common/dmn/client/editors/types/common/JQueryNavTab.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/org/kie/workbench/common/dmn/client/editors/types/DataTypePickerWidget.java
Outdated
Show resolved
Hide resolved
...ient/src/main/java/org/kie/workbench/common/dmn/client/editors/types/DataTypePageNavTab.java
Outdated
Show resolved
Hide resolved
...mmon-dmn-webapp/src/main/resources/org/kie/workbench/common/dmn/showcase/DMNShowcase.gwt.xml
Show resolved
Hide resolved
@manstis Changes applied. |
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.
Thanks @karreiro :-)
See:
DROOLS-3243:
DROOLS-3056:
The core of the performance enhancement in this PR lives in the
DataTypeListItemView
. All@DataField
s are lazy now, this decision makes the code more prolix but considerably increases the performance.Before this improvement, the instantiation of a
DataTypeListItem
in theDataTypeList
was taking ~29ms*, and now it takes less than ~9ms, it has a significant impact when the DMN file has a lot a Data Types.For the scenario that Jozef raised here, the remove operation still holds a small delay but a considerable improvement can be noticed.
The next (and final improvement) will be an enhancement in the algorithm that elects the dependent Data Types in the ItemDefinitionRecordEngine. However, for now, I do think that the current optimisations are good enough.
* The title of the JIRA says 100ms, but I could make some optimisations before starting the real work in the JIRA.