vhakulinen / gnvim

GUI for neovim, without any web bloat
MIT License
1.86k stars 68 forks source link

Fix panic when cell_at fails sometimes #102

Closed llwwns closed 5 years ago

llwwns commented 5 years ago

cell_at caused panic sometimes when I was typing Japanese words.

I don't know what is the real reason but it can be fixed by remove the unwrap and turn the return value of cell_at into optional.

smolck commented 5 years ago

What nvim version are you using? If you’re not using nightly, could you try using GNvim master with nightly and see if the panics still occur? If I recall correctly, panics at the leaf.text.chars().nth(at - 1).unwrap(); (which I assume is where the crash is happening, if I’m wrong please let me know the line where it actually crashes) were caused by a bug in Neovim that nightly fixed, see these comments: https://github.com/vhakulinen/gnvim/issues/81#issuecomment-513439741, https://github.com/vhakulinen/gnvim/issues/81#issuecomment-513931845

You could also try the fix outlined in that issue (again, assuming that’s the line where it crashed).

llwwns commented 5 years ago

I am using the latest master version gnvim build from source. Neovim is not nightly version.

Gnvim paniced when I was typing Japanese characters but not happen everytime.

I tried to println leaf.text and at, when the panic happen, leaf.text equals a single Japanese character ant at equels 2.

llwwns commented 5 years ago
▽t, 1
¬, 1
¬, 1
¬, 1
¬, 1
¬, 1
て, 2
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:347:21
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.29/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.29/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:47
   3: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:36
   4: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:200
   5: std::panicking::default_hook
             at src/libstd/panicking.rs:214
   6: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:477
   7: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:384
   8: rust_begin_unwind
             at src/libstd/panicking.rs:311
   9: core::panicking::panic_fmt
             at src/libcore/panicking.rs:85
  10: core::panicking::panic
             at src/libcore/panicking.rs:49
  11: gnvim::ui::grid::row::Rope::cell_at
  12: gnvim::ui::grid::grid::Grid::flush
  13: gnvim::ui::ui::handle_notify
  14: glib::main_context_channel::dispatch
  15: g_main_context_dispatch
  16: <unknown>
  17: g_main_context_iteration
  18: g_application_run
  19: <O as gio::application::ApplicationExtManual>::run
  20: gnvim::main
  21: std::rt::lang_start::{{closure}}
  22: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:49
  23: std::panicking::try::do_call
             at src/libstd/panicking.rs:296
  24: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:82
  25: std::panicking::try
             at src/libstd/panicking.rs:275
  26: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  27: std::rt::lang_start_internal
             at src/libstd/rt.rs:48
  28: main
  29: __libc_start_main
  30: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
smolck commented 5 years ago

Thanks for the info and backtrace (and PR)! There my be a bigger problem here (whether a Neovim issue or something else) that returning an Option type masks, but doesn’t necessarily fix. This type of character-related stuff is not my specialty though, so someone else probably has a better idea of the problem than I do.

CC: @vhakulinen @badosu

llwwns commented 5 years ago

It looks like this panic happened when cursor is at the second place of a double-width character.

cursor:
(
    0,
    5,
)
row:
Row {
    rope: Some(
        Node(
            Leaf(
                Leaf {
                    text: "  1 ",
                    hl_id: 63,
                    len: 4,
                    double_width: false,
                },
            ),
            Node(
                Leaf(
                    Leaf {
                        text: "て",
                        hl_id: 0,
                        len: 2,
                        double_width: true,
                    },
                ),
                Node(
                    Leaf(
                        Leaf {
                            text: "¬",
                            hl_id: 65,
                            len: 1,
                            double_width: false,
                        },
                    ),
                    Leaf(
                        Leaf {
                            text: "                                                                                                                                                         ",
                            hl_id: 0,
                            len: 153,
                            double_width: false,
                        },
                    ),
                ),
            ),
        ),
    ),
    len: 160,
}

Because a double_width leaf always has single character maybe it could be fix like this:

@@ -235,7 +235,12 @@ impl Rope {
     pub fn cell_at(&self, at: usize) -> Cell {
         match self {
             Rope::Leaf(leaf) => {
-                let c = leaf.text.chars().nth(at - 1).unwrap();
+                let c = leaf.text.chars().nth(
+                  if leaf.double_width() {
+                    0
+                  } else {
+                  at - 1
+                  }).unwrap();
                 Cell {
                     text: c.to_string(),
                     hl_id: leaf.hl_id,

https://github.com/vhakulinen/gnvim/pull/103

badosu commented 5 years ago

Perhaps it makes sense to reopen this until it's not reproducible on master?