Skip to content

Conversation

dr-frmr
Copy link
Contributor

@dr-frmr dr-frmr commented Sep 6, 2024

Problem

The terminal was using string slicing to do all character-level manipulation, causing special characters to create panics due to inserts/removes in the middle of unicode graphemes.

Solution

Rewrite the terminal with the unicode_segmentation crate to properly handle character input.

Testing

1. Use terminal as usual (major refactor, so test old behaviors)
2. Use terminal with special characters!

Docs Update

N/A

Notes

N/A

@dr-frmr dr-frmr requested a review from nick1udwig September 6, 2024 16:41
@dr-frmr
Copy link
Contributor Author

dr-frmr commented Sep 6, 2024

@nick1udwig I have tested this informally, would you mind running through some informal tests of normal user behavior through this terminal on linux? Some behavior may be platform-specific

Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

  • When scrolling cursor near unicode characters (e.g. emojis 😊 ™️) behavior is not quite right: e.g. it takes two lefts/rights to get past either of those emojis
  • Ctrl+R search for an emoji crashes node when there is a hit:
    fake.dev *:thread 'main' panicked at 
    /rustc/791adf759cc065316f054961875052d5bc03e16c/library/alloc/src/string.rs:1704:9:
    assertion failed: self.is_char_boundary(idx)
    
  • When scrolling up to a previously-sent command containing an emoji (not necessarily the last unicode char: could be emoji in middle of command), cursor is messed up (not on an empty space after the command, instead on last character; new entries still go at end)

@dr-frmr dr-frmr requested a review from nick1udwig September 11, 2024 03:56
@dr-frmr
Copy link
Contributor Author

dr-frmr commented Sep 11, 2024

Fixing cursor handling of wider-than-one-column unicode graphemes such as emojis was harder than I thought.. but it's working now on my machine. @nick1udwig would you mind re-reviewing?

Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

Almost perfect. Still crashing with TM emoji Ctrl+R tho:

fake.dev > hi fake2.dev Soon™️
Tue 22:08 terminal:sys: response from fake2.dev: delivered
fake.dev *:thread 'main' panicked at kinode/src/terminal/utils.rs:245:43:
                                                                         byte index 23 is not a char boundary; it is inside '™' (bytes 21..24) of `hi fake2.dev Soon™️`
                                                                   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
                                             Cleaning up "/tmp/kinode-fake-node"...
Done cleaning up "/tmp/kinode-fake-node".

@dr-frmr
Copy link
Contributor Author

dr-frmr commented Sep 11, 2024

@nick1udwig lol turns out that emoji triggers a bit of an edge case. fixed, I think

@nick1udwig
Copy link
Member

nick1udwig commented Sep 11, 2024

Still seeing the Soon™️ Ctrl+R crash

nit:

fake.dev > hi fake2.dev hello 😊
Wed 09:11 terminal:sys: response from fake2.dev: delivered
fake.dev *hi fake2.dev hello 😊

Ctrl+Ring for 😊 at end of a line leads to a big whitespace gap at end of found line. Works fine if emoji is in middle of line, not end.

@nick1udwig
Copy link
Member

