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.

Sign up for GitHub

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

Fix cut-off in text visualisations #6421

Merged
merged 7 commits into from
May 4, 2023
Merged

Fix cut-off in text visualisations #6421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7bd447c
Remove double padding
Procrat Apr 25, 2023
792ed93
Fix scrolling issue
Procrat Apr 25, 2023
9bdfc9e
Fix line height for text grid entries
Procrat Apr 25, 2023
e087e9c
Changelog
Procrat Apr 25, 2023
0b6fd09
Add comment
Procrat Apr 27, 2023
9452501
Merge branch 'develop' into wip/procrat/json-viz-last-row-6196
Procrat Apr 27, 2023
b63966e
Merge branch 'develop' into wip/procrat/json-viz-last-row-6196
Procrat May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@
sending messages to the backend][6341].
- [List Editor Widget][6470]. Now you can edit lists by clicking buttons on
nodes or by dragging the elements.
- [Fixed text visualisations which were being cut off at the last line.][6421]

[6421]: https://github.com/enso-org/enso/pull/6421

#### EnsoGL (rendering engine)

Expand Down
4 changes: 1 addition & 3 deletions app/gui/view/examples/text-grid-visualization/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,15 @@

use ensogl::prelude::*;

use crate::text_visualization::TextGrid;

use ensogl::animation;
use ensogl::application::Application;
use ensogl::display::navigation::navigator::Navigator;
use ensogl::system::web;
use ensogl::system::web::traits::DocumentOps;
use ensogl::system::web::traits::ElementOps;
use ensogl_text_msdf::run_once_initialized;
use ide_view::graph_editor::builtin::visualization::native::text_visualization;
use ide_view::graph_editor::builtin::visualization::native::text_visualization::text_provider;
use ide_view::graph_editor::builtin::visualization::native::text_visualization::TextGrid;



Expand Down
40 changes: 16 additions & 24 deletions app/gui/view/graph-editor/src/builtin/visualization/native/text_visualization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ const CHARS_PER_CHUNK: usize = 20;
/// loaded in each direction around the visible grid. So a value of 5 with a base grid of 20x10 will
/// load 25x15 grid.
const CACHE_PADDING: u32 = 15;
const PADDING_TEXT: f32 = 5.0;
const PADDING_TEXT: f32 = 10.0;



Expand Down Expand Up @@ -325,7 +325,7 @@ impl<T: TextProvider + 'static> TextGrid<T> {

frp::extend! { network

scroll_positition <- all(&scrollbar_h.thumb_position, &scrollbar_v.thumb_position);
scroll_position <- all(&scrollbar_h.thumb_position, &scrollbar_v.thumb_position);

longest_line_with_init <- all(&init, &text_provider.longest_line)._1();
lines_with_init <- all(&init, &text_provider.line_count)._1();
Expand All @@ -350,6 +350,12 @@ impl<T: TextProvider + 'static> TextGrid<T> {
text_grid_content_size_y <- text_grid.content_size.map(|size| size.y).on_change();
text_grid_content_size_y_previous <- text_grid_content_size_y.previous();

scrollbar_h.set_max <+ text_grid_content_size_x;
scrollbar_v.set_max <+ text_grid_content_size_y;

scrollbar_h.set_thumb_size <+ frp.set_size.map(|size| size.x - 2.0 * PADDING_TEXT);
scrollbar_v.set_thumb_size <+ frp.set_size.map(|size| size.y - 2.0 * PADDING_TEXT);

horizontal_scrollbar_change_args <- all(
text_grid_content_size_x,
text_grid_content_size_x_previous,
Expand All @@ -373,34 +379,20 @@ impl<T: TextProvider + 'static> TextGrid<T> {
thumb_position * content_size_y_previous / content_size_y
});

