AF-1987: Create a client side version of all appformer gwt startup services #724

jesuino · 2019-07-03T00:44:24Z

This is the work for AF-1987. Dependencies that requires a server side dependency have a *-client-backend version:

uberfire-security-client-backend
uberfire-preferences-client-backend
uberfire-workbench-client-backend
uberfire-client-backend

Additional an annotation @WorkbenchClientEditor was created and a processor for this annotation generates the necessary activity.

Later work (jesuino' commits) include fixing tests, fixing webapps and adding new tests to cover the processor and a new panel.

During our tests we found that the dependency uberfire-preference-client-backend should be added to the webapps so it will work correctly. Otherwise the webapp fails during GWT compilation

These are the PRs that must be merged with this:

kiegroup/kie-wb-common#2738
kiegroup/kie-wb-distributions#944
kiegroup/jbpm-wb#1365
kiegroup/drools-wb#1181
kiegroup/optaplanner-wb#340

This PR is declaring some new dependencies versions introduced by this PR: kiegroup/droolsjbpm-build-bootstrap#985

jesuino · 2019-07-03T00:44:41Z

cc @ederign @tiagobento would you please review this PR?

Thanks.

domhanak · 2019-07-03T08:24:23Z

...ench/uberfire-workbench-client/src/main/java/org/uberfire/client/mvp/ActivityBeansCache.java

+        nativeRegisterGwtEditorProvider();
+    }
+
+    private native void nativeRegisterGwtEditorProvider() /*-{


Do you mind moving the native code to a separate class? Having it here decreases the readability of the ActivityBeansCache

domhanak · 2019-07-03T08:31:02Z

...ava/org/uberfire/client/workbench/panels/impl/SimpleNoExpandWorkbenchPanelPresenterTest.java

+        presenter.addPart(workbenchPartPresenter);
+        verify(placeManager).tryClosePlace(any(), any());
+    }
+


domhanak · 2019-07-03T08:35:51Z

uberfire-api/src/main/java/org/uberfire/lifecycle/GetContent.java

+@Inherited
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+public @interface GetContent {


Could you please add JavaDoc for this annotation?

domhanak · 2019-07-03T08:36:04Z

uberfire-api/src/main/java/org/uberfire/lifecycle/SetContent.java

+@Inherited
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+public @interface SetContent {


Could you please add JavaDoc for this annotation?

domhanak · 2019-07-03T08:37:18Z

uberfire-client-api/src/main/java/org/uberfire/client/annotations/WorkbenchClientEditor.java

+
+/**
+ * 
+ * This annotation works like {@code @WorkbenchEditor}, and can be used for editors client side only editors.


..used for editors client side only editors. -> ...used for client side only editors

domhanak · 2019-07-03T08:42:16Z

...lient/src/main/java/org/uberfire/preferences/client/store/PreferenceBeanStoreClientImpl.java

+    public <U extends BasePreference<U>, T extends BasePreferencePortable<U>> void save(final T portablePreference,
+                                                                                        final Command successCallback,
+                                                                                        final ParameterizedCommand<Throwable> errorCallback) {
+


nitpick: maybe we could add comments on why save is not implemented here

...orkbench/uberfire-workbench-client/src/main/java/org/uberfire/client/mvp/IsClientEditor.java

+@Inherited
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE})
+public @interface IsClientEditor {


domhanak · 2019-07-03T08:46:06Z

...h/uberfire-workbench-client/src/main/java/org/uberfire/client/mvp/IsTemplatePerspective.java

+@Inherited
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE})
+public @interface IsTemplatePerspective {


Could we please add JavaDoc for these annotation?

...re-workbench-client/src/main/java/org/uberfire/client/mvp/WorkbenchClientEditorActivity.java

+ * An Editor is an activity that is associated with a VFS path. It is expected that the editor will provide the end user
+ * some means of editing and saving the resource represented by the VFS path.
+ */
+public interface WorkbenchClientEditorActivity extends WorkbenchActivity {


...kbench-client/src/main/java/org/uberfire/client/workbench/widgets/listbar/ListBarWidget.java

@@ -69,4 +69,6 @@
     * Disable support to close parts.
     */
    void disableClosePart();
+
+    void disableExpandPart();


domhanak · 2019-07-03T09:04:56Z

...cessors/src/main/java/org/uberfire/annotations/processors/ClientEditorActivityGenerator.java

+
+        String identifier = null;
+
+        for (final AnnotationMirror am : classElement.getAnnotationMirrors()) {


imho this for-cycle could be extracted to a private method (getIdentifier) for better readability

domhanak · 2019-07-03T09:08:59Z

...cessors/src/main/java/org/uberfire/annotations/processors/ClientEditorActivityGenerator.java

+            onStartup1ParameterMethodName = onStartupMethod.getSimpleName().toString();
+        }
+
+        final String onMayCloseMethodName = GeneratorUtils.getOnMayCloseMethodName(classElement,


I think you could construct map of Map<String, ExecutableElement> and then the following parts of code could just process this map in cycle. The map could be created in separate method for better readability.

Sorry, I didn't understand exactly how this could be changed. I think the code for this was based on EditorActivityGenerator. I could make some changes if you want. One example is this if/else to determine if there's a startup method and if it has params, it looks like a boolean logic to me, where 1 is the method name and 0 null, so:

startup exists startup has params noParam withParam 0 0 0 0 1 0 1 0 0 1 0 0 1 1 0 1

So the resulting code:

final String onStartup0ParameterMethodName = (onStartupMethod != null && onStartupMethod.getParameters().isEmpty()) ? onStartupMethod.getSimpleName().toString() : null; final String onStartup1ParameterMethodName = (onStartupMethod != null && !onStartupMethod.getParameters().isEmpty()) ? onStartupMethod.getSimpleName().toString() : null;

which looks worse.

The whole method could be condensed in a map, and late a for in the map fill the methods accordingly, but I would vote to avoid change it because it is, as I said, based on EditorActivityGenerator. What do you think?

I meant something like this

Map<String, String> methodNameMap = ... // or ExecutableElement and you could call setContentMethod.getSimpleName().toString(); on each value of the map later // but I see that GeneratorUtils is not uniform and returns String in some cases and ExecutableElement in others map.put("onCloseMethodName", onCloseMethodName);

domhanak · 2019-07-03T09:09:16Z

...cessors/src/main/java/org/uberfire/annotations/processors/ClientEditorActivityGenerator.java

+/**
+ * A source code generator for Activities
+ */
+public class ClientEditorActivityGenerator extends AbstractGenerator {


I am missing test for this

I also missed the test for this, however, I noticed that it is tested by the processor itself. The processor calls the activity generator and when testing it we can check the activity generator results from there ( see WorkbenchClientEditorProcessorTest). I did not find specific tests for Activity Generators, but I break this in smaller methods (as you mentioned above) and unit test them. Please let me know what you think about it.

Well, I was thinking more unit level, like testing that calling generate with some values returns expected StringBuffer

But I see your point, would it be possible to extend it to cover Lines 87 - 92? those two branches are not covered.

domhanak · 2019-07-03T09:12:48Z

...cessors/src/main/java/org/uberfire/annotations/processors/ClientEditorActivityGenerator.java

+                                                               qualifiers));
+        }
+
+        //Validate getWidgetMethodName and isWidget


