KOGITO-1436: Stunner - Remove double `setContent` call. #3246

ederign · 2020-03-25T21:47:16Z

This PR changes the set content method to return a Promise instead of void.

The goal of this change is to be able to make the loading of editors predictable and remove the need for double set content call.

This PR also includes the changes by @manstis and @romartin at #3236

Merge together with:
apache/incubator-kie-tools#95
kiegroup/appformer#934

The new changes are here: 0d5c3a1

ederign · 2020-03-25T21:48:45Z

kie-wb-common-screens/kie-wb-common-library/kie-wb-common-library-spaces-screen/.gitignore

+target/
+yarn*.log


just a clean up

ederign · 2020-03-25T21:48:56Z

...widgets/src/main/java/org/kie/workbench/common/stunner/client/widgets/presenters/Viewer.java

@@ -40,8 +40,6 @@
        default void onSuccess() {
        }

-        ;
-


another clean up

ederign · 2020-03-25T21:49:13Z

...a/org/kie/workbench/common/stunner/standalone/client/editor/BPMNStandaloneDiagramEditor.java

@@ -316,6 +316,7 @@ public void onError(final ClientRuntimeError error) {
                                          BPMNStandaloneDiagramEditor.this.getEditor().onLoadError(error);
                                      }
                                  });
+        return null;


@romartin I'm not sure what should I return here.

romartin · 2020-03-25T23:27:28Z

Closing as code from this PR is already present in #3236.

romartin · 2020-03-25T23:28:32Z

@ederign oops sorry closed the wrong PR :/

romartin · 2020-03-26T03:08:24Z

Jenkins execute full downsteam build

romartin · 2020-03-26T03:36:01Z

Hey @ederign @manstis @jomarko @tiagobento

Great job guys, I've been testing this locally and it results on most of the acutal blocker tickets for editors being fixed, although I there is still one concrete case in which it doesn't work properly...

See the following video:

https://issues.redhat.com/secure/attachment/12468485/KOGITO-1436-verification.mp4

As you can see, I can verify (VERIFICATION) it works, although I can still see a non-related issue (BUG):

[VERIFICATION] Now there is no need to click "first" on the canvas, to make properties work, you can just open the editor, update the properties, save and it works (see how the it-works.bpmn process is created in the video) - It means removing the double setContent calls makes everything work as expected! Great news guys!
[BUG] On the other hand, I found a bug, that maybe it's still causing confusion to end users, but not related to this. The bug is: after some forms validatoin errors happens, whatever changes to other fields will be not saved properly. So it happens only AFTER some forms validation happens, see how the it-still-does-not-work.bpmn process is being created in the video, but not propertly updated

Sooo IMO we should take these actions:

Try to merge this PR
Try to merge also KOGITO-987 - VsCode editor - process id not saved/updated #3239
Report and fix the issue mentioned above, on top

PS: Also I've been testing this locally with #3239 merged as well, I mean both features in my local, and Ctrl +S is working fine too now in VSCode, expect in same situation that the bug mentioned above. Good job @Josephblt too!!

Again - great job everyone involed, specially @ederign @tiagobento !!!! 👍

CC @LuboTerifaj @domhanak

Thanks!!
Roger

romartin · 2020-03-26T03:39:03Z

CC @lazarotti @porcelli ^^

romartin · 2020-03-26T03:43:08Z

Jenkins execute full downstream build

romartin · 2020-03-26T04:07:42Z

CC @pefernan maybe I'll need your help Pere, after flushing, once some invalid field, it looks the forms state is not properly saved, see comment above. If you have some clue anyway please let us know!! 👍

pefernan · 2020-03-26T11:00:34Z

hey @romartin let me test it locally to understand the issue.

manstis

I tested DMN in VSCode and -webapp-kogito-testing. Both were fine. Thank-you.

domhanak · 2020-03-26T12:40:16Z

Jenkins execute full downstream build

jomarko · 2020-03-26T14:05:18Z

...rg/kie/workbench/common/dmn/webapp/kogito/common/client/editor/AbstractDMNDiagramEditor.java

