JBPM-5538 & JBPM-5449: Popups for unsaved data and no changes since last save #920

Christopher-Chianelli · 2017-06-05T19:51:39Z

Depends on #910, which depends on kiegroup/droolsjbpm-build-bootstrap#449. The unsaved changes message will not appear if you make any changes now. Additionally, if you try saving without making any changes, a popup will appear informing you that there are no changes since you last saved. Some components hashCode functions need to be updated so that it changes when the component changes.

kie-ci · 2017-06-05T19:52:10Z

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci · 2017-06-05T19:52:13Z

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci · 2017-06-05T19:53:09Z

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci · 2017-06-05T19:55:08Z

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

romartin · 2017-06-09T21:14:13Z

ok to test

romartin · 2017-06-09T21:16:48Z

Hey @Christopher-Chianelli , first of all thanks for this.
I just realized now that this PR exists hehe, feel free for next times, when creating PRs for Stunner, to add a reference to my github user in the comment, this way I will receive the notification and will start as soon as possible the review :) Up to you!
Thanks!

romartin

Hey @Christopher-Chianelli , tested and it works good yeah, anyway there are still missing a couple of things:

It's missing to add the equals/hashCode implementations tfor the beans in the CaseManagement API modules, as it's being considered a KIE editor too.
Unit tests in general - so for HashUtils methods, unit tests for the updates in AbstractProjectDiagramEditor, and probably makes sense too creating a single test class for testing the equality for all existing BPMN and another one for testing CM nodes..
Thanks!!

Christopher-Chianelli · 2017-06-13T15:33:52Z

@romartin I added equals and hashCode functions for the remaining beans, and wrote tests for saving, HashUtils, and equals/hashCode.

romartin

Hey @Christopher-Chianelli , thanks it's almost completed hehe! Just added a few comments, hope easy to fix in a bit. On the other hand good job, tested and it works good.
BTW please remember to squash into a single commit!
Thanks!

romartin · 2017-06-13T18:39:47Z

...er-core-common/src/main/java/org/kie/workbench/common/stunner/core/graph/impl/GraphImpl.java

