Ensure Table Visualisation is the Default Visualization for Tables #4120

MichaelMauderer · 2023-02-03T15:08:14Z

Pull Request Description

Adds a mechanism for the GUI to choose an appropriate default visualization for a newly created node.

Peek.2023-02-03.15-26.mp4

Important Notes

This is a workaround for missing functionality that would take much longer to implement.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

…ation_For_Table_184267396

JaroslavTulach

Yup, having tables on by default will make life of everyone way easier.

JaroslavTulach · 2023-02-04T10:37:34Z

app/gui/view/graph-editor/src/builtin/visualization/java_script/table.js

@@ -11,7 +11,7 @@ loadStyleFromString(scrollbarStyle)
 // ===========================

 class TableVisualization extends Visualization {
-    static inputType = 'Any'
+    static inputType = 'Standard.Table.Data.Table.Table'


Unit tests have been written where possible.

Is there a test to fail if the Table type is renamed? Types get renamed quite frequently in the stdlib and I'd be sad to see this functionality stop working without a warning/failing test.

Unit tests have been written where possible.

Is there a test to fail if the Table type is renamed? Types get renamed quite frequently in the stdlib and I'd be sad to see this functionality stop working without a warning/failing test.

That is a good point. But I am somewhat unsure how to do this, as we don't currently have working integration tests.
Do you think it would be sensible to put in a test in the standard library that checks the name of the type of Table and says something like "If you need to change this test, also update visualization/java_script/table.js:14? "

farmaazon · 2023-02-06T09:48:54Z

app/gui/view/graph-editor/src/component/visualization/container.rs

+        let result = next_on_list.or_else(|| vis_list.first()).cloned();
+        result


Return directly. Linter should complain about it.

farmaazon · 2023-02-06T09:51:39Z

app/gui/view/graph-editor/src/component/visualization/registry.rs

+        // Visualisations are order by "matching the type" first, followed by and then "matching any
+        // type". So we just take the first one, which should be the most appropriate one.
+        // This should be replaced with the proper solution described in
+        // https://www.pivotaltracker.com/story/show/184390437


Please add TODO mark in this comment.

…ation_For_Table_184267396

radeusgd · 2023-02-06T21:33:01Z

app/gui/view/graph-editor/src/builtin/visualization/java_script/table.js

-    static inputType = 'Any'
+    static inputType = 'Standard.Table.Data.Table.Table'


I tried building the IDE to verify this, but after wasting a lot of time I'm still having some troubles using the new build script on Windows (I will try figure this out in due course).

In the meantime, I have a hunch (the one I wanted to verify before posting, but got impatient): if we specify the inputType of visualization to be Standard.Table.Data.Table.Table, will the Table visualization even show at all for Database Tables whose type is Standard.Database.Data.Table.Table? I think it is important to not break the Table visualization on the Database module and would consider breaking it as a regression.

I'm not sure if this PR causes it (as I was unable to verify empirically), but looking at the code I'm afraid it may.

btw. cc @jdunkerley I think this is another argument for why we should migrate towards a single facade Table type that will only then delegate to the proper backend (be it DB backend or in-memory) under the hood. Having one Table type instead of 2 (or more) will be much better. Will probably be worth doing as part of the SQL IR improvements.

For now though, it would be good to ensure if we can keep the Table visualization on Database Tables too. Can we use sum types in inputType?

While this is only a temporary solution until such time as we have the method calling default_visualization, @radeusgd is correct we need this on a few more types. Could we make the inputType an array of types instead of a single value?

For now, Table viz should be the default for Vector, Array, In Memory Column and In Memory Table.
And SQL should be the default for Database Column and Database Table.

We are very likely to increase this list over the next few days (I want to name Map and JS Object for example default to the Table) so this mechanism is critical assuming that #5195 will not be completed for a while.

@radeusgd - I don't think the facade type has anything to do here.

And at least so far attempts to use such a type within Enso have been annoying. I tried this for the database connection type with a facade calling onto a backing true connection but the approach was very unpleasant and didn't work well. Either way, a discussion for a different space.

There can be any number of types, but there is one limitation: the matching is done on the name as a string. The only special handling is done for Any. So I can add that back in to ensure this stays usable for all types. And I can add more types for which it makes sense for this to be the default.

So the list of types to add is

Standard.Database.Data.Table.Table

Vector

Array

In Memory Column

In Memory Table

Any other ones that come to mind? @jdunkerley @radeusgd

yes please. For the first pass:

Table:

Standard.Base.Data.Vector.Vector

Standard.Base.Data.Array.Array

Standard.Table.Data.Table.Table

Standard.Table.Data.Column.Column

and lets make SQL viz the default for:

Standard.Database.Data.Table.Table

Standard.Database.Data.Column.Column

We can then hopefully easily add to this as we need later on. (ideally without bugging your team!)

I'd just like to ensure that while for

Standard.Database.Data.Table.Table

Standard.Database.Data.Column.Column
we have the SQL visualization as the default, the Table visualization should still be available to choose for these types.

yes the Table viz should definitely be available for it.

Table:

* `Standard.Base.Data.Vector.Vector` * `Standard.Base.Data.Array.Array` * `Standard.Table.Data.Table.Table` * `Standard.Table.Data.Column.Column`

All added.

and lets make SQL viz the default for:

* `Standard.Database.Data.Table.Table` * `Standard.Database.Data.Column.Column`

Already seem to be defined as the input for SQL vis, so they should have it as default now.

@MichaelMauderer / @farmaazon is #5195 planned for the release?

I don't think it is. @farmaazon ?

I'd just like to ensure that while for

* `Standard.Database.Data.Table.Table` * `Standard.Database.Data.Column.Column` we have the SQL visualization as the default, the `Table` visualization should still be available to choose for these types.

I've re-added “Any” to Table Vis can be chosen for every node. This might be the best compromise here until we get the proper system.

jdunkerley

This is a great step forward. Thanks for getting this in place.

…ation_For_Table_184267396

Closes #5639 This PR fixes a regression introduced in #4120, due to which new nodes that were being edited had an empty visualization preview. Now newly created visualization containers do get a default visualization set even before the visualization's input type is set. https://user-images.githubusercontent.com/117099775/221256294-c87a50da-c8b0-4f00-bf84-f5ae551fca72.mp4

MichaelMauderer self-assigned this Feb 3, 2023

MichaelMauderer added 2 commits February 3, 2023 15:21

Implement workaround for missing default visualisation functionality,

4aba000

Update changelog.

8682a25

MichaelMauderer force-pushed the wip/MichaelMauderer/Fix_Defaullt_Visualisation_For_Table_184267396 branch from 0573b07 to 8682a25 Compare February 3, 2023 15:30

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

295cb23

…ation_For_Table_184267396

MichaelMauderer marked this pull request as ready for review February 3, 2023 15:31

MichaelMauderer requested review from wdanilo and farmaazon as code owners February 3, 2023 15:31

JaroslavTulach approved these changes Feb 4, 2023

View reviewed changes

farmaazon approved these changes Feb 6, 2023

View reviewed changes

MichaelMauderer mentioned this pull request Feb 6, 2023

Table Vis is not the default visualization for Tables #5033

Closed

MichaelMauderer linked an issue Feb 6, 2023 that may be closed by this pull request

Table Vis is not the default visualization for Tables #5033

Closed

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

012dc05

…ation_For_Table_184267396

radeusgd reviewed Feb 6, 2023

View reviewed changes

MichaelMauderer added 3 commits February 7, 2023 13:33

Implement review feedback.

6852e69

Expand list of defaults.

f7a1633

Add test to avoid regressions.

af3a600

MichaelMauderer requested review from jdunkerley, radeusgd and JaroslavTulach February 7, 2023 13:39

jdunkerley approved these changes Feb 7, 2023

View reviewed changes

MichaelMauderer and others added 2 commits February 7, 2023 13:41

Update link to issue.

a731e55

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

362524c

…ation_For_Table_184267396

MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Feb 7, 2023

mergify bot added 6 commits February 7, 2023 14:58

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

c00d313

…ation_For_Table_184267396

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

4f9e033

…ation_For_Table_184267396

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

9bf35a0

…ation_For_Table_184267396

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

2af88a4

…ation_For_Table_184267396

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

5a92d3c

…ation_For_Table_184267396

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

60cbd3d

…ation_For_Table_184267396

Merge branch 'develop' into wip/MichaelMauderer/Fix_Defaullt_Visualis…

37b8a18

…ation_For_Table_184267396

mergify bot merged commit d3e46e9 into develop Feb 7, 2023

mergify bot deleted the wip/MichaelMauderer/Fix_Defaullt_Visualisation_For_Table_184267396 branch February 7, 2023 23:23

galin-enso mentioned this pull request Feb 23, 2023

Fix missing visualization preview in CB #5757

Merged

4 tasks

hubertp mentioned this pull request Mar 3, 2023

Visualization Sometimes Blank on First View #5620

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure Table Visualisation is the Default Visualization for Tables #4120

Ensure Table Visualisation is the Default Visualization for Tables #4120

MichaelMauderer commented Feb 3, 2023 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Feb 4, 2023

MichaelMauderer Feb 6, 2023

farmaazon Feb 6, 2023

farmaazon Feb 6, 2023

radeusgd Feb 6, 2023

jdunkerley Feb 6, 2023

jdunkerley Feb 6, 2023

MichaelMauderer Feb 7, 2023

jdunkerley Feb 7, 2023 •

edited

Loading

radeusgd Feb 7, 2023

jdunkerley Feb 7, 2023

MichaelMauderer Feb 7, 2023

MichaelMauderer Feb 7, 2023

MichaelMauderer Feb 7, 2023

jdunkerley left a comment

		let result = next_on_list.or_else(\|\| vis_list.first()).cloned();
		result

		static inputType = 'Any'
		static inputType = 'Standard.Table.Data.Table.Table'

Ensure Table Visualisation is the Default Visualization for Tables #4120

Ensure Table Visualisation is the Default Visualization for Tables #4120

Conversation

MichaelMauderer commented Feb 3, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

MichaelMauderer commented Feb 3, 2023 •

edited

Loading

jdunkerley Feb 7, 2023 •

edited

Loading