Implement Lazy Table Visualisation UI integration. #5806

MichaelMauderer · 2023-03-03T13:49:06Z

Pull Request Description

Adds a new table visualization that is lazily retrieving data from the engine. Closes #5002.

Peek.2023-03-03.14-08.mp4

Important Notes

Leaves the old table visualization in place, but renames it to "Table (legacy)".
Visual design will need to be matched to a reference design.

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.

kazcw · 2023-03-03T16:33:14Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

 const PADDING_TEXT: f32 = 5.0;

+const TEXT_PADDING: f32 = 1.5;


Could use docs--the difference between these is not obvious.

kazcw · 2023-03-03T16:34:59Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

+    /// Position of the cell in the table in columns and rows.
+    pub cell:  GridPosition,
+    /// Position of the chunk in the cell in chunk index and lines.
+    pub chunk: GridPosition,


Should this be a ChunkCoordinate?

kazcw · 2023-03-03T16:37:12Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

+    }
+}
+
+/// Position and size of a sub-grid within a grid.


Docs should distinguish this from GridWindow.

kazcw

Approved, with some comments.

As I understand it, we are calculating what text we need based on Unicode character counts, although the displayed size of the text depends on the number of resulting glyphs. Please check & document behavior when the number of glyphs is very different from the number of characters, particularly when the input contains many Unicode combining characters.

kazcw · 2023-03-03T16:41:08Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

+                color::Lcha::new(0.95, 0.0, 0.0, 1.0),
+            _ => color::Lcha::new(1.0, 0.0, 0.0, 0.0),


These could be constants.

They should probably be taken form the theme. Changed it to that.

kazcw · 2023-03-03T16:47:02Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