fake.dev > hi fake2.dev Soon™️
Thu 11:31 terminal:sys: response from fake2.dev: delivered
fake.dev *:thread 'main' panicked at kinode/src/terminal/utils.rs:252:43:
                                                                         byte index 23 is not a char boundary; it is inside '™' (bytes 21..24) of `hi fake2.dev Soon™️`
                                                                   stack backtrace:
                                                                                      0: rust_begin_unwind
                    at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:652:5
         1: core::panicking::panic_fmt
                                                   at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/panicking.rs:72:14
                                         2: core::str::slice_error_fail_rt
                                                                             3: core::str::slice_error_fail
                     at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/str/mod.rs:89:5
        4: core::str::traits::<impl core::slice::index::SliceIndex<str> for core::ops::range::RangeTo<usize>>::index
                              at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/str/traits.rs:370:21
                      5: <alloc::string::String as core::ops::index::Index<I>>::index
                                                                                                  at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/alloc/src/string.rs:2463:9
                                                                                       6: kinode::terminal::utils::underline
                                      at ./kinode/src/terminal/utils.rs:252:43
                                                                                 7: kinode::terminal::State::search
                             at ./kinode/src/terminal/mod.rs:73:58
                                                                     8: kinode::terminal::handle_event::{{closure}}
                             at ./kinode/src/terminal/mod.rs:805:9
                                                                     9: kinode::terminal::terminal::{{closure}}
                         at ./kinode/src/terminal/mod.rs:285:117
                                                                  10: kinode::main::{{closure}}::{{closure}}
                      at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/macros/select.rs:557:49
                                11: <tokio::future::poll_fn::PollFn<F> as core::future::future::Future>::poll
                       at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/future/poll_fn.rs:58:9
                                12: kinode::main::{{closure}}
                                                                          at ./kinode/src/main.rs:411:28
       13: <core::pin::Pin<P> as core::future::future::Future>::poll
                                                                                 at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/future/future.rs:123:9
                                                                          14: tokio::runtime::park::CachedParkThread::block_on::{{closure}}
                                                     at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/park.rs:281:63
                                                              15: tokio::runtime::coop::with_budget
                                                                                                               at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/coop.rs:107:5
                    16: tokio::runtime::coop::budget
                                                                 at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/coop.rs:73:5
                                                                        17: tokio::runtime::park::CachedParkThread::block_on
                                      at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/park.rs:281:31
                                               18: tokio::runtime::context::blocking::BlockingRegionGuard::block_on
                             at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/context/blocking.rs:66:9
                                                19: tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}
                                         at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/scheduler/multi_thread/mod.rs:87:13
                                                                       20: tokio::runtime::context::runtime::enter_runtime
                                    at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/context/runtime.rs:65:16
                                                       21: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
                                   at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/scheduler/multi_thread/mod.rs:86:9
                                                                22: tokio::runtime::runtime::Runtime::block_on_inner
                              at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/runtime.rs:363:45
                                          23: tokio::runtime::runtime::Runtime::block_on
                                                                                                     at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/runtime.rs:333:13
              24: kinode::main
                                           at ./kinode/src/main.rs:458:5
                                                                          25: core::ops::function::FnOnce::call_once
                              at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/ops/function.rs:250:5
                    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
        Cleaning up "/tmp/kinode-fake-node"...
Done cleaning up "/tmp/kinode-fake-node".

@nick1udwig
Copy link
Member

