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

Fallback to default visualization based on type, when no viz has been explicitly set #8389

Merged
merged 12 commits into from
Dec 1, 2023

Conversation

somebody1234
Copy link
Contributor

@somebody1234 somebody1234 commented Nov 24, 2023

Pull Request Description

  • Closes Default visualization #8386
    • Attempts to execute <expr>.default_visualization to query the engine for the correct fallback type
    • If that is not possible, falls back to checking inputType - first for an exact match, then falling back to Any (i.e. the text/JSON visualization)
    • Does not decide fallback based on the shape of the returned JSON

Important Notes

Contains pretty significant refactors of VisualizationMetadata to allow it to be unset.

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.

@somebody1234 somebody1234 added CI: No changelog needed Do not require a changelog entry for this PR. -gui labels Nov 24, 2023
app/gui2/shared/yjsModel.ts Outdated Show resolved Hide resolved
app/gui2/src/components/GraphEditor/GraphVisualization.vue Outdated Show resolved Hide resolved
app/gui2/src/components/GraphEditor/GraphVisualization.vue Outdated Show resolved Hide resolved
app/gui2/src/components/GraphEditor/GraphVisualization.vue Outdated Show resolved Hide resolved
app/gui2/src/components/GraphEditor/GraphVisualization.vue Outdated Show resolved Hide resolved
@somebody1234
Copy link
Contributor Author

spent way too long debugging an issue caused by me, when return (implicit undefined return) when converting to file format when identifier was null, meaning viz visibility was not saved when explicitly using default_visualization.

i haven't checked GUI1 but am assuming this must break compatibility with it, as the original impl lacked nullable project in the file storage format for viz metadata.

@somebody1234 somebody1234 added the x-new-feature Type: new feature request label Nov 28, 2023
app/gui2/src/stores/graph/index.ts Outdated Show resolved Hide resolved
app/gui2/ydoc-server/fileFormat.ts Outdated Show resolved Hide resolved
@somebody1234
Copy link
Contributor Author

merged develop into this branch because GitHub says there were conflicts (probably due to the moved files), but merge-ort had no issues

@Frizi Frizi added the CI: Ready to merge This PR is eligible for automatic merge label Dec 1, 2023
@mergify mergify bot merged commit c60bf6e into develop Dec 1, 2023
32 of 33 checks passed
@mergify mergify bot deleted the wip/sb/default-viz-simple branch December 1, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge x-new-feature Type: new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default visualization
3 participants