From f46381616272ba18a1a8a3f441b836c79bd1a029 Mon Sep 17 00:00:00 2001 From: Christian Duerr Date: Thu, 15 Mar 2018 20:13:00 +0100 Subject: [PATCH] Refactor `span_simple` selection The current `span_simple` selection is everything but simple. This version should have the same functionality as the current `span_simple` with the difference that a lot of complexity has been removed. Not only is this code shorter, it should also be significantly easier to understand with no "magic" to it. This will hopefully prevent us from having an unmaintainable blob of off-by-one guessing in the repo. Also removed the `out` file which I used in the original PR because scrollback is not implemented yet. :) --- out | 104 -------------------------------- src/selection.rs | 153 ++++++++++++----------------------------------- 2 files changed, 39 insertions(+), 218 deletions(-) delete mode 100644 out diff --git a/out b/out deleted file mode 100644 index 667994e3..00000000 --- a/out +++ /dev/null @@ -1,104 +0,0 @@ - Compiling alacritty v0.1.0 (file:///home/undeadleech/programming/rust/alacritty) -warning: unused variable: `count` - --> src/input.rs:69:26 - | -69 | fn scroll(&mut self, count: isize) {} - | ^^^^^ help: consider using `_count` instead - | - = note: #[warn(unused_variables)] on by default - -warning: unused variable: `previous_scroll_limit` - --> src/grid/mod.rs:217:13 - | -217 | let previous_scroll_limit = self.scroll_limit; - | ^^^^^^^^^^^^^^^^^^^^^ help: consider using `_previous_scroll_limit` instead - -warning: value assigned to `start` is never read - --> src/selection.rs:184:13 - | -184 | let mut start = initial_expansion.start; - | ^^^^^^^^^ - | - = note: #[warn(unused_assignments)] on by default - -warning: value assigned to `end` is never read - --> src/selection.rs:185:13 - | -185 | let mut end = initial_expansion.end; - | ^^^^^^^ - -warning: method is never used: `pop` - --> src/grid/storage.rs:50:5 - | -50 | pub fn pop(&mut self) -> Option { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: #[warn(dead_code)] on by default - -warning: method is never used: `swap` - --> src/grid/storage.rs:69:5 - | -69 | pub fn swap(&mut self, a: usize, b: usize) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - - Finished dev [unoptimized + debuginfo] target(s) in 9.82 secs - Running `target/debug/alacritty` -thread 'main' panicked at 'attempt to subtract with overflow', src/index.rs:414:17 -stack backtrace: - 0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace - at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49 - 1: std::sys_common::backtrace::_print - at libstd/sys_common/backtrace.rs:71 - 2: std::panicking::default_hook::{{closure}} - at libstd/sys_common/backtrace.rs:59 - at libstd/panicking.rs:207 - 3: std::panicking::default_hook - at libstd/panicking.rs:223 - 4: std::panicking::rust_panic_with_hook - at libstd/panicking.rs:402 - 5: std::panicking::begin_panic_fmt - at libstd/panicking.rs:349 - 6: rust_begin_unwind - at libstd/panicking.rs:325 - 7: core::panicking::panic_fmt - at libcore/panicking.rs:72 - 8: core::panicking::panic - at libcore/panicking.rs:51 - 9: >::sub_assign - at src/index.rs:414 - 10: alacritty::selection::Selection::span_simple - at src/selection.rs:283 - 11: alacritty::selection::Selection::to_span - at src/selection.rs:167 - 12: alacritty::term::Term::renderable_cells::{{closure}} - at src/term/mod.rs:1058 - 13: >::and_then - at /checkout/src/libcore/option.rs:612 - 14: alacritty::term::Term::renderable_cells - at src/term/mod.rs:1057 - 15: alacritty::display::Display::draw::{{closure}} - at src/display.rs:368 - 16: alacritty::renderer::QuadRenderer::with_api - at src/renderer/mod.rs:658 - 17: alacritty::display::Display::draw - at src/display.rs:360 - 18: alacritty::run - at src/main.rs:203 - 19: alacritty::main - at src/main.rs:62 - 20: std::rt::lang_start::{{closure}} - at /checkout/src/libstd/rt.rs:74 - 21: std::panicking::try::do_call - at libstd/rt.rs:59 - at libstd/panicking.rs:306 - 22: __rust_maybe_catch_panic - at libpanic_unwind/lib.rs:102 - 23: std::rt::lang_start_internal - at libstd/panicking.rs:285 - at libstd/panic.rs:361 - at libstd/rt.rs:58 - 24: std::rt::lang_start - at /checkout/src/libstd/rt.rs:74 - 25: main - 26: __libc_start_main - 27: _start diff --git a/src/selection.rs b/src/selection.rs index f775f1f6..20b1d601 100644 --- a/src/selection.rs +++ b/src/selection.rs @@ -245,127 +245,52 @@ impl Selection { } fn span_simple(grid: &G, region: &Range) -> Option { - let mut start = region.start.point; + let start = region.start.point; let start_side = region.start.side; - let mut end = region.end.point; + let end = region.end.point; let end_side = region.end.side; let cols = grid.dimensions().col; - // Handle some edge cases - if start.line > end.line { - if end.col > Column(0) { - start.col += 1; - end.col -= 1; - } - // Special case for when a multi-line selection to the - // bottom ends on a new line with just one cell selected - // and the first cell should not be selected - else { - if start_side == Side::Right { - start.col += 1; - } - - // Remove the single selected cell if mouse left window - if end_side == Side::Left { - end.line += 1; - end.col = cols - 1; - } - - return Some(Span { - cols, - front: end, - tail: start, - ty: SpanType::Inclusive, - }); - } - } else if start.line < end.line { - end.col += 1; - if start.col > Column(0) { - start.col -= 1; - } - // Special case for when a selection is started - // in the first cell of a line - else { - let ty = match end_side { - Side::Left => SpanType::ExcludeTail, - Side::Right => SpanType::Inclusive, - }; - - // Switch start cell to last cell of previous line - if start_side == Side::Left { - start.line += 1; - start.col = cols - 1; - } - - return Some(Span { - ty, - cols, - front: start, - tail: end, - }); - } - } - - let (front, tail, front_side, tail_side) = if start > end { - // Selected upward; start/end are swapped - (end, start, end_side, start_side) - } else { - // Selected downward; no swapping - (start, end, start_side, end_side) - }; - - debug_assert!(!(tail < front)); - - // Single-cell selections are a special case - if start == end { - if start_side == end_side { - return None; - } else { - return Some(Span { - cols, - ty: SpanType::Inclusive, - front, - tail, - }); - } - } - - // The other special case is two adjacent cells with no - // selection: [ B][E ] or [ E][B ] - let adjacent = tail.line == front.line && tail.col - front.col == Column(1); - if adjacent && front_side == Side::Right && tail_side == Side::Left { + // No selection for single cell with identical sides or two cell with right+left sides + if (start == end && start_side == end_side) + || (start_side == Side::Right && end_side == Side::Left && end.col == start.col + 1) + { return None; } - Some(match (front_side, tail_side) { - // [FX][XX][XT] - (Side::Left, Side::Right) => Span { - cols, - front, - tail, - ty: SpanType::Inclusive - }, - // [ F][XX][T ] - (Side::Right, Side::Left) => Span { - cols, - front, - tail, - ty: SpanType::Exclusive - }, - // [FX][XX][T ] - (Side::Left, Side::Left) => Span { - cols, - front, - tail, - ty: SpanType::ExcludeTail - }, - // [ F][XX][XT] - (Side::Right, Side::Right) => Span { - cols, - front, - tail, - ty: SpanType::ExcludeFront - }, + // Make sure front is always the "bottom" and tail is always the "top" + let (mut front, mut tail, front_side, tail_side) = + if start.line > end.line || start.line == end.line && start.col <= end.col { + // Selected upward; start/end are swapped + (end, start, end_side, start_side) + } else { + // Selected downward; no swapping + (start, end, start_side, end_side) + }; + + // Remove last cell if selection ends to the left of a cell + if front_side == Side::Left && start != end { + if front.col != Column(0) { + front.col -= 1; + } + // Special case when selection starts to left of first cell + else { + front.col = cols - 1; + front.line += 1; + } + } + + // Remove first cell if selection starts at the right of a cell + if tail_side == Side::Right && front != tail { + tail.col += 1; + } + + // Return the selection with all cells inclusive + Some(Span { + cols, + front, + tail, + ty: SpanType::Inclusive, }) } }