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

Panics when resizing #1408

Merged
merged 4 commits into from
Jan 16, 2022
Merged

Panics when resizing #1408

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d94720c
Change buffer.get & buffer.get_mut to return Option, Implement Trait …
Dec 29, 2021
116045b
Prevent FilePicker from drawing outside buffer (rust panics)
Dec 29, 2021
0841a88
apply suggestion
Jan 1, 2022
a2014f6
add function in_bounds to avoid useless calculations
Jan 1, 2022
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
6 changes: 2 additions & 4 deletions helix-term/src/ui/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ impl EditorView {
let x = area.right();
let border_style = theme.get("ui.window");
for y in area.top()..area.bottom() {
surface
.get_mut(x, y)
surface[(x, y)]
.set_symbol(tui::symbols::line::VERTICAL)
//.set_symbol(" ")
.set_style(border_style);
Expand Down Expand Up @@ -393,8 +392,7 @@ impl EditorView {
.add_modifier(Modifier::DIM)
});

surface
.get_mut(viewport.x + pos.col as u16, viewport.y + pos.row as u16)
surface[(viewport.x + pos.col as u16, viewport.y + pos.row as u16)]
.set_style(style);
}
}
Expand Down
2 changes: 1 addition & 1 deletion helix-term/src/ui/menu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ impl<T: Item + 'static> Component for Menu<T> {
let is_marked = i >= scroll_line && i < scroll_line + scroll_height;