Suggestion: we could validate all 6 cases, collect errors and only throw exception afterwards. Instead of fixing one problem at a time, you could fix them all at once.

I agree, but as I said above, it is looks to be based on the EditorActivityGenerator hence I was resistant to modify this class specifically

Imho, if we have an opportunity to improve, we should take it. Why copy old patterns when they can be improved?
After some closer code inspection I would leave this part as is and maybe improved the setup as i suggested here

domhanak

Left some comments to start with, once the build passes I will continue with local testing

jesuino · 2019-07-03T15:40:36Z

I see enforcer rules issues, I am checking it locally and will update the PR soon and then trigger a new build

tiagobento · 2019-07-03T14:14:46Z

dashbuilder/dashbuilder-webapp/src/main/resources/META-INF/ErraiApp.properties

@@ -66,4 +66,4 @@ errai.marshalling.serializableTypes=org.dashbuilder.dataprovider.StaticProviderT
                                    org.dashbuilder.dataset.sort.ColumnSort \
                                    org.dashbuilder.dataset.sort.DataSetSort \
                                    org.dashbuilder.dataset.sort.SortedList
-
+errai.ioc.enabled.alternatives=org.uberfire.preferences.client.store.PreferenceBeanStoreClientImpl


Does this work? I can be mistaken, but I think uberfire-security-client-backend already has an alternative enabled for a PreferenceBeanStore.. don't they conflict during GWT compilation?