fake.dev *:thread 'main' panicked at kinode/src/terminal/utils.rs:252:43:
                                                                         byte index 23 is not a char boundary; it is inside '™' (bytes 21..24) of `hi fake2.dev Soon™️`
                                                                   stack backtrace:
                                                                                      0:     0x5a966c9ebfc5 - std::backtrace_rs::backtrace::libunwind::trace::hd6ee197414bf99ea
                                                                                                           at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/../../backtrace/src/backtrace/libunwind.rs:105:5
                           1:     0x5a966c9ebfc5 - std::backtrace_rs::backtrace::trace_unsynchronized::h9fd55c2ad0dd7a49
                                                    at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
                                                                2:     0x5a966c9ebfc5 - std::sys_common::backtrace::_print_fmt::hbfd47ed60586204a
                                                                             at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/sys_common/backtrace.rs:68:5
                                                                            3:     0x5a966c9ebfc5 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hc54e3c03d028fdab
                                 at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/sys_common/backtrace.rs:44:22
                                 4:     0x5a966ca1d48b - core::fmt::rt::Argument::fmt::h63876f32ff1a0b39
                                    at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/fmt/rt.rs:165:63
                        5:     0x5a966ca1d48b - core::fmt::write::h30641d08eff9646d
                                                                                                                  at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/fmt/mod.rs:1169:21
     6:     0x5a966c9e733f - std::io::Write::write_fmt::ha08930cd6b63daf8
                                                                                                        at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/io/mod.rs:1835:15
                                                                                            7:     0x5a966c9ebd9e - std::sys_common::backtrace::_print::h19ad376a55928d7d
                                                                                                     at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/sys_common/backtrace.rs:47:5
                                                                                                    8:     0x5a966c9ebd9e - std::sys_common::backtrace::print::ha81cc7b81fad1382
                                                                                                            at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/sys_common/backtrace.rs:34:9
        9:     0x5a966c9ed2b9 - std::panicking::default_hook::{{closure}}::hf2a4bac5991bc13d
                                                                                              10:     0x5a966c9ecffd - std::panicking::default_hook::h2f6875ce27856b17
                                                                                                  at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:298:9
                                                                                      11:     0x5a966c9ed7c3 - std::panicking::rust_panic_with_hook::h00f5098df7893592
                                                                                                  at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:795:13
                                                                                       12:     0x5a966c9ed6a4 - std::panicking::begin_panic_handler::{{closure}}::hcf7a40b8788aaa6a
                                                                                                               at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:664:13
                                                                                                    13:     0x5a966c9ec489 - std::sys_common::backtrace::__rust_end_short_backtrace::h40fad163f3ed5f54
                                                                                                                                 at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/sys_common/backtrace.rs:171:18
                              14:     0x5a966c9ed3d7 - rust_begin_unwind
                                                                                                       at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:652:5
                                                                                           15:     0x5a966ca1a573 - core::panicking::panic_fmt::hdcca81ebc39c328c
                                                                                             at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/panicking.rs:72:14
                                                                                  16:     0x5a966ca2139f - core::str::slice_error_fail_rt::h7c732a3f2d8a60fb
                                                           17:     0x5a966ca20f8a - core::str::slice_error_fail::h2bea854a8f27bbae
                                                              at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/str/mod.rs:89:5
                                                18:     0x5a966c4b23be - core::str::traits::<impl core::slice::index::SliceIndex<str> for core::ops::range::RangeTo<usize>>::index::hf50661a495d37da8
                                                                                                                                 at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/str/traits.rs:370:21
                     19:     0x5a966c49aeeb - <alloc::string::String as core::ops::index::Index<I>>::index::hee59f3b7c88da9c7
                                                         at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/alloc/src/string.rs:2463:9
                                             20:     0x5a966921e05a - kinode::terminal::utils::underline::h0e8dbe26854a021c
                                                       at /home/nick/git/kinode/kinode/src/terminal/utils.rs:252:43
                  21:     0x5a9669124e93 - kinode::terminal::State::search::h0746e8c07e9965dc
                                                                                                                            at /home/nick/git/kinode/kinode/src/terminal/mod.rs:73:58
                                                                                    22:     0x5a9668bb6801 - kinode::terminal::handle_event::{{closure}}::hef6b956cdd5c0e68
                                                                                                       at /home/nick/git/kinode/kinode/src/terminal/mod.rs:805:9
                                                               23:     0x5a9668baf265 - kinode::terminal::terminal::{{closure}}::hf7208f4c7ac1377b
                                                                              at /home/nick/git/kinode/kinode/src/terminal/mod.rs:285:117
                                        24:     0x5a9669e73f88 - kinode::main::{{closure}}::{{closure}}::h1180219d3bd4c291
                                                      at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/macros/select.rs:557:49
                                                                25:     0x5a96696fb884 - <tokio::future::poll_fn::PollFn<F> as core::future::future::Future>::poll::hfe701d3f5f76081b
                                                                                                                 at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/future/poll_fn.rs:58:9
                       26:     0x5a9669e71eb9 - kinode::main::{{closure}}::h6d46fae9ca2b118d
                                                                                                                           at /home/nick/git/kinode/kinode/src/main.rs:411:28
                                                                            27:     0x5a966982eded - <core::pin::Pin<P> as core::future::future::Future>::poll::he1abf8634dc36ace
                                                                                                             at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/future/future.rs:123:9
   28:     0x5a96699ed1af - tokio::runtime::park::CachedParkThread::block_on::{{closure}}::h4baabb92500914be
                                        at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/park.rs:281:63
                                                 29:     0x5a96699ec7c2 - tokio::runtime::coop::with_budget::hbbe2450c743a725b
                                                          at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/coop.rs:107:5
                                                                  30:     0x5a96699ec7c2 - tokio::runtime::coop::budget::hb720ce444f0e35a3
                                                                      at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/coop.rs:73:5
                                                                             31:     0x5a96699ec7c2 - tokio::runtime::park::CachedParkThread::block_on::hbf3cd4519f286d7d
                                                                                                     at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/park.rs:281:31
           32:     0x5a96690f3a50 - tokio::runtime::context::blocking::BlockingRegionGuard::block_on::hba42d3ddf0e309dc
                                                   at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/context/blocking.rs:66:9
                                                                      33:     0x5a9669a706de - tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}::hd753a92f32a53a81
                                                                                                                         at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/scheduler/multi_thread/mod.rs:87:13
                                                    34:     0x5a9669d7d5d0 - tokio::runtime::context::runtime::enter_runtime::h19e2dca3e046b020
                                                                           at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/context/runtime.rs:65:16
                                                                                              35:     0x5a9669a70621 - tokio::runtime::scheduler::multi_thread::MultiThread::block_on::hb0b6c9cdc3f5d84d
                                 at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/scheduler/multi_thread/mod.rs:86:9
                                                              36:     0x5a96693e6c12 - tokio::runtime::runtime::Runtime::block_on_inner::h85dd1bdacb16a4e0
                                                                                      at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/runtime.rs:363:45
                                                                                                  37:     0x5a96693e6e54 - tokio::runtime::runtime::Runtime::block_on::h259390b4f7c5fc76
                                                                                                                    at /home/nick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/runtime.rs:333:13
                             38:     0x5a9669c06376 - kinode::main::h8e3b50afd9d71736
                                                                                                                    at /home/nick/git/kinode/kinode/src/main.rs:458:5
                                                                    39:     0x5a9668da335b - core::ops::function::FnOnce::call_once::h0c13d02e2352fee8
                                                                                  at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/ops/function.rs:250:5
                                                                          40:     0x5a9668d54dee - std::sys_common::backtrace::__rust_begin_short_backtrace::h10229b6364886de9
                                                                                                          at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/sys_common/backtrace.rs:155:18
       41:     0x5a966973e2f1 - std::rt::lang_start::{{closure}}::h812310f63e96ebec
                                                                                                                  at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/rt.rs:159:18
                                                                                                42:     0x5a966c9dd95d - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h892a44dda998865d
                                                        at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/core/src/ops/function.rs:284:13
                                                 43:     0x5a966c9dd95d - std::panicking::try::do_call::hc9014de36221acd3
                                                     at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:559:40
                                          44:     0x5a966c9dd95d - std::panicking::try::h453ccda36877974e
                                     at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:523:19
                          45:     0x5a966c9dd95d - std::panic::catch_unwind::hbaa92d92e060ac9d
                                                                                                                             at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panic.rs:149:14
           46:     0x5a966c9dd95d - std::rt::lang_start_internal::{{closure}}::hcd3273ff8ab79716
                                                                                                                               at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/rt.rs:141:48
          47:     0x5a966c9dd95d - std::panicking::try::do_call::h6403d9d92974ec13
                                                                                                                 at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:559:40
   48:     0x5a966c9dd95d - std::panicking::try::h01fff6009d51d8ea
                                                                                                 at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panicking.rs:523:19
                                                                                      49:     0x5a966c9dd95d - std::panic::catch_unwind::h1c26bc26d86edc32
                                                                                      at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/panic.rs:149:14
                                                                       50:     0x5a966c9dd95d - std::rt::lang_start_internal::h20d36132e6183eef
                                                                           at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/rt.rs:141:20
                                                         51:     0x5a966973e2ca - std::rt::lang_start::ha30528054dd48873
                                                    at /rustc/791adf759cc065316f054961875052d5bc03e16c/library/std/src/rt.rs:158:17
                                  52:     0x5a9669c0642e - main
                                                                 53:     0x7eff69229d90 - __libc_start_call_main
                                            at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
                                                                                                   54:     0x7eff69229e40 - __libc_start_main_impl
                                                                              at ./csu/../csu/libc-start.c:392:3
               55:     0x5a9668ad7865 - _start
                                                56:                0x0 - <unknown>
                                                                                  Cleaning up "/tmp/kinode-fake-node"...
Done cleaning up "/tmp/kinode-fake-node".
nick@pop-os:~/git/kinode (dr/terminal-special-chars $%=)$ git log
commit ab57a7e39fc6a34da9804d2f48d20f59fe068409 (HEAD -> dr/terminal-special-chars, origin/dr/terminal-special-chars)
Author: dr-frmr <docterformer@protonmail.com>
Date:   Thu Sep 12 14:56:44 2024 -0400

    remove conditional allowing for non-char-boundary inserts

Copy link
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

lol

@dr-frmr dr-frmr merged commit 44d47da into develop Sep 12, 2024
1 check passed
@dr-frmr dr-frmr deleted the dr/terminal-special-chars branch September 12, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants