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

211: Revise TypeHints and server side feedback for creation actions #285

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

CamilleLetavernier
Copy link
Member

@CamilleLetavernier CamilleLetavernier commented Sep 11, 2023

  • Refactor EdgeTypeHint definition

    • Add a new 'dynamic' property, indicating that new edges need and additional check with the server before allowing creation
    • Make source/target element type ids properties optional
      If not defined, all potential element types are considered to be valid sources/targets
  • Add RequestEdgeCheckAction and EdgeCheckResultAction response to implement
    the dynamic check

  • Update the EdgeCreationTool to check with the server when trying to
    create a new edge configured as Dynamic

  • Refactor TypeHintsProvider and `ApplyTypeHintsCommand

  • Simplify type-hints aware can connect implementation. Just validate against the source/target types
    of the edge hint that is applicable for the given routable

  • Ensure that the default (i.e. class-level implementation) of canConnect is called if no typehint is applicable to the given routable

  • Remove getValidEdgeElementTypes function from TypeHintsProvider. Was only used
    for the old type-hints aware canConnect implementation. Is no longer needed and incomplete anyways (does not consider nested subtypes)

  • Add tests for type-hints feature
    Fixes Consider unit testing Type Hints glsp#45

  • Remove hasCompatibleType utility function from smodel-util as it is no longer used
    and the implementation was incomplete anyways (did not consider nested subtypes)

  • Also: Don't use stroked lines for edges until Edge edit does not work well with stroked lines glsp#1083 is fixed

Part of eclipse-glsp/glsp/issues/211

Co-authored-by: Camille Letavernier <[email protected] >

Note:

refs eclipse-glsp/glsp#211

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 @CamilleLetavernier.
I only had time to do a preliminary code review for now, but I will also test the changes later today

- Refactor EdgeTypeHint definition
  -  Add a new 'dynamic' property, indicating that new edges need and additional check with the server before allowing creation
  -  Make source/target element type ids properties optional
  If not defined, all potential element types are considered to be valid sources/targets
- Add `RequestEdgeCheckAction` and `EdgeCheckResultAction` response to implement
 the dynamic check
- Update the EdgeCreationTool to check with the server when trying to
create a new edge configured as Dynamic

-Refactor `TypeHintsProvider` and `ApplyTypeHintsCommand
 - Simplify type-hints aware can connect implementation. Just validate against the source/target types
    of the edge hint that is applicable for the given routable 
  - Ensure that the default (i.e. class-level implementation) of  `canConnect` is called if no typehint is applicable to the given routable
 - Remove `getValidEdgeElementTypes` function from `TypeHintsProvider`.  Was only used
    for the old type-hints aware `canConnect` implementation. Is no longer needed and incomplete anyways (does not consider nested subtypes)
- Add tests for type-hints feature
  Fixes eclipse-glsp/glsp#45
- Remove `hasCompatibleType` utility function from `smodel-util` as it is no longer used
  and the implementation was incomplete anyways (did not consider nested subtypes)

- Also: Don't use stroked lines for edges until eclipse-glsp/glsp/issues/1083 is fixed

Part of eclipse-glsp/glsp/issues/211

Co-authored-by: Camille Letavernier <[email protected] >
@tortmayr tortmayr force-pushed the issues/211-edgeHints branch from 796399b to f16c040 Compare September 24, 2023 17:16
@tortmayr tortmayr marked this pull request as ready for review September 24, 2023 17:17
@tortmayr
Copy link
Contributor

tortmayr commented Sep 24, 2023

@martin-fleck-at I have taken over this draft from Camille and have finalized the PR. Could you please have a look at it?

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

I did a quick test on this and the changes look good to me and work as expected. I do have some minor comments that could be integrated but nothing related to the core functionality. Thank you!

@tortmayr tortmayr requested review from martin-fleck-at and removed request for tortmayr September 26, 2023 07:49
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

LGTM!

@tortmayr tortmayr self-requested a review September 26, 2023 13:15
@tortmayr tortmayr merged commit 5435240 into master Sep 26, 2023
5 checks passed
@tortmayr tortmayr deleted the issues/211-edgeHints branch September 26, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider unit testing Type Hints
3 participants