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

DROOLS-2755: [DMN Designer] Error when the user switches between DMN files #2003

Merged
merged 3 commits into from
Aug 6, 2018
Merged

DROOLS-2755: [DMN Designer] Error when the user switches between DMN files #2003

merged 3 commits into from
Aug 6, 2018

Conversation

manstis
Copy link
Member

@manstis manstis commented Jul 25, 2018

See https://issues.jboss.org/browse/DROOLS-2755

I moved all the @ApplicationScoped injection (that was making the editor unusable as "multi-instance") to use Stunner's CanvasControls and ManagedSession (that acts as a session scoped factory for different controls ). I've previously used this for a cache but now use the mechanism a bit more for the different objects I need at (Stunner) session scope (not to be confused with HTTP session).

@manstis manstis requested review from karreiro and jomarko July 25, 2018 15:09
});
}

private void focusLastParameter(final HasParametersControl hasParameters) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was added in addition to the scope of the JIRA to ensure the text in the "Parameters" popup editor (for a Function) was not selected (as a result of the double click on the canvas). It manifested itself while I was testing the fix in drools-wb-webapp (I thought to fix now, rather than file a new JIRA).

Copy link
Contributor

Choose a reason for hiding this comment

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

Great :-)

@@ -71,6 +81,11 @@ public DMNEditorSession(final ManagedSession session,
stunnerPreferencesRegistry);
}

@Override
public ManagedSession getSession() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes access modifier to public (from protected).

@@ -43,14 +53,25 @@ public DMNViewerSession(final ManagedSession session,
canvasCommandManager);
}

@Override
public ManagedSession getSession() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes access modifier to public (from protected).

@@ -14,6 +14,6 @@
~ limitations under the License.
-->

<div>
<div class="uf-no-select">
Copy link
Member Author

Choose a reason for hiding this comment

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

Prevents the text in the "popup" context menu ("insert row..", "insert column.." etc and the "Expression type" selector for an UndefinedExpressionGrid) from being selected (again, this was found testing this PR in drools-wb-webapp).

@manstis
Copy link
Member Author

manstis commented Jul 25, 2018

Jenkins please retest this.

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Very good approach for the problem, @manstis :-)

(it tried locally, and it's working fine too)

Thank you.

});
}

private void focusLastParameter(final HasParametersControl hasParameters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great :-)

@manstis
Copy link
Member Author

manstis commented Aug 1, 2018

@jomarko This is OK for review now thank-you.

DOMUtil.removeAllChildren(parametersContainer);
parameters.forEach(parameter -> parametersContainer.appendChild(makeParameterView(parameter).getElement()));
parameterViewInstances.forEach(parameterView -> parametersContainer.appendChild(parameterView.getElement()));
}
Copy link

Choose a reason for hiding this comment

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

Missing coverage for this method.

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 Test in new commit.

return;
}
parameterViewInstances.get(index).focus();
}
Copy link

Choose a reason for hiding this comment

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

Missing coverage for this method.

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 Test in new commit.

final Integer width = parentElement.getOffsetWidth();
final Integer height = parentElement.getOffsetHeight();

if (width > 0 && height > 0) {
Copy link

Choose a reason for hiding this comment

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

In unit tests we don't have covered case when this if is false.

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 Test in new commit.

@manstis
Copy link
Member Author

manstis commented Aug 2, 2018

@jomarko This is ready for re-review.

There was a nasty merge conflict that I've waved my magic wand at; however https://issues.jboss.org/browse/DROOLS-2564 could be affected (I tested locally and it appears OK however you might want to double check).

@manstis
Copy link
Member Author

manstis commented Aug 2, 2018

@jomarko PS This PR also fixes https://issues.jboss.org/browse/DROOLS-2765 you might want to double check that too (at the moment a Stunner read-only session prevents the User from viewing the grid view of the graph nodes.. I propose a new JIRA for that).

@jomarko
Copy link

jomarko commented Aug 3, 2018

  • Rechecked DROOLS-2564, found DROOLS-2829 (low priority)
  • Checked DROOLS-2765, but still see [1]

[1]
screenshot from 2018-08-03 08-29-20

@jomarko
Copy link

jomarko commented Aug 3, 2018

jenkins execute full downstream build

@manstis
Copy link
Member Author

manstis commented Aug 3, 2018

Jenkins please execute full downstream build.

@manstis
Copy link
Member Author

manstis commented Aug 3, 2018

Jenkins please execute full downstream build.

@manstis
Copy link
Member Author

manstis commented Aug 6, 2018

Jenkins please execute full downstream build.

@manstis manstis merged commit c8a514c into kiegroup:master Aug 6, 2018
jeremyary pushed a commit to jeremyary/kie-wb-common that referenced this pull request Aug 31, 2018
…files (kiegroup#2003)

* DROOLS-2755: [DMN Designer] Error when the user switches between DMN files

* DROOLS-2755: [DMN Designer] Error when the user switches between DMN files. Updates following peer review.

* DROOLS-2755: [DMN Designer] Error when the user switches between DMN files. Changes following rebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants