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

Prevent incorrect application of list widget on incompatible expressions #6771

Merged
merged 12 commits into from
May 24, 2023

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented May 18, 2023

Fixes #6760
Fixes #6747

Pull Request Description

Refactored the logic behind selecting appropriate widgets for span tree nodes. Now the bulk of it is moved into widget methods. When a given widget type is reporting to be not compatible with the expression, it will not be used even if the configuration was overriden using an method argument annotation. In that case, the usual logic for automatically selecting the appropriate widget will kick in.

image

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Frizi Frizi force-pushed the wip/frizi/widget-node-match branch from 6a6fed0 to 3c13ccc Compare May 18, 2023 22:52
@Frizi Frizi added the CI: No changelog needed Do not require a changelog entry for this PR. label May 19, 2023
@Frizi Frizi marked this pull request as ready for review May 19, 2023 10:42
@Frizi Frizi requested a review from farmaazon May 19, 2023 17:59
@farmaazon
Copy link
Contributor

farmaazon commented May 22, 2023

QA Report 🔴

  1. One of the reported issues IDE doesn't render Vector concatenation. #6747 is not fully fixed: the second problem (expression not visible entirely on syntax error) still occurs:

image

(The node's expression is [1, 2, 3] + var1 + [4, 6, "])

  1. I cannot connect to gray port if it's just a label (drop-downs and list editors can be connected into).

image

Similarly, here I cannot connect to "than", because the entire argument is selected:

image

@wdanilo
Copy link
Member

wdanilo commented May 23, 2023

@farmaazon good catches!

@Frizi
Copy link
Contributor Author

Frizi commented May 23, 2023

@farmaazon Fixed mentioned issues, ready for second QA round.
image

Note that in above node with incorrect syntax, the right hand side "almost-vector" is not actually recognized as such due to how the expression is parsed. It is in fact not a even distinct subtree that could receive a single widget. Therefore it will not allow drag and drop.

Simplified span-tree:

[1, 2, 3] + var1 + [4, 6, "] Root
[1, 2, 3] + var1 + [4, 6, "]  ╰─Argument
[1, 2, 3] + var1 + [4, 6         ├─Argument
[1, 2, 3] + var1 + [4, 6         │  ╰─Argument
[1, 2, 3] + var1 + [4            │     ├─Argument
[1, 2, 3] + var1                 │     │  ├─Chained
[1, 2, 3]                        │     │  │  ├─Argument
[                                │     │  │  │  ├─Token
 1                               │     │  │  │  ├─Argument
  ,                              │     │  │  │  ├─Token
    2                            │     │  │  │  ├─Argument
     ,                           │     │  │  │  ├─Token
       3                         │     │  │  │  ├─Argument
        ]                        │     │  │  │  ╰─Token
          +                      │     │  │  ├─Operation
            var1                 │     │  │  ╰─Argument
                 +               │     │  ├─Operation
                   [4            │     │  ╰─Argument
                   [             │     │     ├─Operation       // This subtree looks weird, but all of those
                   [             │     │     │  ╰─Argument     // nodes received a distinct unique AST IDs.
                   [             │     │     │     ╰─Argument 
                   [             │     │     │        ╰─Token
                    4            │     │     ╰─Argument
                     ,           │     ├─Operation
                       6         │     ╰─Argument
                        ,        ├─Operation
                          "]     ╰─Argument

@farmaazon
Copy link
Contributor

QA report 2: 🟢

Issues fixed, no more found.

@Frizi Frizi added the CI: Ready to merge This PR is eligible for automatic merge label May 23, 2023
@Frizi Frizi added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 24, 2023
@Frizi Frizi added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Ready to merge This PR is eligible for automatic merge labels May 24, 2023
@Frizi Frizi force-pushed the wip/frizi/widget-node-match branch from 6a7f283 to 9884907 Compare May 24, 2023 11:21
@mergify mergify bot merged commit 4cbd5f4 into develop May 24, 2023
@mergify mergify bot deleted the wip/frizi/widget-node-match branch May 24, 2023 12:30
Procrat added a commit that referenced this pull request May 26, 2023
* develop:
  Allow casting a Mixed column into a concrete type (#6777)
  Stop graph editing when in full-screen visualization mode (#6844)
  Handle `show-dashboard` event (#6837)
  Fix some dashboard issues (#6668)
  Fix JWT leak (#6815)
  Fix "set username" screen (#6824)
  Fallback to opened date when ordering projects (#6814)
  Various test improvements to increase coverage and speed things up (#6820)
  do not activate nested dropdowns together (#6830)
  Clearly select single specialization with enum dispatch pattern (#6819)
  Prevent incorrect application of list widget on incompatible expressions (#6771)
Procrat added a commit that referenced this pull request May 30, 2023
…le-6756-6804

* develop: (22 commits)
  Coalesce graph editor view invalidations (#6786)
  Append warnings extracted before tail call execution (#6849)
  Detect and override hooks of the same kind (#6842)
  Dynamic app resampling and better performance measurements. (#6595)
  Show spinner when opening/creating a project, take #2 (#6827)
  Infrastructure for testing inter project imports and exports (#6840)
  Only initialise visualisation chooser if it is used. (#6758)
  Allow casting a Mixed column into a concrete type (#6777)
  Stop graph editing when in full-screen visualization mode (#6844)
  Handle `show-dashboard` event (#6837)
  Fix some dashboard issues (#6668)
  Fix JWT leak (#6815)
  Fix "set username" screen (#6824)
  Fallback to opened date when ordering projects (#6814)
  Various test improvements to increase coverage and speed things up (#6820)
  do not activate nested dropdowns together (#6830)
  Clearly select single specialization with enum dispatch pattern (#6819)
  Prevent incorrect application of list widget on incompatible expressions (#6771)
  Update GraalVM to 22.3.1 JDK17 (#6750)
  Import/export syntax error have more specific messages (#6808)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
4 participants