@@ -264,12 +386,14 @@ impl<T: TextProvider + 'static> TextGrid<T> {
            theme_update <- all(init, font_name, font_size);
            text_grid.set_entries_params <+  theme_update.map(
                f!([dom_entry_root]((_, font_name, font_size)) {
+                    let font_name = font_name.clone();
+                    let font_size = *font_size;
+                    let parent = Some(dom_entry_root.clone_ref());

                    dom_entry_root.set_style_or_warn("font-family", font_name.clone());


(Not new code, but) this can be passed by reference.

kazcw · 2023-03-03T16:49:12Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

+    visualization::Definition::new(
+        visualization::Signature::new(
+            path,
+            "Standard.Table.Data.Table.Table",


Named constant.

kazcw · 2023-03-03T16:52:50Z

...gui/view/graph-editor/src/builtin/visualization/native/text_visualization/grid_view_entry.rs

@@ -18,10 +19,50 @@ use std::fmt::Write;
 // === Model ===
 // =============

+


Extra line.

kazcw · 2023-03-03T16:53:35Z

...gui/view/graph-editor/src/builtin/visualization/native/text_visualization/grid_view_entry.rs

+    pub content:  Content,
+    pub bg_color: color::Rgba,
+}
+


Extra line.

kazcw · 2023-03-03T17:24:05Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+            + chunk.x as usize;
+        let chunk_y =
+            self.rows.iter().take(row).fold(0, |acc, x| acc + x.unwrap_or(0)) + chunk.y as usize;
+        // error!("Converting {table_position:?} to {chunk_x} {chunk_y}");


Commented code.

kazcw · 2023-03-03T17:47:37Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+
+    /// Return the table item at the given position in the grid. The given grid coordinates address
+    /// the grid in chunks/lines.
+    pub fn grid_view_position_to_table_item(


This function is called for every displayed cell, and it performs O(rows + cols) string-scans and allocations. What about caching the results of the iter_row_items_per_line and iter_column_items_per_chunk operations?

We don't usually have too many cells at once (probably a few dozen or a few hundreds at most). And keeping the cache correct could be a bit tricky as we can get updates to the table layout, so we need to carefully consider the cache invalidation there. Not a huge issue, but a bit of extra work for sure.

So, while this is a good comment, I think before implementing this, we should profile the actual impact of this.

kazcw · 2023-03-03T18:11:43Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+    fn try_from(data: serde_json::Value) -> Result<Self, Self::Error> {
+        Ok(serde_json::from_value(data.clone()).unwrap_or_else(|_| {
+            let data_str = serde_json::to_string_pretty(&data);
+            let data_str = data_str.unwrap_or_else(|e| format!("<Cannot render data: {e}.>"));
+            data_str.into()
+        }))
+    }


Why return Ok if the input can't be deserialized?

The whole implementation is actually no longer needed. Removed it.

kazcw · 2023-03-03T18:14:29Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

@@ -335,11 +991,61 @@ fn fill_emtpy_chunks(chunks: Vec<Chunk>) -> Vec<Chunk> {
        .collect()
 }

+// =========================


Missing newlines.

kazcw · 2023-03-03T18:14:38Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+            Err(visualization::DataError::BinaryNotSupported)
+        }
+    }
+}


Missing newlines after.

MichaelMauderer · 2023-03-06T13:21:26Z

Approved, with some comments.

As I understand it, we are calculating what text we need based on Unicode character counts, although the displayed size of the text depends on the number of resulting glyphs. Please check & document behavior when the number of glyphs is very different from the number of characters, particularly when the input contains many Unicode combining characters.

That is a pretty good point that has not been accounted for in the current design for both lazy table and lazy text.

What we need to consider are graphemes. We can assume that we use as fixed width font, so all graphemes should be the same size. But on both the backend and the frontend we need to work with chunks of graphemes, to not split something up between chunks.
The first thing to look in is, how Enso supports graphemes and then splits strings based on them. And then also update the GUI part to use the unicode-segementation crate to work with graphemes on the GUI side.

I think this is mostly a “bookkeeping” issue for chunking and retrieving the data. Rendering them should then not be any different.

jdunkerley

Just approving the slight Enso changes.

radeusgd · 2023-03-06T14:27:33Z

What we need to consider are graphemes. We can assume that we use as fixed width font, so all graphemes should be the same size. But on both the backend and the frontend we need to work with chunks of graphemes, to not split something up between chunks. The first thing to look in is, how Enso supports graphemes and then splits strings based on them.

On the library side, grapheme clusters are first-class citizens for Enso strings - i.e. by default Text.length, Text.at, Text.take (to take a substring) will all use grapheme-cluster-based indexing, i.e. the length of a single grapheme cluster that spans multiple codepoints but is a single grapheme will be 1 and indexing is also taking that into account. So on this side we should already be all good.

radeusgd · 2023-03-06T14:31:34Z

What we need to consider are graphemes. We can assume that we use as fixed width font, so all graphemes should be the same size. But on both the backend and the frontend we need to work with chunks of graphemes, to not split something up between chunks. The first thing to look in is, how Enso supports graphemes and then splits strings based on them.

On the library side, grapheme clusters are first-class citizens for Enso strings - i.e. by default Text.length, Text.at, Text.take (to take a substring) will all use grapheme-cluster-based indexing, i.e. the length of a single grapheme cluster that spans multiple codepoints but is a single grapheme will be 1 and indexing is also taking that into account. So on this side we should already be all good.

However, we need rendering to be consistent with grapheme cluster handling. An interesting and potentially problematic part are emojis with combining characters, for example on the web a facepalm with a color and gender modifier will be correctly rendered as a single grapheme:🤦🏼‍♂️.

At least on my machine it shows up as:

And Enso does treat it as having length == 1. But in my terminal it does show up as 3 graphemes actually:

farmaazon

Code looks good, however I miss important information in the docs here and there.

farmaazon · 2023-03-06T16:24:53Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

+use text_provider::BackendTextProvider;
+use text_provider::TextProvider;


I think some of those are imported with full crate::... path, and here are short paths. Or is text_provider also a crate?

farmaazon · 2023-03-06T16:29:29Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs

+
+/// Position of a chunk in a table. Addressed by table cell first, then by chunk index/line within
+/// the cell.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)]
+pub struct TablePosition {
+    /// Position of the cell in the table in columns and rows.
+    pub cell:  GridPosition,
+    /// Position of the chunk in the cell in chunk index and lines.
+    pub chunk: ChunkCoordinate,
+}


What is a table in this context? The module documentation mentions grid only. Is Table same as Grid?

I've updated the module docs. "Table" in this context refers to the data in the backend, which consists of strings organized in rows/columns.

farmaazon · 2023-03-07T08:16:47Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+/// Column Width in lines
+type RowHeight = usize;


Copied docs.

farmaazon · 2023-03-07T08:17:32Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq, Copy)]
+/// A table item and its position. Only used for working on the layout, not for individual cells
+/// or accessing content.


We usually put docs before derives.

farmaazon · 2023-03-07T08:24:32Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+    /// Iterate over the item in the table. Only returns the actual content items. Does not return
+    /// the dividers or the headings.
+    fn iter_content_columns(&self) -> impl Iterator<Item = TableItem> + '_ {


The documentation does not explain the method's name.

farmaazon · 2023-03-07T08:24:54Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+    /// Iterate over the item in the table. Only returns the actual content items. Does not return
+    /// the dividers or the headings.
+    fn iter_content_rows(&self) -> impl Iterator<Item = TableItem> + '_ {


farmaazon · 2023-03-07T08:29:33Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+    /// Convert the given cell / chunk position from the backend data into a chunk position for the
+    /// text cache.
+    pub fn backend_table_position_to_grid_content_position(


It might be a good idea to add information about time complexity - here and in next methods. We iterate over all columns on the left (if I understand the code correctly), and that might be substantial.

Or are those displayed columns only? If yes, then I miss that important information here or in the structure's docs.

farmaazon · 2023-03-07T08:34:31Z

app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization/text_provider.rs

+type RowIndex = usize;
+
+/// Specification of a table layout. Contains the column and row widths, as well as the column and
+/// row names. Provides methods for updating the specification and for accessing layout properties.
+/// We allow for a table specification to be partially specified, i.e., some of the information may
+/// be `None`. This is used to allow for lazy initialisation of the layout and incremental updates
+/// as the table is updated.
+#[derive(Clone, Debug, Deserialize, Serialize, Default)]
+pub struct TableSpecification {
+    /// The width of each column in characters.


Please make subsection for TableSpecification, and the subsection for define_endpoints_2 below.

farmaazon · 2023-03-07T08:41:07Z

...gui/view/graph-editor/src/builtin/visualization/native/text_visualization/grid_view_entry.rs

+impl Model {
+    /// Get the content of the model.
+    pub fn text(&self, width: usize) -> String {
+        match &self.content {
+            Content::Text { content, .. } => content.to_string(),
+            _ => " ".repeat(width),
+        }
+    }


What is width exactly? Why once we return string of given width, but in other case return content without adjusting width?

Removed the parameter. This was used earlier for some formatting but is not needed any more. The current use is obsolete and can return an empty string instead.

…ion_UI_Integration_5002 # Conflicts: # CHANGELOG.md # app/gui/view/graph-editor/Cargo.toml

wdanilo · 2023-03-15T03:16:25Z

What's the status of this PR? :)

farmaazon · 2023-03-15T08:26:27Z

Fixing bugs and then QA is still to be done AFAIK

…r/Lazy_Table_Visualisation_UI_Integration_5002 # Conflicts: # CHANGELOG.md

enso-bot · 2023-03-19T23:38:03Z

Michael Mauderer reports a new STANDUP for the provided date (2023-03-17):

Progress: Worked on cleaning up code for the Lazy Table PR after fixing a regression affecting the Lazy Text visualization and doing additional testing. It should be finished by 2023-03-20.

Next Day: Next day I will be working on the #5931 task. Start on the "A drop-down that allows changing the execution mode" task.

…r/Lazy_Table_Visualisation_UI_Integration_5002 # Conflicts: # CHANGELOG.md

enso-bot · 2023-03-20T16:20:57Z

Michael Mauderer reports a new STANDUP for today (2023-03-20):

Progress: Finished the bug fixes and cleanup on the Lazy Table PR. Will now go James for testing. Starting on "A drop-down that allows changing the execution mode". It should be finished by 2023-03-20.

Next Day: Next day I will be working on the #5931 task. Continue work on setting up all the entities for a demo scene with the new dropdown menu.

vitvakatu · 2023-04-04T14:11:59Z

QA Report 🔴

Unfortunately, I found some issues.

When opening lazy vis, it does not display most of the entries

It only displays a field of a single entry, and you need to scroll for a while to allow the rest of the entries to load.

This does not happen when you reload the project, though – so if the lazy vis was open, it displays a full list after it's loaded. The way to reproduce it is to switch between the lazy vis and the legacy table.

2023-04-04.17-56-51.mp4

Scrolling feels more laggy than legacy vis

It takes a while to load new entries when I scroll.

2023-04-04.17-54-48.mp4

For comparison, legacy vis works more smoothly:

2023-04-04.17-55-52.mp4

Scrolling with two fingers on the touchpad does not work

Only dragging the scrollbar works. I can't see any scrollbars in full-screen mode, so it's impossible to scroll fullscreen vis without a scroll wheel.

When opening the full-screen vis, part of the entries is not displayed

2023-04-04.18-00-43.mp4

In full-screen vis, nodes are scrolled when you scroll

Most definitely not the issue of this particular PR.

It requires clicking on the visualization before scrolling to trigger this behavior.

xvcgreg · 2023-06-26T13:27:18Z

@MichaelMauderer what's the status on this PR and what's the holdup?

MichaelMauderer · 2023-06-26T13:38:18Z

@MichaelMauderer what's the status on this PR and what's the holdup?

It's not currently scheduled to be worked on due to other tasks being prioritized. Once it gets picked up again, it will need to be updated to develop and QA feedback needs to be addressed.

hubertp · 2023-09-22T12:54:25Z

Obsolete now, closing.

Implement Lazy Table Visualisation UI integration.

1f1f6b3

MichaelMauderer self-assigned this Mar 3, 2023

Changelog update.

b4489af

MichaelMauderer force-pushed the wip/MichaelMauderer/Lazy_Table_Visualisation_UI_Integration_5002 branch from 3588df0 to b4489af Compare March 3, 2023 15:57

Remove opt level change.

d0c28c7

MichaelMauderer marked this pull request as ready for review March 3, 2023 16:00

MichaelMauderer requested review from jdunkerley, radeusgd, wdanilo and farmaazon as code owners March 3, 2023 16:00

kazcw reviewed Mar 3, 2023

View reviewed changes

kazcw approved these changes Mar 3, 2023

View reviewed changes

MichaelMauderer added 2 commits March 6, 2023 12:49

Implement review comments.

a3a6920

Use theme for colors.

64a872a

jdunkerley approved these changes Mar 6, 2023

View reviewed changes

Remove out of sync tests.

0567222

farmaazon approved these changes Mar 7, 2023

View reviewed changes

wdanilo approved these changes Mar 8, 2023

View reviewed changes

MichaelMauderer added 5 commits March 9, 2023 12:51

Improved docs and minor review feedback.

1930a3e

Performance improvements.

746be30

Correctly compute with graphemes instead of chars.

13939de

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

3ede80f

…ion_UI_Integration_5002 # Conflicts: # CHANGELOG.md # app/gui/view/graph-editor/Cargo.toml

Fix Cargo.toml after merge.

acb3605

farmaazon mentioned this pull request Mar 14, 2023

Use AG Grid instead of the old table.js rendering #5915

Merged

5 tasks

Avoid out of bounds access.

bdfdca6

Merge remote-tracking branch 'origin/develop' into wip/MichaelMaudere…

e7fe4c2

…r/Lazy_Table_Visualisation_UI_Integration_5002 # Conflicts: # CHANGELOG.md

MichaelMauderer added 2 commits March 20, 2023 16:19

Bug fixes and cleanup.

5203fbf

Merge remote-tracking branch 'origin/develop' into wip/MichaelMaudere…

6af319e

…r/Lazy_Table_Visualisation_UI_Integration_5002 # Conflicts: # CHANGELOG.md

MichaelMauderer added 3 commits March 21, 2023 11:42

Bug fixes and cleanup.

11523eb

Lint.

d07746f

Fix heading chunks.

08bb3f5

xvcgreg closed this Jun 13, 2023

farmaazon reopened this Jun 13, 2023

farmaazon requested a review from GregoryTravis as a code owner June 13, 2023 11:41

farmaazon marked this pull request as draft June 13, 2023 11:42

GregoryTravis approved these changes Jun 13, 2023

View reviewed changes

hubertp closed this Sep 22, 2023

farmaazon deleted the wip/MichaelMauderer/Lazy_Table_Visualisation_UI_Integration_5002 branch May 9, 2024 07:40

		const PADDING_TEXT: f32 = 5.0;

		const TEXT_PADDING: f32 = 1.5;

		color::Lcha::new(0.95, 0.0, 0.0, 1.0),
		_ => color::Lcha::new(1.0, 0.0, 0.0, 0.0),

		@@ -18,10 +19,50 @@ use std::fmt::Write;
		// === Model ===
		// =============

		use text_provider::BackendTextProvider;
		use text_provider::TextProvider;

Implement Lazy Table Visualisation UI integration. #5806

Implement Lazy Table Visualisation UI integration. #5806

Conversation

MichaelMauderer commented Mar 3, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazcw 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

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

MichaelMauderer commented Mar 6, 2023

jdunkerley left a comment

Choose a reason for hiding this comment

radeusgd commented Mar 6, 2023

radeusgd commented Mar 6, 2023

farmaazon 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

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

wdanilo commented Mar 15, 2023

farmaazon commented Mar 15, 2023 • edited Loading

enso-bot bot commented Mar 19, 2023

enso-bot bot commented Mar 20, 2023

vitvakatu commented Apr 4, 2023

QA Report 🔴

When opening lazy vis, it does not display most of the entries

Scrolling feels more laggy than legacy vis

Scrolling with two fingers on the touchpad does not work

When opening the full-screen vis, part of the entries is not displayed

In full-screen vis, nodes are scrolled when you scroll

xvcgreg commented Jun 26, 2023

MichaelMauderer commented Jun 26, 2023

hubertp commented Sep 22, 2023

MichaelMauderer commented Mar 3, 2023 •

edited

Loading

farmaazon commented Mar 15, 2023 •

edited

Loading