When I remove it I have the following error:

Unsatisfied constructor dependency @Default org.uberfire.preferences.shared.bean.PreferenceBeanStore for Class @Any @org.uberfire.ext.preferences.client.central.tree.TreeView() org.uberfire.ext.preferences.client.central.tree.TreeHierarchyStructurePresenter. Some beans were found that satisfied this dependency, but must be enabled: [INFO] org.uberfire.preferences.client.store.PreferenceBeanStoreClientImpl

I did change from uberfire-preference-client-backend to uberfire-preference-client, perhaps I should keep uberfire-preference-client-backend?

Fixed by also adding uberfire-preference-client dependency.

jesuino · 2019-07-03T23:25:21Z

@domhanak Thanks for your comments, I addressed most of them in #373cbaa246c3f99ed80e62e0b297feca5a6c3472

I am not changing the ClientEditorActivityGenerator for the reasons I commented above, let me know your thoughts.

I need also to test more apps with the changes (part of AF-2018), but if you see any issue please let me know.

jesuino · 2019-07-03T23:25:27Z

Jenkins execute full downstream build

jesuino · 2019-07-04T20:24:06Z

Jenkins execute full downstream build

tiagobento · 2019-07-05T14:46:33Z

Jenkins please retest this

tiagobento · 2019-07-05T14:46:47Z

Jenkins execute full downstream build

ederign · 2019-07-05T18:28:02Z

Jenkins please retest this

jesuino · 2019-07-05T20:13:10Z

thanks @tiagobento

domhanak · 2019-07-09T07:15:10Z

...ench/uberfire-workbench-client/src/main/java/org/uberfire/client/mvp/ActivityBeansCache.java

@@ -296,6 +323,9 @@ private boolean activitySupportsPath(ActivityAndMetaInfo activity, Path path) {
        return new ArrayList<String>(activitiesById.keySet());
    }

+    public void noOp() {


A small comments inside of this method explaining it could help with readiability

// Does nothing, simulates...

jesuino · 2019-07-11T16:18:02Z

Jenkins execute full downstream build

…I bean

jesuino · 2019-07-11T22:41:04Z

Jenkins execute full downstream build

domhanak · 2019-07-12T10:41:36Z

jenkins retest this

domhanak · 2019-07-12T11:55:42Z

@jesuino

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M1:enforce (no-duplicate-declared-dependencies) on project dashbuilder-services: Some Enforcer rules have failed

Looks like something is wrong. But I am not sure if it has something to do with the changes in the PR yet.

ederign · 2019-07-12T14:55:55Z

The dashbuilder failures should probably be a lack of rebase.

ederign · 2019-07-12T15:55:26Z

Jenkins execute full downstream build

domhanak · 2019-07-15T13:01:16Z

jenkins execute full downstream build

jesuino · 2019-07-15T22:29:41Z

I will trigger another build, but please notice that the failing tests seems unrelated

jesuino · 2019-07-15T22:31:11Z

Jenkins execute full downstream build

tiagobento · 2019-07-16T13:46:28Z

These tests failed, but they don't look related to me.

- org.jbpm.executor.impl.wih.AsyncContinuationSupportTest.testAsyncModeWithParallelGateway
- org.kie.server.integrationtests.drools.pmml.ApplyScorecardMultiModuleIntegrationTest.org.kie.server.integrationtests.drools.pmml.ApplyScorecardMultiModuleIntegrationTest
- org.jbpm.executor.cdi.CDISimpleExecutorTest.org.jbpm.executor.cdi.CDISimpleExecutorTest

@jesuino @ederign @domhanak If you agree, it looks good for me to merge.

jesuino · 2019-07-16T15:31:09Z

@tiagobento I also think that these tests failures are unrelated to this PR, hence I would vote to merge it - also merge all the other related PRs,please

jesuino · 2019-07-17T20:27:43Z

Jenkins execute full downstream build

ederign · 2019-07-17T21:47:04Z

Testing failures are not related: org.jbpm.executor.impl.wih.AsyncContinuationSupportTest.testAsyncModeWithParallelGateway
org.kie.server.integrationtests.drools.pmml.ApplyScorecardMultiModuleIntegrationTest.org.kie.server.integrationtests.drools.pmml.ApplyScorecardMultiModuleIntegrationTest
org.jbpm.executor.cdi.CDISimpleExecutorTest.org.jbpm.executor.cdi.CDISimpleExecutorTest

domhanak reviewed Jul 3, 2019

View reviewed changes

...orkbench/uberfire-workbench-client/src/main/java/org/uberfire/client/mvp/IsClientEditor.java

@Inherited

@Retention(RetentionPolicy.RUNTIME)

@Target({ElementType.TYPE})

public @interface IsClientEditor {

This comment was marked as outdated.

Sign in to view

domhanak reviewed Jul 3, 2019

View reviewed changes

...kbench-client/src/main/java/org/uberfire/client/workbench/widgets/listbar/ListBarWidget.java

@@ -69,4 +69,6 @@

* Disable support to close parts.

*/

void disableClosePart();

void disableExpandPart();

This comment was marked as outdated.

Sign in to view

domhanak reviewed Jul 3, 2019

View reviewed changes

tiagobento reviewed Jul 3, 2019

View reviewed changes

jesuino force-pushed the AF-1987 branch from b41c287 to 73f5cdc Compare July 3, 2019 23:19

jesuino force-pushed the AF-1987 branch from 73f5cdc to 9cbb40a Compare July 4, 2019 18:24

domhanak reviewed Jul 9, 2019

View reviewed changes

porcelli and others added 7 commits July 11, 2019 14:10

AF-1987: Initial changes to allow editors to work offline

ce79449

AF-1987: Fixing tests and enforcer issues

7f50242

AF-1987: Fixing webapps to pass GWT compilation

91bfb86

AF-1987: Adding new tests

8629353

AF-1987: Fixes and improvements

aad676a

AF-1987: Fixing webapps to use the correct preference implementation

7fe00f8

AF-1987: Removing old commend and making GWTEditorNativeRegister a CD…

efd725f

…I bean

jesuino mentioned this pull request Jul 11, 2019

AF-1987: Adding client with backend calls dependencies kiegroup/droolsjbpm-build-bootstrap#985

Merged

AF-1987: Fixing wires and commenting noOp method

7d1c36f

jesuino force-pushed the AF-1987 branch from e2dd2c6 to 7d1c36f Compare July 11, 2019 22:40

AF-1987: Fixing enforcer issue

cd9b131

domhanak approved these changes Jul 17, 2019

View reviewed changes

ederign approved these changes Jul 17, 2019

View reviewed changes

ederign merged commit b88c5b0 into kiegroup:master Jul 17, 2019


		String identifier = null;

		for (final AnnotationMirror am : classElement.getAnnotationMirrors()) {

AF-1987: Create a client side version of all appformer gwt startup services #724

AF-1987: Create a client side version of all appformer gwt startup services #724

Conversation

jesuino commented Jul 3, 2019 • edited

jesuino commented Jul 3, 2019

domhanak Jul 3, 2019 • edited

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

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

domhanak Jul 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domhanak Jul 12, 2019 • edited

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

domhanak left a comment

Choose a reason for hiding this comment

jesuino commented Jul 3, 2019

tiagobento Jul 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesuino commented Jul 3, 2019

jesuino commented Jul 3, 2019

jesuino commented Jul 4, 2019

tiagobento commented Jul 5, 2019

tiagobento commented Jul 5, 2019

ederign commented Jul 5, 2019

jesuino commented Jul 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesuino commented Jul 11, 2019

jesuino commented Jul 11, 2019

domhanak commented Jul 12, 2019

domhanak commented Jul 12, 2019

ederign commented Jul 12, 2019

ederign commented Jul 12, 2019

domhanak commented Jul 15, 2019

jesuino commented Jul 15, 2019

jesuino commented Jul 15, 2019

tiagobento commented Jul 16, 2019

jesuino commented Jul 16, 2019

jesuino commented Jul 17, 2019

ederign commented Jul 17, 2019

jesuino commented Jul 3, 2019 •

edited

domhanak Jul 3, 2019 •

edited

domhanak Jul 3, 2019 •

edited

domhanak Jul 12, 2019 •

edited

tiagobento Jul 3, 2019 •

edited