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

Handlers for Smart Connector UIExtension #70

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

yentelmanero
Copy link

@yentelmanero yentelmanero commented Nov 30, 2023

This PR is part of my bachelor research conducted in TU Wien in the Business Informatics Group.


In this PR, the ActionHandler and ContextActionProvider as well as the related workflow example implementations for the Smart Connector UIExtension are included (more info on Smart Connector in Client PR)

Client PR
Theia PR

@haydar-metin
Copy link

haydar-metin commented Dec 19, 2023

Thanks for the PR! I will test it after the changes are done!

Can you also add some description to your PR and reference the other PRs you have created, something like:

This PR is part of my bachelor research conducted in TU Wien in the [Business Informatics Group](https://www.big.tuwien.ac.at/).

---

<Little Description>

<Other PR references>

packages/server/src/common/di/diagram-module.ts Outdated Show resolved Hide resolved

createSmartConnectorGroupItem(handlers: CreateOperationHandler[], kind: string, selectedNodeType: string,
showOnly?: SmartConnectorGroupUIType): PaletteItem[] {
const includedInNodeFilter = (e: string): boolean => this.nodeOperationFilter[selectedNodeType].includes(e);

Choose a reason for hiding this comment

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

The this.nodeOperationFilter[selectedNodeType].includes(e) part can cause an error if the selectedNodeType does not exist in nodeOperationFilter. Check the updated types provided before.

Choose a reason for hiding this comment

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

Solution could be something like !!this.nodeOperationFilter[selectedNodeType]?.includes(e)

Copy link

@haydar-metin haydar-metin left a comment

Choose a reason for hiding this comment

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

LGTM, just small changes

@yentelmanero
Copy link
Author

Did not mean to mention that PR with that last commit

@tortmayr tortmayr self-requested a review March 25, 2024 08:00
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks, in general the code looks good to me.
I have a few inline comments/suggestions.
As discussed offline we like to have a more generic SelectionPaletteProvider API instead of the SmartConnectorAPI. Please rename the artifacts accordingly.

Other than that the change works really well!

};

protected override nodeOperationFilter = {
[ModelTypes.AUTOMATED_TASK]: [ModelTypes.WEIGHTED_EDGE, ModelTypes.AUTOMATED_TASK, ModelTypes.MANUAL_TASK,
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be handled generically. All the required information is present in the typehints of the DiagramConfiguration.
Could be moved up to the DefaultSmartConnectorProvider.

I don't expect that this is handled with this PR but we should create a follow-up for that.

* A {@link ContextActionsProvider} for {@link PaletteItem}s in the Smart Connector which appears when a node is selected.
*/
@injectable()
export abstract class SmartConnectorItemProvider implements ContextActionsProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline we would like to use a more generic SelectionPaletteProvider API (that is inline with CommandPaletteProvider and ToolpaletteProvider) and the current SmartConnector is then just the default implementation of that provider.
So please rename this to SelectionPaletteProvider.

};

@injectable()
export class DefaultSmartConnectorItemProvider extends SmartConnectorItemProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export class DefaultSmartConnectorItemProvider extends SmartConnectorItemProvider {
export class DefaultSelectionPaletteProvider` extends SelectionPaletteProvider {

* Returns the context id of the provider.
*/
get contextId(): string {
return 'smart-connector';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return 'smart-connector';
return 'selection-palette';

@@ -54,6 +54,14 @@ export abstract class GModelCreateNodeOperationHandler extends GModelOperationHa
container.children.push(element);
element.parent = container;
this.actionDispatcher.dispatchAfterNextUpdate(SelectAction.create({ selectedElementsIDs: [element.id] }));
// Creates default edge on node creation when a source ID is given in the CreateNodeOperation
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why this is done this way, but we should find a more generic solution here.

Ideally we would dispatch a CompoundOperation on the client-side that contains the CreateNode and CreateEdge operations. Currently the API does not support this, because we need context-specific information. i.e. the CreateEdge operation needs to know the id of the newly created node it should connect to.

We should create a follow-up for that.

import { GNode } from '@eclipse-glsp/graph';

@injectable()
export class OpenSmartConnectorActionHandler implements ActionHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be handled entirely on client side.
The server should only provide the selection palette actions via the RequestContextActions handler but should not need to know any other implementation details about how they will be handled/rendered on the client side.

@@ -216,6 +219,7 @@ export abstract class DiagramModule extends GLSPModule {
binding.add(SaveModelActionHandler);
binding.add(UndoRedoActionHandler);
binding.add(ComputedBoundsActionHandler);
binding.add(OpenSmartConnectorActionHandler);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be handled on client side

@tortmayr
Copy link
Contributor

@yentelmanero
Thank your for this awesome contribution.
I have reviewed and tested the changes. Everything seems to work as expected and all review comments have been addressed.
Nice work!

We will keep this PR open and merge it after the 2.2.0 Release is done (end of May). This way we have a full release cycle to polish the feature and address follow-up issues before we are shipping it to adopters.

In any case, from our side this concludes the practical aspect of your bachelor thesis. We don't require/desire any more changes from your side. Congrats!

@tortmayr tortmayr self-requested a review April 24, 2024 10:25
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