+                            }
+
+                            @Override
+                            public void endEdgeTraversal(final Edge edge) {


the method endEdgeTraversal can be removed - no need to override this method, as already exist a default method on the interface

@romartin done! The two methods that just called the default were there just in case I need to modify them; since I don't they were removed. The multiple if statements were there since before for some reason my hashCode implementation for the nodes weren't being called. I have squashed all the commits. Note that this branch is now based at HEAD~1, since the current master branch is not compiling due to missing methods.

romartin · 2017-06-13T18:39:55Z

...er-core-common/src/main/java/org/kie/workbench/common/stunner/core/graph/impl/GraphImpl.java

+                            int[] myHashArr = hashArr;
+                            @Override
+                            public void
+                            startGraphTraversal(final org.kie.workbench.common.stunner.core.graph.Graph graph) {


the method startGraphTraversal can be removed - no need to override this method, as already exist a default method on the interface

romartin · 2017-06-13T18:42:30Z

...er-core-common/src/main/java/org/kie/workbench/common/stunner/core/graph/impl/GraphImpl.java

+                                super.startEdgeTraversal(edge);
+                                final Object content = edge.getContent();
+                                myHashArr[0] = HashUtil.combineHashCodes(myHashArr[0], content.hashCode());
+                                if (content instanceof Child) {


I think that the different hash values combined inside these if sentences (not only this one, I mean all in this method) are not necessary, so in this method only combining the hash for the edge.getContent() should be enough. I think it's true because both sources and target nodes for each edge instance, are already being processed in the other callback method startNodeTraversal, so probably you're duplicating the hashes twice this way.
Does it makes sense? Can you try it please?

romartin

Yeah sounds good now @Christopher-Chianelli , thanks! :)

romartin · 2017-06-13T20:43:43Z

CC @manstis @hasys in case you want to review too :)

manstis

Added @hasys for review too.

Could you please also run code formatter over the changes as I see a number of inconsistencies. It might also be worth adding similar changes to the Case Management definitions.

manstis · 2017-06-14T07:39:01Z

...n-stunner-core-common/src/main/java/org/kie/workbench/common/stunner/core/util/HashUtil.java

@@ -0,0 +1,19 @@
+package org.kie.workbench.common.stunner.core.util;


manstis · 2017-06-14T07:41:09Z

.../src/test/java/org/kie/workbench/common/stunner/bpmn/definition/HashCodeAndEqualityTest.java

@@ -0,0 +1,334 @@
+/*
+ * Copyright 2016 Red Hat, Inc. and/or its affiliates.


jomarko · 2017-06-14T10:30:27Z

...ner-core-common/src/main/java/org/kie/workbench/common/stunner/core/diagram/DiagramImpl.java

@@ -28,4 +29,24 @@ public DiagramImpl(final @MapsTo("name") String name,
        super(name,
              metadata);
    }
+
+    @Override
+    public int hashCode()


This hashCode() and equals() below are kind of suspicious for me. Are we sure that getGraph() returns always non null value? Same question about getMetadata() and getName().

Thanks for finding that! Before, HashUtil.combineHashCodes took an array of Objects and called hashCode on each of them (after doing a null check). However, it always called Object's hashCode implementation, so I changed it to take an array of ints.

Christopher-Chianelli · 2017-06-14T14:34:37Z

Reformatted code, added/updated copyright and added more null checks.

manstis

@Christopher-Chianelli Please resolve the merge conflict.

Christopher-Chianelli · 2017-06-15T13:46:49Z

Resolved; we inserted packages at the same line. (I might have forgotten to alphabetize the order though...)

Christopher-Chianelli · 2017-06-15T13:50:42Z

(now alphabetized)

hasys · 2017-06-15T13:59:21Z

Jenkins execute full downstream build

kie-ci · 2017-06-19T13:18:14Z

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

romartin · 2017-06-19T17:25:44Z

ok to test

hasys · 2017-06-20T08:33:49Z

Jenkins execute full downstream build

hasys

Hi @Christopher-Chianelli,
thank you, looks good! Just some thoughts from me:

BUG: Open process, move any node, try to save => you will see "You have no unsaved changes."
Improvements: Show notification "You have no unsaved changes.", not Error.
Also tasks and unrelated issues were reported: JBPM-6126, JBPM-6127, JBPM-6128.
about missing tests see inline comments and JBPM-6126

hasys · 2017-06-20T11:58:08Z

...-api/src/main/java/org/kie/workbench/common/stunner/cm/definition/CaseManagementDiagram.java

+                    dimensionsSet.equals(other.dimensionsSet);
+        }
+        return false;
+    }


Not tested methods.

hasys · 2017-06-20T11:58:25Z

...gmt-api/src/main/java/org/kie/workbench/common/stunner/cm/definition/ReusableSubprocess.java

+                    dataIOSet.equals(other.dataIOSet);
+        }
+        return super.equals(o);
+    }


Not tested methods.

hasys · 2017-06-20T11:58:42Z

...ner-core-common/src/main/java/org/kie/workbench/common/stunner/core/diagram/DiagramImpl.java

+        } else {
+            return false;
+        }
+    }


Not tested methods.

hasys · 2017-06-20T11:59:16Z

...er-core-common/src/main/java/org/kie/workbench/common/stunner/core/graph/impl/GraphImpl.java

+        } else {
+            return false;
+        }
+    }


Not tested additions.

hasys · 2017-06-20T12:00:43Z

.../src/main/java/org/kie/workbench/common/stunner/project/diagram/impl/ProjectDiagramImpl.java

+            return false;
+        }
+    }
+


Not tested methods.

hasys · 2017-06-20T12:01:45Z

...ava/org/kie/workbench/common/stunner/project/client/editor/AbstractProjectDiagramEditor.java

+
+    private boolean hasUnsavedChanges() {
+        return getCurrentDiagramHash() != originalHash;
+    }


No tests for changes in the file.

hasys · 2017-06-20T12:03:59Z

...ava/org/kie/workbench/common/stunner/project/client/editor/AbstractProjectDiagramEditor.java

+                                continueSaveOnceValid.execute();
+                            } else {
+                                //TODO: Localize message
+                                errorPopupPresenter.showMessage("You have no unsaved changes.");


I see duplicated not localized literal. @romartin how do we handle it for now?

yes sure, please inject the TranslationService so you can use errai i18n here. Thanks!! :)

romartin · 2017-06-20T17:23:36Z

Hey @Christopher-Chianelli , please can you verify that latest included BPMN bean, the EmbeddedSubprocess, does also contains the right equals/hasCode methods? This new bean has been added a few days ago, before this commit, so probably it needs to override that methods too.
Thanks!

Christopher-Chianelli · 2017-06-20T17:55:30Z

I cannot really check anything that been added in the last commit, @romartin; the last commit fails to build on master due to missing classes (BlobImpl, I think, from uberfire). I can add the hashCode/equals, but I won't be able to test or run it on the webclient (compilation fails during gwt compile, not mvn build).

romartin · 2017-06-20T18:25:19Z

Hey @Christopher-Chianelli , mmm the build works for me, but yeahh not sure why Errai snapshots seem not to be building on nexus for last few days... anyway building it locally makes it work, so please just build latest Errai and Uberfire locally and you will be able to build & run the app. Lemme know if any issues! Thanks!

Christopher-Chianelli · 2017-06-20T19:15:49Z

That fixed it! Thanks @romartin!

Christopher-Chianelli · 2017-06-22T21:03:20Z

Added bunch of tests, made moving nodes count as changes and did localization (I think, notify me if I made a mistake). Also added hashCode and equals for EmbeddedSubprocess.

romartin · 2017-06-22T22:08:08Z

Jenkins execute full downstream build

romartin

Hey @Christopher-Chianelli
Rather than just a couple of minor things, all good for me, tested and it works fine.
Requesting only these minor changes and then all good for me, can be merged.
Thanks!

romartin · 2017-06-23T00:03:05Z

...ava/org/kie/workbench/common/stunner/project/client/editor/AbstractProjectDiagramEditor.java

+            super.onSave();
+        }
+        else {
+            //TODO: Localize message


AFAIK this TODO can be removed

I only wrote the message in the English Common Constants properties file, so I don't know how it'll behave in other languages.

romartin · 2017-06-23T00:05:27Z

...ava/org/kie/workbench/common/stunner/project/client/editor/AbstractProjectDiagramEditor.java

+        }
+        else {
+            //TODO: Localize message
+            Window.alert(CommonConstants.INSTANCE.NoChangesSinceLastSave());


Please better using the method showError (in this class), it uses an uberfire generic popup widget - we should avoid the use of Window.alert

Do you want it to be an error or notification? @hasys requested for it to be a notification (since I couldn't find a notification class that is applicable to what data I have at the time of the popup, so I used WIndow.alert for a generic notification).

Yes, sorry, I mean something like this:

Just notification, no Message Dialog with buttons.

hasys · 2017-06-23T13:46:58Z

Jenkins execute full downstream build

save

Christopher-Chianelli · 2017-06-23T15:24:47Z

Changed Window.alert to an info notification and removed the TODO @romartin @hasys.

romartin

Great, thanks @Christopher-Chianelli !!

hasys · 2017-06-26T12:13:41Z

Jenkins execute full downstream build

hasys

Hi @Christopher-Chianelli,

thank you, looks really cool! Sorry, but I found one issue there JBPM-6157, but guess it's better to merge this PR and fix bug later (in current Sprint or next one).

Christopher-Chianelli · 2017-06-26T16:03:49Z

Thanks for informing me @hasys! I'll start investigating the issue immediately.

Christopher-Chianelli · 2017-06-26T18:20:40Z

Fixed issue @hasys, should I just push the fix to this branch?

hasys · 2017-06-27T08:43:55Z

Hi @Christopher-Chianelli, for me two small PR are better than one huge. If it's possible I prefer merge this one (it is already big one) and send new PR. Thank you!

jomarko

My inputs incorporated. Thanks.

Christopher-Chianelli force-pushed the save-popup-always-show branch 4 times, most recently from 734bfb3 to 0d6e08e Compare June 9, 2017 12:39

romartin requested changes Jun 9, 2017

View reviewed changes

Christopher-Chianelli force-pushed the save-popup-always-show branch 3 times, most recently from bde023b to aa4559e Compare June 13, 2017 15:27

romartin requested changes Jun 13, 2017

View reviewed changes

Christopher-Chianelli force-pushed the save-popup-always-show branch from aa4559e to d9278bc Compare June 13, 2017 19:07

romartin approved these changes Jun 13, 2017

View reviewed changes

manstis requested a review from hasys June 14, 2017 07:38

manstis reviewed Jun 14, 2017

View reviewed changes

jomarko reviewed Jun 14, 2017

View reviewed changes

Christopher-Chianelli force-pushed the save-popup-always-show branch 2 times, most recently from 74bcf50 to e400932 Compare June 14, 2017 14:30

manstis reviewed Jun 15, 2017

View reviewed changes

Christopher-Chianelli force-pushed the save-popup-always-show branch from 9d7ae06 to aac4587 Compare June 15, 2017 13:50

Christopher-Chianelli force-pushed the save-popup-always-show branch 2 times, most recently from 6d43b27 to e0522bf Compare June 16, 2017 19:05

hasys requested changes Jun 20, 2017

View reviewed changes

Christopher-Chianelli force-pushed the save-popup-always-show branch 3 times, most recently from 5d0e608 to a54f2f2 Compare June 22, 2017 20:58

romartin requested changes Jun 23, 2017

View reviewed changes

JBPM-5538 & JBPM-5449: Popups for unsaved data and no changes since last

386ec28

save

Christopher-Chianelli force-pushed the save-popup-always-show branch from a54f2f2 to 386ec28 Compare June 23, 2017 15:23

romartin approved these changes Jun 23, 2017

View reviewed changes

hasys approved these changes Jun 26, 2017

View reviewed changes

jomarko approved these changes Jun 27, 2017

View reviewed changes

manstis approved these changes Jun 27, 2017

View reviewed changes

manstis merged commit ef84b62 into kiegroup:master Jun 27, 2017

		@@ -0,0 +1,19 @@
		package org.kie.workbench.common.stunner.core.util;

		@@ -0,0 +1,334 @@
		/*
		* Copyright 2016 Red Hat, Inc. and/or its affiliates.

JBPM-5538 & JBPM-5449: Popups for unsaved data and no changes since last save #920

JBPM-5538 & JBPM-5449: Popups for unsaved data and no changes since last save #920

Conversation

Christopher-Chianelli commented Jun 5, 2017

kie-ci commented Jun 5, 2017

kie-ci commented Jun 5, 2017

kie-ci commented Jun 5, 2017

kie-ci commented Jun 5, 2017

romartin commented Jun 9, 2017

romartin commented Jun 9, 2017

romartin left a comment

Choose a reason for hiding this comment

Christopher-Chianelli commented Jun 13, 2017

romartin left a comment

Choose a reason for hiding this comment

romartin Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Christopher-Chianelli Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

romartin Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

romartin Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

romartin left a comment

Choose a reason for hiding this comment

romartin commented Jun 13, 2017

manstis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Christopher-Chianelli commented Jun 14, 2017

manstis left a comment

Choose a reason for hiding this comment

Christopher-Chianelli commented Jun 15, 2017

Christopher-Chianelli commented Jun 15, 2017

hasys commented Jun 15, 2017

kie-ci commented Jun 19, 2017

romartin commented Jun 19, 2017

hasys commented Jun 20, 2017

hasys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romartin commented Jun 20, 2017

Christopher-Chianelli commented Jun 20, 2017

romartin commented Jun 20, 2017

Christopher-Chianelli commented Jun 20, 2017

Christopher-Chianelli commented Jun 22, 2017

romartin commented Jun 22, 2017

romartin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasys commented Jun 23, 2017

Christopher-Chianelli commented Jun 23, 2017

romartin left a comment

Choose a reason for hiding this comment

hasys commented Jun 26, 2017

hasys left a comment

Choose a reason for hiding this comment

Christopher-Chianelli commented Jun 26, 2017

Christopher-Chianelli commented Jun 26, 2017

hasys commented Jun 27, 2017

jomarko left a comment

Choose a reason for hiding this comment

romartin Jun 13, 2017 •

edited

Loading

Christopher-Chianelli Jun 13, 2017 •

edited

Loading

romartin Jun 13, 2017 •

edited

Loading

romartin Jun 13, 2017 •

edited

Loading

jomarko Jun 14, 2017 •

edited

Loading