-
Notifications
You must be signed in to change notification settings - Fork 317
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
Implement Lazy Text Visualisation. #3910
Implement Lazy Text Visualisation. #3910
Conversation
## UNSTABLE | ||
ADVANCED | ||
|
||
Returns the data requested to render a lazy view of the default visualisation. | ||
to_lazy_visualization_data : Text | ||
to_lazy_visualization_data self text_window_position text_window_size chunk_size = | ||
text = self.to_default_visualization_data | ||
min_length_for_laziness = chunk_size * (text_window_size.at 0) * (text_window_size.at 1) | ||
if text.length <= min_length_for_laziness then text else | ||
get_lazy_visualisation_text_window text text_window_position text_window_size chunk_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we put these as an extension method in Standard.Visualisation.
The circular dependency this introduces into the library structure is not great. We will also move to_default_visualisation
there as well soon.
## UNSTABLE | |
ADVANCED | |
Returns the data requested to render a lazy view of the default visualisation. | |
to_lazy_visualization_data : Text | |
to_lazy_visualization_data self text_window_position text_window_size chunk_size = | |
text = self.to_default_visualization_data | |
min_length_for_laziness = chunk_size * (text_window_size.at 0) * (text_window_size.at 1) | |
if text.length <= min_length_for_laziness then text else | |
get_lazy_visualisation_text_window text text_window_position text_window_size chunk_size | |
## UNSTABLE | |
ADVANCED | |
Returns the data requested to render a lazy view of the default visualisation. | |
Any.to_lazy_visualization_data : Vector Integer -> Vector Integer -> Text | |
Any.to_lazy_visualization_data self text_window_position text_window_size chunk_size = | |
text = self.to_default_visualization_data | |
min_length_for_laziness = chunk_size * (text_window_size.first) * (text_window_size.second) | |
if text.length <= min_length_for_laziness then text else | |
get_lazy_visualisation_text_window text text_window_position text_window_size chunk_size |
By all means put in Main.enso
within Standard.Visualisation
for now. We can organise tidying up within library team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Moving this around.
@@ -7,6 +7,7 @@ import project.Meta | |||
from project.Data.Json import all | |||
from project.Data.Boolean import Boolean, True, False | |||
from project.Error.Common import Error, dataflow_error_handler | |||
from Standard.Visualization.Text import get_lazy_visualisation_text_window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces a circular dependency between Visualisation and Base.
@@ -4,6 +4,7 @@ import project.Meta | |||
|
|||
from project.Data.Boolean import Boolean, True, False | |||
from project.Error.Common import Error, Type_Error_Data | |||
from Standard.Visualization.Text import get_lazy_visualisation_text_window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
text = self.to_default_visualization_data | ||
min_length_for_laziness = chunk_size * (text_window_size.at 0) * (text_window_size.at 1) | ||
if text.length <= min_length_for_laziness then text else | ||
get_lazy_visualisation_text_window text text_window_position text_window_size chunk_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing new line at end of file.
## UNSTABLE | ||
ADVANCED | ||
Returns the data requested to render a lazy view of the text. | ||
to_lazy_visualization_data : Text | ||
to_lazy_visualization_data self text_window_position text_window_size chunk_size = | ||
get_lazy_visualisation_text_window self text_window_position text_window_size chunk_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## UNSTABLE | |
ADVANCED | |
Returns the data requested to render a lazy view of the text. | |
to_lazy_visualization_data : Text | |
to_lazy_visualization_data self text_window_position text_window_size chunk_size = | |
get_lazy_visualisation_text_window self text_window_position text_window_size chunk_size | |
## UNSTABLE | |
ADVANCED | |
Returns the data requested to render a lazy view of the text. | |
Text.to_lazy_visualization_data : Vector Integer -> Vector Integer -> Text | |
Text.to_lazy_visualization_data self text_window_position text_window_size chunk_size = | |
get_lazy_visualisation_text_window self text_window_position text_window_size chunk_size |
Returns the data requested to render a lazy view of the text. | ||
to_lazy_visualization_data : Text | ||
to_lazy_visualization_data self text_window_position text_window_size chunk_size = | ||
get_lazy_visualisation_text_window self text_window_position text_window_size chunk_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing new line at end of file.
|
||
spec = Test.group "By coordinates" <| | ||
Test.specify "Get correct chunks" <| | ||
(Preprocessor.lazy_preprocessor sample_text_multi_line [0,0] [1,1] 5).should_equal '{"chunks":"[[[0, 0], \\"ABCDE\\"]]","lines":2,"max_line_length":19}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could consider using Pair
instead of Vector
for the API: Pair.new 0 0
instead of [0, 0]
but as an internal function am happy with Vector or an Array if easier.
chunks = coordinates.map (ix -> format_chunk ix (get_text_chunk ix)) | ||
data_string = format_chunks chunks | ||
lines = text.lines.length | ||
active_lines = y_range.map (line_ix -> text.lines.at line_ix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
active_lines = y_range.map (line_ix -> text.lines.at line_ix) | |
active_lines = y_range.map text.lines.at |
lines = text.lines.length | ||
active_lines = y_range.map (line_ix -> text.lines.at line_ix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should cache text.lines
as recalculating will be costly.
lines = text.lines.length | |
active_lines = y_range.map (line_ix -> text.lines.at line_ix) | |
text_lines = text.lines | |
lines = text_lines.length | |
active_lines = y_range.map text_lines.at |
text = self.to_default_visualization_data | ||
min_length_for_laziness = chunk_size * (text_window_size.at 0) * (text_window_size.at 1) | ||
if text.length <= min_length_for_laziness then text else | ||
get_lazy_visualisation_text_window text text_window_position text_window_size chunk_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_lazy_visualisation_text_window text text_window_position text_window_size chunk_size | |
get_lazy_visualisation_text_window text text_window_position text_window_size chunk_size | |
Please add a newline at the end of the file.
ADVANCED | ||
|
||
Returns the data requested to render a lazy view of the default visualisation. | ||
to_lazy_visualization_data : Text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the types of the arguments? The signature suggest that this method takes no arguments and just returns a Text, but it does seem to take arguments.
to_lazy_visualization_data : Text | ||
to_lazy_visualization_data self text_window_position text_window_size chunk_size = | ||
get_lazy_visualisation_text_window self text_window_position text_window_size chunk_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this logic be guarded on text length in the same way as for Any
? Seems like computing the window is an overkill for very short texts which may appear relatively often.
|
||
from Standard.Base.Data.Text.Extensions import slice_text | ||
|
||
## Return a sub-window of a string. The window is defined by line/chunk coordinates. The siz of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Return a sub-window of a string. The window is defined by line/chunk coordinates. The siz of | |
## Return a sub-window of a string. The window is defined by line/chunk coordinates. The size of |
@@ -0,0 +1,203 @@ | |||
//! A cache that stores a grid of items. The cache contains a grid of items, and aa padding around |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! A cache that stores a grid of items. The cache contains a grid of items, and aa padding around | |
//! A cache that stores a grid of items. The cache contains a grid of items, and a padding around |
ABCDEFGHIJKLMNOPQRS | ||
1234567890 | ||
|
||
spec = Test.group "By coordinates" <| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec = Test.group "By coordinates" <| | |
spec = Test.group "Lazy Text Visualization" <| |
The current approach will show the following report in our test runs:
By coordinates:
- Get correct chunks
it is a bit hard to read what test that refers to, so I think it would be good to rename to make it clearer.
Also in specify
we usually start with 'should', but it is not a requirement.
Test.specify "Get correct chunks" <| | ||
(Preprocessor.lazy_preprocessor sample_text_multi_line [0,0] [1,1] 5).should_equal '{"chunks":"[[[0, 0], \\"ABCDE\\"]]","lines":2,"max_line_length":19}' | ||
(Preprocessor.lazy_preprocessor sample_text_multi_line [1,1] [1,1] 5).should_equal '{"chunks":"[[[1, 1], \\"67890\\"]]","lines":2,"max_line_length":10}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test where more than 1 chunk is returned?
(If that is even to be supported, but looking at the code it seems to be. I think then it should be tested)
make_grid_visualisation_response chunks lines max_line_length = | ||
(Json.from_pairs [["chunks", chunks], ["lines", lines], ["max_line_length", max_line_length]]).to_text | ||
|
||
## Return a chunk of text from a string. The chunk is defined by a its size and a line/chunk index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Return a chunk of text from a string. The chunk is defined by a its size and a line/chunk index | |
## Return a chunk of text from a string. The chunk is defined by its size and a line/chunk index |
coordinate. | ||
get_item_from text chunk_size index = | ||
line_ix = index.at 1 | ||
if line_ix > text.lines.length then "" else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we get out of bounds shouldn't we return some meaningful flag? Like a Nothing or a dataflow error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional because all the visualization will just want to know that there is nothing to display. We could try to omit the value for the requested cell, but then it will be harder to differentiate between "I have requested this and know that there is nothing to display" and "I do not know the value to display".
I guess we could also return null
(in JSON) and this would probably be the nicer design for potential optimizations on the IDE side. So I'm changing it to that.
@@ -94,7 +95,7 @@ fn init(app: &Application) { | |||
let camera = scene.camera(); | |||
let navigator = Navigator::new(scene, &camera); | |||
|
|||
let text_source = sample_text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor this constant to a variable with a descriptive name pls.
@@ -1,12 +1,19 @@ | |||
//! Example visualisation showing the provided data as text. | |||
//! Lazy text visualisation that can show text based data from the backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to name the visualization as "lazy_text_visualization". It is just a text visualization. The fact that it's lazy is its internal thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what is "text based data"? Can it show non-text based data, like tables, that can be printed to text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The visualization shows (at the stage of this PR) only text (or JSON data rendered as text). Conversion to text or JSON, happens on the engine side.
fn set_position_and_size(&self, pos: &Vector2, size: &Vector2) { | ||
self.text.set_position_xy(*pos); | ||
/// Number of characters that can be displayed in one grid cell. Also referred to as `chunk`. | ||
const CHARS_PER_CELL: f32 = 10.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coudl you expand this docs, please? For example, why this number was chosen? Can we choose smaller / bigger one? Also, what happens if you put non-jnteger number here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, and changed to usize
. The value was first used as f32
for size computations, but usize makes more sense.
const CHARS_PER_CELL: f32 = 10.0; | ||
/// Extra chunks to load around the visible grid to ensure smooth scrolling. Extra chunks are | ||
/// loaded in each direction around the visible grid. So a value of 5 with a base grid of 20x10 will | ||
/// load 25x15 grid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is a "chunk" = "line"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunk
is part of a line, but not a full line. I've added more explanation in the module description and the docs of the CHARS_PER_CHUNK
constant.
@@ -207,44 +133,34 @@ impl<T: TextProvider> Model<T> { | |||
fn set_size(&self, size: Vector2) { | |||
self.scroll_bar_horizontal.set_position_y(-size.y / 2.0); | |||
self.scroll_bar_horizontal.set_length(size.x); | |||
self.scroll_bar_horizontal | |||
.mod_position_y(|y| y + (scrollbar::WIDTH - scrollbar::PADDING) / 2.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, after merging develop, this needs to be changed to mod_y
.
Ok(LazyGridData { chunks, line_count, longest_line }) | ||
} else { | ||
let data_str = serde_json::to_string_pretty(&*content); | ||
let data_str = data_str.unwrap_or_else(|e| format!("<Cannot render data: {}>", e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing dot
.unwrap_or_default() | ||
.unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of improper data, we are getting defaults here, but we do not emit a warning, which can hide errors - am I right?
let chunks = content | ||
.lines() | ||
.enumerate() | ||
.flat_map(|(line_ix, line)| { | ||
line.chars() | ||
.chunks(CHARS_PER_CELL as usize) | ||
.into_iter() | ||
.enumerate() | ||
.map(move |(chunk_ix, chunk)| { | ||
let chunk = chunk.collect::<String>(); | ||
let pos = GridPosition::new(chunk_ix as i32, line_ix as i32); | ||
(pos, chunk) | ||
}) | ||
.collect_vec() | ||
}) | ||
.collect(); | ||
let line_count = content.lines().count() as u32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split it into a few lines with well named vars, please?
ADVANCED | ||
|
||
Returns the data requested to render a lazy view of the default visualisation. | ||
to_lazy_visualization_data : Text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should explicitly tell here that it is for "lazy" visualization. All of our visualizations should be lazy, so maybe it should be just named "to_visualization_data"?
|
||
## Format a chunk of text and meta information for the lazy visualisation. | ||
make_grid_visualisation_response chunks lines max_line_length = | ||
(Json.from_pairs [["chunks", chunks], ["lines", lines], ["max_line_length", max_line_length]]).to_text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these names, like "chunks" or "lines" are hardcoded here. Can we instead just serialize an Enso struct instead of hardcoding them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful <3
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466 # Conflicts: # CHANGELOG.md # app/gui/view/graph-editor/src/builtin/visualization/native/text_grid.rs
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466 # Conflicts: # CHANGELOG.md
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466 # Conflicts: # CHANGELOG.md # distribution/lib/Standard/Visualization/0.0.0-dev/src/Main.enso # distribution/lib/Standard/Visualization/0.0.0-dev/src/Preprocessor.enso
@jdunkerley @radeusgd Could you re-review? |
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466
let clipping_div = web::document.create_div_or_panic(); | ||
let clipping_div = DomSymbol::new(&clipping_div); | ||
let dom_entry_root = web::document.create_div_or_panic(); | ||
let size = default(); | ||
let text_provider = default(); | ||
|
||
clipping_div.set_style_or_warn("overflow", "hidden"); | ||
dom_entry_root.set_style_or_warn("position", "absolute"); | ||
scene.dom.layers.front.manage(&clipping_div); | ||
root.add_child(&clipping_div); | ||
clipping_div.append_or_warn(&dom_entry_root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super complex. Without proper explanation it is not understandable. Please refactor to a well documented function
on_data_update <- frp.send_data.constant(()); | ||
text_grid.request_model_for_visible_entries <+ any(on_data_update,init); | ||
|
||
// === Visualisation API Inputs === | ||
|
||
eval frp.set_size ((size) model.set_size(*size)); | ||
|
||
|
||
// === Text Grid API === | ||
|
||
requested_entry <- text_grid.model_for_entry_needed.map2(&text_grid.grid_size, | ||
f!([model]((row, col), _grid_size) { | ||
let text = model.get_string_for_cell(*row,*col); | ||
let model = grid_view_entry::Model{text}; | ||
(*row, *col, model) | ||
}) | ||
); | ||
text_grid.model_for_entry <+ requested_entry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really long FRP definition. Please refactor it to a few, well named and well-documented functions. See lib/rust/ensogl/component/text/src/component/text.rs
(function Text::init
for reference)
|
||
|
||
// ================================= | ||
// === Text Processing Utilities === |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This title is very generic. In fact, anything can be considered an "utility". What we are doing here is measuring the glyph size.
// This extends the lifetime of the closure which is what we want here. Otherwise, the closure | ||
// would be destroyed and the callback cannot be called. | ||
#[allow(clippy::forget_non_drop)] | ||
mem::forget(closure); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use mem:forget
if you do not need to. You can store the closure in network
instead (it has a function for storing such things). Even better would be to store it in the visualization struct itself, so it can be properly documented. Using mem::forget
should be the absolutely last resort.
// ================= | ||
// === GridCache === | ||
// ================= | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
let y = line as i32; | ||
let x = chunk_index as i32; | ||
let result = self.text_cache.borrow_mut().get_item(Vector2::new(x, y)); | ||
// self.register_access.emit(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented code
/// Crate a preprocessor configuration for the lazy text preprocessor. | ||
fn lazy_text_preprocessor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the documentation and the function name are mysterious for me. Could you describe it better, please? Also, the "laziness" should be explained here.
grid_position: GridPosition, | ||
grids_size: GridSize, | ||
) -> PreprocessorConfiguration { | ||
let p = PreprocessorConfiguration::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p
is not the best name for a variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. This was left over debug code.
} | ||
|
||
|
||
#[cfg(test)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing section
@@ -100,7 +100,7 @@ impl Registry { | |||
|
|||
/// Add default visualizations to the registry. | |||
pub fn add_default_visualizations(&self) { | |||
self.add(builtin::visualization::native::RawText::definition()); | |||
self.add(builtin::visualization::native::text_visualization::lazy_text_visualisation()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was writing about it in the previous PR review. All our visualisations should be "lazy", so mentioning "lazy" in any name doesnt make sense. This should be "text_visualization" instead. Please fix here and in other similar places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this one. Changed.
Cargo.toml
Outdated
@@ -32,7 +32,7 @@ default-members = ["app/gui", "lib/rust/*"] | |||
console_error_panic_hook = { git = 'https://github.com/enso-org/console_error_panic_hook' } | |||
|
|||
[profile.dev] | |||
opt-level = 0 | |||
opt-level = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional change?
// === Tests === | ||
// ============= | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
d6c5ae6
to
7f3c4e0
Compare
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466
…Column_Grid_View_Text_Visualization_with_the_IDE_and_Engine_#183453466
Pull Request Description
Implements #183453466.
Peek.2022-11-24.15-24.mp4
Important Notes
Text
type, which makes use of its internal representation to send dataChecklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide build
and./run ide watch
.