-    public void setContent(final String path,
+    public Promise setContent(final String path,
                           final String value) {


please update formatting

jomarko · 2020-03-26T14:08:52Z

...ito-client/src/main/java/org/kie/workbench/common/kogito/client/editor/BaseKogitoEditor.java

@@ -145,7 +145,7 @@ public boolean isDirty() {
     * @param value
     * Editor's content
     */
-    public abstract void setContent(final String path, final String value);
+    public abstract Promise setContent(final String path, final String value);


In appformer we changed to Promise<Void> shouldn't we follow also here?

That's a current limitation of our annotation processors. Other methods also return untyped Promises for now.

jomarko · 2020-03-26T14:57:55Z

Manual check in business central was same as on vs code apache/incubator-kie-tools#95 (comment), but passed all scenarios.

romartin · 2020-03-26T17:06:40Z

Hey @ederign @paulovmr @tiagobento

Please this PR is blocking this other one for being properly reviewed: #3239

Could you please prioritize this review and try to merge this ASAP?

Thx!

romartin · 2020-03-26T17:07:48Z

CC @LuboTerifaj @Josephblt ^^

sonarcloud · 2020-03-26T21:13:34Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

8.5% Coverage
0.0% Duplication

romartin and others added 2 commits March 24, 2020 19:56

KOGITO-1436: Stunner - Remove double setContent call.

405c81a

Remove workaround for double-setContent(..)

94a3ac6

ederign requested review from manstis, paulovmr and tiagobento March 25, 2020 21:47

This was referenced Mar 25, 2020

[KOGITO-1436]Remove double setContent call kiegroup/appformer#934

Merged

KOGITO-1436: Remove double setContent call apache/incubator-kie-tools#95

Merged

ederign requested a review from jomarko March 25, 2020 21:48

ederign commented Mar 25, 2020

View reviewed changes

kie-wb-common-screens/kie-wb-common-library/kie-wb-common-library-spaces-screen/.gitignore

target/

yarn*.log

Copy link

Member Author

ederign Mar 25, 2020

Choose a reason for hiding this comment

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

just a clean up

ederign commented Mar 25, 2020

View reviewed changes

ederign mentioned this pull request Mar 25, 2020

KOGITO-1436: Stunner - Remove double setContent call. #3236

Closed

romartin closed this Mar 25, 2020

romartin reopened this Mar 25, 2020

romartin mentioned this pull request Mar 26, 2020

KOGITO-987 - VsCode editor - process id not saved/updated #3239

Merged

romartin approved these changes Mar 26, 2020

View reviewed changes

manstis approved these changes Mar 26, 2020

View reviewed changes

jomarko reviewed Mar 26, 2020

View reviewed changes

tiagobento approved these changes Mar 26, 2020

View reviewed changes

[KOGITO-1436]Remove double setContent call

42ba81f

ederign force-pushed the KOGITO-1436 branch from 0d5c3a1 to 42ba81f Compare March 26, 2020 17:28

ederign merged commit 1fa0ec9 into kiegroup:master Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KOGITO-1436: Stunner - Remove double `setContent` call. #3246

KOGITO-1436: Stunner - Remove double `setContent` call. #3246

ederign commented Mar 25, 2020 •

edited

ederign Mar 25, 2020

ederign Mar 25, 2020

ederign Mar 25, 2020

romartin commented Mar 25, 2020

romartin commented Mar 25, 2020

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020 •

edited

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020

pefernan commented Mar 26, 2020

manstis left a comment

domhanak commented Mar 26, 2020

jomarko Mar 26, 2020

ederign Mar 26, 2020

jomarko Mar 26, 2020

tiagobento Mar 26, 2020

jomarko commented Mar 26, 2020

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020

sonarcloud bot commented Mar 26, 2020

KOGITO-1436: Stunner - Remove double setContent call. #3246

KOGITO-1436: Stunner - Remove double setContent call. #3246

Conversation

ederign commented Mar 25, 2020 • edited

ederign Mar 25, 2020

Choose a reason for hiding this comment

ederign Mar 25, 2020

Choose a reason for hiding this comment

ederign Mar 25, 2020

Choose a reason for hiding this comment

romartin commented Mar 25, 2020

romartin commented Mar 25, 2020

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020 • edited

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020

pefernan commented Mar 26, 2020

manstis left a comment

Choose a reason for hiding this comment

domhanak commented Mar 26, 2020

jomarko Mar 26, 2020

Choose a reason for hiding this comment

ederign Mar 26, 2020

Choose a reason for hiding this comment

jomarko Mar 26, 2020

Choose a reason for hiding this comment

tiagobento Mar 26, 2020

Choose a reason for hiding this comment

jomarko commented Mar 26, 2020

romartin commented Mar 26, 2020

romartin commented Mar 26, 2020

sonarcloud bot commented Mar 26, 2020

KOGITO-1436: Stunner - Remove double `setContent` call. #3246

KOGITO-1436: Stunner - Remove double `setContent` call. #3246

ederign commented Mar 25, 2020 •

edited

romartin commented Mar 26, 2020 •

edited