Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Default Visualisations Per Type #1737

Closed
1 of 2 tasks
iamrecursion opened this issue May 11, 2021 · 10 comments
Closed
1 of 2 tasks

Default Visualisations Per Type #1737

iamrecursion opened this issue May 11, 2021 · 10 comments
Assignees
Labels
p-medium Should be completed in the next few sprints

Comments

@iamrecursion
Copy link
Contributor

iamrecursion commented May 11, 2021

Summary

The IDE needs to know what is a default visualization for given expression, basing on the type and value of the expression. See https://github.com/enso-org/ide/issues/1548 for more details.

Value

Allow IDE implement https://github.com/enso-org/ide/issues/1548

Specification

  • Implementation of default_visualization method for some standard types, as extension methods in visualization module
  • Add a "one-shot-call" method to the Language Server API: The method should take execution context, expression id and preprocessor (like attaching visualization) and as a result there should be a single binary package: the value of given expression in given execution context processed by preprocessor (because result is binary, probably the whole method should be in binary protocol).

Acceptance Criteria & Test Cases

@iamrecursion iamrecursion added Type: Enhancement p-medium Should be completed in the next few sprints labels May 11, 2021
@4e6 4e6 mentioned this issue May 21, 2021
4 tasks
@4e6
Copy link
Contributor

4e6 commented May 25, 2021

Update.

Waiting for the specification of the default_visualization method from @MichaelMauderer (discussed on discord).

@4e6 4e6 added the s-info-needed Status: more information needed from submitter label May 25, 2021
@MichaelMauderer
Copy link
Contributor

From the IDE perspective it would be best if the default_visualization returned something that has a to_json method. The resulting JSON should have two fields: project and name.

  • name indicates the name of the visualization.
  • project can be either
    • "Builtin" for visualizations provided by the IDE.
    • "CurrentProject" for visualizations in the current project.
    • {"Library": "<PATH>"} where <PATH> is the path to a library where the visualization can be found.

Examples are:

{ "project": "Builtin", "name": "VisCheese" }
{ "project": "CurrentProject", "name": "JunkyChart" }
{ "project": { "Library": "/cool/vis/project" }, "name": "BubbleBars" }

Note to everything in here is actually supported by the IDE at the moment. For example, we can't load visualizations from other libraries.

This proposal maps closely to the current structure that is used in the IDE and would be directly serializable on our end, but we can discuss if modifications to this might improve it, simplify it or make it more future-proof.

The actual values returned right now should all be referring to the existing visualizations which are represented by the following paths:

[
  { "project": "Builtin", "name": "JSON" },
  { "project": "Builtin", "name": "Scatter Plot" },
  { "project": "Builtin", "name": "Histogram" },
  { "project": "Builtin", "name": "Heatmap" },
  { "project": "Builtin", "name": "Table" },
  { "project": "Builtin", "name": "SQL Query" },
  { "project": "Builtin", "name": "Geo Map" },
  { "project": "Builtin", "name": "Image" }
]

@4e6 4e6 removed the s-info-needed Status: more information needed from submitter label May 26, 2021
@4e6 4e6 assigned MichaelMauderer and unassigned 4e6 May 27, 2021
@kustosz
Copy link
Contributor

kustosz commented May 31, 2021

The CurrentProject option is not really feasible – it would be a caller-dependent method – the call to my_value.default_visualization would have to return different values depending on where it is called. This is a can of implicit worms I'm not sure we want to open in the language. Implementing just the Builltin / Library distinction for now.

@MichaelMauderer
Copy link
Contributor

Okay. I guess it should be possible to translate this on the IDE side to a Library call, when we start supporting this.

@kustosz
Copy link
Contributor

kustosz commented Jun 1, 2021

@MichaelMauderer after playing around a bit with this API I'd like to propose the following schema:

  1. If it's a builtin visualization, the structure would be {"library": null, name: "Vis Name"}
  2. If it's a library vis, the structure would be {"library": {"path": "/my/path" }, name: "Vis Name"}
    Rationale:
  3. "library": null is IMO the cleanest way of indicating "this visualization does not come from a library", which is the case for our builtin visualizations – they are all built into the IDE.
  4. "library": {"path": "/my/path"} makes the inner object with path extensible – we can drop more data into it in the future if need be.

@MichaelMauderer
Copy link
Contributor

Sounds sensible to me.

Now I'm just wondering what kind of path we are actually using here. Would this be an import path for a library?

@kustosz
Copy link
Contributor

kustosz commented Jun 1, 2021

That is a very good point. Seems to me like there should be 2 pieces of information needed, but please confirm, as I'm not really sure what the IDE wants to do with this. It probably should include:

  1. The name of the library, so that you can import it in the preprocessor (using some convention for structuring visualization projects, TBD I guess, but anyway lib name + vis name should be enough)
  2. Some on-disk path that could be sent to file manager so that the IDE can download relevant assets?

@MichaelMauderer
Copy link
Contributor

The IDE should probably not deal with on-disk assets. This would be the job of the project manager. Only the project manager can ensure that this is portable to the cloud. So, I guess essentially we need the "fully qualified library name"/import path and the visualization name. Which still fits the API described above.

Maybe path could be named name or import_path in case we do want to do something special that will actually require something more pathy.

@kustosz
Copy link
Contributor

kustosz commented Jun 1, 2021

Yeah, renaming to name.

@iamrecursion
Copy link
Contributor Author

The IDE should probably not deal with on-disk assets. This would be the job of the project manager. Only the project manager can ensure that this is portable to the cloud. So, I guess essentially we need the "fully qualified library name"/import path and the visualization name. Which still fits the API described above.

Maybe path could be named name or import_path in case we do want to do something special that will actually require something more pathy.

The language server is the only component that runs in the cloud. The project manager does not.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
p-medium Should be completed in the next few sprints
Projects
None yet
Development

No branches or pull requests

5 participants