explore panics in debug mode

qfel commented 3 months ago

Describe the bug

explore panics when scrolled past the end

How to reproduce

run $env | explore
Keep using PageDown till it stops scrolling
Pres the down arrow

Expected behavior

Do nothing (already viewing the end of table)

Screenshots

 × Main thread panicked.
  ├─▶ at crates/nu-explore/src/views/record/mod.rs:697:5
  ╰─▶ 77 76
  help: set the `RUST_BACKTRACE=1` environment variable to display a backtrace.

This is on

fn get_percentage(value: usize, max: usize) -> usize {
    debug_assert!(value <= max, "{value:?} {max:?}");

Configuration

key	value
version	0.96.2
major	0
minor	96
patch	2
branch
commit_hash
build_os	linux-x86_64
build_target	x86_64-unknown-linux-gnu
rust_version	rustc 1.78.0 (9b00956e5 2024-04-29)
rust_channel	1.78.0-x86_64-unknown-linux-gnu
cargo_version	cargo 1.78.0 (54d8815d0 2024-03-26)
build_time	2024-08-03 20:14:38 -07:00
build_rust_channel	debug
allocator	mimalloc
features	default, sqlite, system-clipboard, trash
installed_plugins

Additional context

The problem seems to be WindowCursor manages both "view" and a "window" that sometimes seems to be assumed to fit within the view. But sometimes it isn't clamped on update and this seems to trigger the debug_assert.

I was looking at fixing, but realized the current design is a bit unhandy:

WindowCursor has view and window fields
- But the view field really defines the view size and window position
- While the window field defines the window size and cursor position within the window
- So if you want to move the window, you update the view field!
We need to maintain the invariant that the window always fits within the view (so that the cursor can't move past data)
- With the current design it's lossy - you can keep clamping the window due to operations on WindowCursor, but you lose the original size and so can't reliably "unclamp" it on reverse operations
- In practice it kind of works because the drawing code happens to reset window sizes - but it still feels unnatural it's needed, could lead to issues in the future and might already be a problem (if the reset happens too early/too late); not sure
- Alternatively, we could "merge" Cursor into WindowCursor so that we can preserve the window size information (while reporting the clamped value via the public API). With Cursor being a separate thing, many methods don't have enough information to do that.

Any thoughts about getting rid of Cursor as a separate entity so that we can preserve the original window size?

sholderbach commented 3 months ago

You seem to have narrowed it down quite a bit already, I think @rgwood last worked on simplifying the internal navigation mechanisms if you have questions.

fdncred commented 3 months ago

This was Reilly's cursor overhaul PR https://github.com/nushell/nushell/pull/12979 that may provide some insight.

rgwood commented 3 months ago

I was looking at fixing, but realized the current design is a bit unhandy:

Yeah it's got some problems! 😅

I tried to clean up the cursor code in the above PR to make it easier to follow, but it's still far from perfect and it's possible I introduced a bug.

Any thoughts about getting rid of Cursor as a separate entity so that we can preserve the original window size?

I'd be OK with this.

nushell / nushell