size_update <- all(&frp.set_size, &text_grid.content_size);
scrollbar_sizes <- size_update.map(|(vis_size, content_size)| {
vis_size.iter().zip(content_size.iter()).map(|(vis_size, content_size)| {
if *content_size > 0.0 {
(vis_size / content_size).clamp(0.0,1.0)
} else {
0.0
}
}).collect_tuple::<(f32,f32)>()
}).unwrap();
scrollbar_h.set_thumb_size <+ scrollbar_sizes._0();
scrollbar_v.set_thumb_size <+ scrollbar_sizes._1();

viewport <- all_with4(
viewport <- all_with3(
&init,
&scroll_positition,
&scroll_position,
&frp.set_size,
&text_grid.content_size,
f!([dom_entry_root](_, scroll_position, vis_size, content_size) {
f!([dom_entry_root] (_, scroll_position, vis_size) {
let (scroll_x, scroll_y) = *scroll_position;
let top = -scroll_y * content_size.y;
let top = -scroll_y;
let bottom = top - vis_size.y;
let left = scroll_x * content_size.x;
let right = left + vis_size.x;
let left = scroll_x;
let right = left + vis_size.x;

// Set DOM element size.
dom_entry_root.set_style_or_warn("top", format!("{}px", top + PADDING_TEXT));
dom_entry_root.set_style_or_warn("left", format!("{}px", -left + PADDING_TEXT));
dom_entry_root.set_style_or_warn("top", format!("{top}px"));
dom_entry_root.set_style_or_warn("left", format!("{}px", -left));

// Output viewport.
let viewport = grid_view::Viewport {top, bottom, left, right};
Expand Down
5 changes: 5 additions & 0 deletions .../view/graph-editor/src/builtin/visualization/native/text_visualization/grid_view_entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ impl Entry {
let mut style = "position: absolute; white-space: pre; pointer-events: auto;".to_string();
write!(style, "left: {left}px; top: {top}px;").ok();
write!(style, "width: {width}px; height: {height}px;").ok();
// The default line height in browsers is 1.2, which is great for multi-line text and
// elements whose height is greater than the line height. In this case, however, where the
// height and the font size are set to the same value, the default setting of 1.2 pushes
// the text below the allotted space.
write!(style, "line-height: 1").ok();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this? Looks magical. If we need it, it should have doc comment with explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained in the PR description, but I'll copy-paste it into a comment.

Copy link
Member

@wdanilo wdanilo Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if you are referring to this explanation:

The line height of text grid entries was set to the default of 1.2. That's the default line height in browsers, which is great for multi-line text and elements whose height is greater than the line height. In this case, however, where the height and the font size are set to the same value, the default setting of 1.2 pushes the text below the allotted space.

Then I do not understand it. I was reading it several times and it's not clear to me. default of 1.2 is great for multi-line text readability, like in our case. We should not change it to 1.0.

Anyway, you've written "In this case, however, where the height and the font size are set to the same value, the default setting of 1.2 pushes the text below the allotted space":

  • Height of what is the same as the font size?
  • What is "allotted space"?
  • Why these things are "pushed below" this space?

Can we make it working with default line-height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Height of what is the same as the font size?

The height of the element this part of the code is defining: the grid view entry.

  • What is "allotted space"?

The vertical space that is available for the text in this element which is fixed by the height which is explicitly set in the line above.

  • Why these things are "pushed below" this space?

Because with a font size of 12px and a line height of 1.2, there's not a enough room in an element that's only 12px high. It needs something like 1.2 * 12px ~= 14px.

Can we make it working with default line-height?

Another option might be to make the entry size higher: not set it to any value and let the browser derive it from the font size and line height. Looking at the code, however, it looks like we always let the entry size be set externally, by the component that uses the text grid. That might be substantial change because the grid view is used in a bunch of places. Any other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, can't we just change this line:
https://github.com/enso-org/enso/blob/develop/lib/rust/ensogl/app/theme/hardcoded/src/lib.rs#L590 ?

It sets the height of the grid entry as far as I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wdanilo: To make sure there's no confusion here, this PR doesn't change the distance between lines of text. The grid view entries are still fixed to the same height. This just changes where the text appears in relation to the grid view entry div.


self.text.set_attribute_or_warn("style", style);
}
Expand Down