if is_marked {
let cell = surface.get_mut(area.x + area.width - 2, area.y + i as u16);
let cell = &mut surface[(area.x + area.width - 2, area.y + i as u16)];
cell.set_symbol("▐ ");
// cell.set_style(selected);
// cell.set_style(if is_marked { selected } else { style });
Expand Down
12 changes: 6 additions & 6 deletions helix-term/src/ui/picker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ impl<T: 'static> Component for FilePicker<T> {
// |picker | | |
// | | | |
// +---------+ +---------+

let render_preview = area.width > MIN_SCREEN_WIDTH_FOR_PREVIEW;
let area = inner_rect(area);
// -- Render the frame:
Expand Down Expand Up @@ -492,10 +493,9 @@ impl<T: 'static> Component for Picker<T> {
let sep_style = Style::default().fg(Color::Rgb(90, 89, 119));
let borders = BorderType::line_symbols(BorderType::Plain);
for x in inner.left()..inner.right() {
surface
.get_mut(x, inner.y + 1)
.set_symbol(borders.horizontal)
.set_style(sep_style);
if let Some(cell) = surface.get_mut(x, inner.y + 1) {
cell.set_symbol(borders.horizontal).set_style(sep_style);
}
}

// -- Render the contents:
Expand All @@ -505,15 +505,15 @@ impl<T: 'static> Component for Picker<T> {
let selected = cx.editor.theme.get("ui.text.focus");

let rows = inner.height;
let offset = self.cursor / (rows as usize) * (rows as usize);
let offset = self.cursor / std::cmp::max(1, rows as usize) * (rows as usize);
k2d222 marked this conversation as resolved.
Show resolved Hide resolved

let files = self.matches.iter().skip(offset).map(|(index, _score)| {
(index, self.options.get(*index).unwrap()) // get_unchecked
});

for (i, (_index, option)) in files.take(rows as usize).enumerate() {
if i == (self.cursor - offset) {
surface.set_string(inner.x - 2, inner.y + i as u16, ">", selected);
surface.set_string(inner.x.saturating_sub(2), inner.y + i as u16, ">", selected);
}

surface.set_string_truncated(
Expand Down
2 changes: 1 addition & 1 deletion helix-term/src/ui/prompt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ impl Prompt {
.max(BASE_WIDTH);

let cols = std::cmp::max(1, area.width / max_len);
let col_width = (area.width - (cols)) / cols;
let col_width = (area.width.saturating_sub(cols)) / cols;

let height = ((self.completion.len() as u16 + cols - 1) / cols)
.min(10) // at most 10 rows (or less)
Expand Down
3 changes: 1 addition & 2 deletions helix-tui/src/backend/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ impl Backend for TestBackend {
I: Iterator<Item = (u16, u16, &'a Cell)>,
{
for (x, y, c) in content {
let cell = self.buffer.get_mut(x, y);
*cell = c.clone();
self.buffer[(x, y)] = c.clone();
}
Ok(())
}
Expand Down
63 changes: 48 additions & 15 deletions helix-tui/src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,17 @@ impl Default for Cell {
/// use helix_view::graphics::{Rect, Color, Style, Modifier};
///
/// let mut buf = Buffer::empty(Rect{x: 0, y: 0, width: 10, height: 5});
/// buf.get_mut(0, 2).set_symbol("x");
/// assert_eq!(buf.get(0, 2).symbol, "x");
/// buf[(0, 2)].set_symbol("x");
/// assert_eq!(buf[(0, 2)].symbol, "x");
/// buf.set_string(3, 0, "string", Style::default().fg(Color::Red).bg(Color::White));
/// assert_eq!(buf.get(5, 0), &Cell{
/// assert_eq!(buf[(5, 0)], Cell{
/// symbol: String::from("r"),
/// fg: Color::Red,
/// bg: Color::White,
/// modifier: Modifier::empty()
/// });
/// buf.get_mut(5, 0).set_char('x');
/// assert_eq!(buf.get(5, 0).symbol, "x");
/// buf[(5, 0)].set_char('x');
/// assert_eq!(buf[(5, 0)].symbol, "x");
/// ```
#[derive(Debug, Default, Clone, PartialEq)]
pub struct Buffer {
Expand Down Expand Up @@ -162,15 +162,13 @@ impl Buffer {
}

/// Returns a reference to Cell at the given coordinates
pub fn get(&self, x: u16, y: u16) -> &Cell {
let i = self.index_of(x, y);
&self.content[i]
pub fn get(&self, x: u16, y: u16) -> Option<&Cell> {
self.index_of_opt(x, y).map(|i| &self.content[i])
}

/// Returns a mutable reference to Cell at the given coordinates
pub fn get_mut(&mut self, x: u16, y: u16) -> &mut Cell {
let i = self.index_of(x, y);
&mut self.content[i]
pub fn get_mut(&mut self, x: u16, y: u16) -> Option<&mut Cell> {
self.index_of_opt(x, y).map(|i| &mut self.content[i])
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather see you use the original method and then use self.content.get(i). That way you're not doing bounds checks twice (once in index_of_opt, once internally in self.content[]).

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 don't think that would work. There is no way to avoid bounds checks on a Vec (to my knowledge) and the check performed by index_of is actually different:
Say the buffer is of size (2, 2), then asking for out-of-bounds cell at (2, 0) will compute index=2, which is in bounds of vec but correspond to incorrect cell at (0,1).

(We could accept that and tolerate that buffer can return bad cells when out of bounds, but I don't think optimizing performance for this kind of double checks is worth it)

Copy link
Member

Choose a reason for hiding this comment

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

Well there's unsafe get_unchecked() if you previously validate the index.

We inherited the Buffer implementation from https://github.com/fdehau/tui-rs/ and I think this explains why it panics and only uses a debug_assert! rather than Option: Buffer::get_mut is a hot path that gets called a lot on every render. On frequent re-renders the slowdown will be noticeable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to use get_unchecked or do you overall disagree with the proposed solution?

I think it's best to have safe functions in Buffer at the cost of (what I think to be) unnoticeable performance gains. I could be wrong, but I think the terminal emulator is more responsible of refresh latency than the editor.

Also, 'premature optimization is the root of all evil' :) Helix is in alpha state, it should be stable and feature-full before being optimized for speed / latency.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, I think the change is OK since now in most cases we use [] instead of get_mut anyway (like set_stringn).


/// Returns the index in the Vec<Cell> for the given global (x, y) coordinates.
Expand Down Expand Up @@ -205,6 +203,20 @@ impl Buffer {
((y - self.area.y) * self.area.width + (x - self.area.x)) as usize
}

/// Returns the index in the Vec<Cell> for the given global (x, y) coordinates,
/// or `None` if the coordinates are out of range.
fn index_of_opt(&self, x: u16, y: u16) -> Option<usize> {
if x >= self.area.left()
&& x < self.area.right()
&& y >= self.area.top()
&& y < self.area.bottom()
{
Some(self.index_of(x, y))
} else {
None
}
}

/// Returns the (global) coordinates of a cell given its index
///
/// Global coordinates are offset by the Buffer's area offset (`x`/`y`).
Expand Down Expand Up @@ -278,6 +290,11 @@ impl Buffer {
where
S: AsRef<str>,
{
// prevent panic if out of range
if self.index_of_opt(x, y).is_none() || width == 0 {
return (x, y);
}
k2d222 marked this conversation as resolved.
Show resolved Hide resolved

let mut index = self.index_of(x, y);
let mut x_offset = x as usize;
let width = if ellipsis { width - 1 } else { width };
Expand Down Expand Up @@ -372,15 +389,15 @@ impl Buffer {
pub fn set_background(&mut self, area: Rect, color: Color) {
for y in area.top()..area.bottom() {
for x in area.left()..area.right() {
self.get_mut(x, y).set_bg(color);
self[(x, y)].set_bg(color);
}
}
}

pub fn set_style(&mut self, area: Rect, style: Style) {
for y in area.top()..area.bottom() {
for x in area.left()..area.right() {
self.get_mut(x, y).set_style(style);
self[(x, y)].set_style(style);
}
}
}
Expand Down Expand Up @@ -408,7 +425,7 @@ impl Buffer {
pub fn clear(&mut self, area: Rect) {
for x in area.left()..area.right() {
for y in area.top()..area.bottom() {
self.get_mut(x, y).reset();
self[(x, y)].reset();
}
}
}
Expand All @@ -417,7 +434,7 @@ impl Buffer {
pub fn clear_with(&mut self, area: Rect, style: Style) {
for x in area.left()..area.right() {
for y in area.top()..area.bottom() {
let cell = self.get_mut(x, y);
let cell = &mut self[(x, y)];
cell.reset();
cell.set_style(style);
}
Expand Down Expand Up @@ -509,6 +526,22 @@ impl Buffer {
}
}

impl std::ops::Index<(u16, u16)> for Buffer {
type Output = Cell;

fn index(&self, (x, y): (u16, u16)) -> &Self::Output {
let i = self.index_of(x, y);
&self.content[i]
}
}

impl std::ops::IndexMut<(u16, u16)> for Buffer {
fn index_mut(&mut self, (x, y): (u16, u16)) -> &mut Self::Output {
let i = self.index_of(x, y);
&mut self.content[i]
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
16 changes: 8 additions & 8 deletions helix-tui/src/widgets/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,53 +140,53 @@ impl<'a> Widget for Block<'a> {
// Sides
if self.borders.intersects(Borders::LEFT) {
for y in area.top()..area.bottom() {
buf.get_mut(area.left(), y)
buf[(area.left(), y)]
.set_symbol(symbols.vertical)
.set_style(self.border_style);
}
}
if self.borders.intersects(Borders::TOP) {
for x in area.left()..area.right() {
buf.get_mut(x, area.top())
buf[(x, area.top())]
.set_symbol(symbols.horizontal)
.set_style(self.border_style);
}
}
if self.borders.intersects(Borders::RIGHT) {
let x = area.right() - 1;
for y in area.top()..area.bottom() {
buf.get_mut(x, y)
buf[(x, y)]
.set_symbol(symbols.vertical)
.set_style(self.border_style);
}
}
if self.borders.intersects(Borders::BOTTOM) {
let y = area.bottom() - 1;
for x in area.left()..area.right() {
buf.get_mut(x, y)
buf[(x, y)]
.set_symbol(symbols.horizontal)
.set_style(self.border_style);
}
}

// Corners
if self.borders.contains(Borders::RIGHT | Borders::BOTTOM) {
buf.get_mut(area.right() - 1, area.bottom() - 1)
buf[(area.right() - 1, area.bottom() - 1)]
.set_symbol(symbols.bottom_right)
.set_style(self.border_style);
}
if self.borders.contains(Borders::RIGHT | Borders::TOP) {
buf.get_mut(area.right() - 1, area.top())
buf[(area.right() - 1, area.top())]
.set_symbol(symbols.top_right)
.set_style(self.border_style);
}
if self.borders.contains(Borders::LEFT | Borders::BOTTOM) {
buf.get_mut(area.left(), area.bottom() - 1)
buf[(area.left(), area.bottom() - 1)]
.set_symbol(symbols.bottom_left)
.set_style(self.border_style);
}
if self.borders.contains(Borders::LEFT | Borders::TOP) {
buf.get_mut(area.left(), area.top())
buf[(area.left(), area.top())]
.set_symbol(symbols.top_left)
.set_style(self.border_style);
}
Expand Down
2 changes: 1 addition & 1 deletion helix-tui/src/widgets/paragraph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ impl<'a> Widget for Paragraph<'a> {
if y >= self.scroll.0 {
let mut x = get_line_offset(current_line_width, text_area.width, self.alignment);
for StyledGrapheme { symbol, style } in current_line {
buf.get_mut(text_area.left() + x, text_area.top() + y - self.scroll.0)
buf[(text_area.left() + x, text_area.top() + y - self.scroll.0)]
.set_symbol(if symbol.is_empty() {
// If the symbol is empty, the last char which rendered last time will
// leave on the line. It's a quick fix.
Expand Down
8 changes: 4 additions & 4 deletions helix-view/src/graphics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ impl FromStr for Modifier {
/// ];
/// let mut buffer = Buffer::empty(Rect::new(0, 0, 1, 1));
/// for style in &styles {
/// buffer.get_mut(0, 0).set_style(*style);
/// buffer[(0, 0)].set_style(*style);
/// }
/// assert_eq!(
/// Style {
Expand All @@ -332,7 +332,7 @@ impl FromStr for Modifier {
/// add_modifier: Modifier::BOLD,
/// sub_modifier: Modifier::empty(),
/// },
/// buffer.get(0, 0).style(),
/// buffer[(0, 0)].style(),
/// );
/// ```
///
Expand All @@ -348,7 +348,7 @@ impl FromStr for Modifier {
/// ];
/// let mut buffer = Buffer::empty(Rect::new(0, 0, 1, 1));
/// for style in &styles {
/// buffer.get_mut(0, 0).set_style(*style);
/// buffer[(0, 0)].set_style(*style);
/// }
/// assert_eq!(
/// Style {
Expand All @@ -357,7 +357,7 @@ impl FromStr for Modifier {
/// add_modifier: Modifier::empty(),
/// sub_modifier: Modifier::empty(),
/// },
/// buffer.get(0, 0).style(),
/// buffer[(0, 0)].style(),
/// );
/// ```
#[derive(Debug, Clone, Copy, PartialEq)]
Expand Down