make `cursive` work with `ncurses-rs` v6

[correabuscar]

but first: ncurses-rs needs the changes in this PR: https://github.com/jeaye/ncurses-rs/pull/218 (technically only the ones in the last 3 commits of April 11th seen here)

However pancurses doesn't need these changes looks like, for cursive to work with it, so 0.17 seems good (but cursive's examples are not tested with 0.17).

The following were used successfully:

• cargo build
• cargo test
• cargo build --all-features
• cargo test --all-features
• cargo build --all-targets
• cargo test --all-targets
• cargo build --all-targets --all-features
• cargo test --all-targets --all-features

... on each of these target environments:

• [x] Gentoo 2.15 default/linux/amd64/23.0/split-usr/no-multilib (stable)
• [x] NixOS (if PKG_CONFIG_PATH is set to the dir containing ncurses.pc file, or used nix-shell command in cursive repo dir)
• [x] Fedora 39
• [x] Ubuntu Desktop 22.04.4 LTS (needs libncurses-dev)
  • [x] tested to work without pkg-config and pkgconf installed.
  • [x] tested to work with pkg-config installed.
• [x] FreeBSD 14.0-RELEASE
  • [x] some examples tested
  • [ ] all examples tested
• [x] OpenBSD 7.5 GENERIC
  • [x] needs one of LC_CTYPE=en_US.UTF-8 or LANG=en_US.UTF-8 to be set otherwise it looks as if it doesn't have wide chars support so cursive and pancuses examples look pretty broken.
• [x] MacOS Mojave v10.14.6 (with this)
  • [x] some examples tested
  • [ ] all examples tested
• [x] Windows 11 WSL1 Arch Linux Linux DESKTOP 4.4.0-22621-Microsoft #2506-Microsoft Fri Jan 01 08:00:00 PST 2016 x86_64 GNU/Linux
  • [x] some examples tested
  • [ ] all examples tested

TODO:

• [x] compiles(and passes cargo test) with pancurses (dep) 0.17
• [x] compiles(and passes cargo test) with pancurses main branch from github with this PR in.
• [x] see if ncurses::newterm call was ignored without a let _ = before, in v5 too. If so, keep it untouched. It wasn't. No warnings with v5.
• [ ] does cursive need version bump in this PR? ie. version = "0.21.0" or shall this be left to be done by the repo maintainer(s) ?!
• [ ] use exact ncurses-rs version in Cargo.toml after it gets published, ie. so it's not lower than that version, because then it would break compilation of cursive. Note that pancurses doesn't need a version bump (see above), although the examples I'm running are through the PR-ed pancurses only.
• [ ] maybe bump pancurses in Cargo.toml too, after it gets its PR for the v6 ncurses-rs usage.
• [x] Actually doesn't matter://replace here(and below) is causing heap allocation in a context where the function arg was &str so it might not be desired. Find another way to show strings that have \0 in them? or what? err? Actually nevermind, because CString::new is used immediately after which does allocate on heap anyway.
• [x] add cargo test --example select_test -- --nocapture in addition to the suggested --bin which doesn't work.
• [x] fix all warnings (because they were introduced by this PR / the upgrade to ncurses-rs v6)
  • [x] warning: attribute should be applied to a foreign function or static #[link_name="box"] not a foreign function or static, this is on ncurses-rs
  • [x] warning: unused Result that must be used, (for all 3 cases) happens due to CString::new This function will return an error if the supplied bytes contain an internal 0 byte.
    • [x] ncurses::newterm .unwrap() is safe here due to using None as the first arg to newterm
    • [x] ncurses::mvaddstr
    • [x] ncurses::addstr
    • [x] decide what to do when \0 is encountered in the string: ignore, unwrap, rethrow(the ? aka try operator) then fix the warnings for ncurses::mvaddstr and ncurses::addstr above.
    • [x] Make test cases for
      • [x] print_at with string that contains \0 in the middle.
      • [x] print_at_rep with string that contains \0 in the middle.
      • [x] made one #[test] for both because otherwise they'd interfere with each other during parallel testing, well maybe not unless --nocapture is used. Well, let's just say I've had to run reset to restore my terminal.
• [ ] run cargo clippy and fix:
  • [x] errors (there were none)
  • [ ] warnings
    • [ ] pre this PR warnings
      • [ ] warning: this seems like a manual implementation of the non-exhaustive pattern pub enum MouseButton
• [x] passes cargo test --features="ncurses-backend,ansi,toml"
• [x] I'm watching(github notifications All Activity) the entire repo for any future-reported breakage, to address it.
• [x] test that examples compile and run seemingly ok(on Gentoo, NixOS and Fedora):
  • [x] advanced_user_data
  • [x] ansi
  • [x] autocomplete
  • [x] builder
  • [x] colors
  • [x] ctrl_c
  • [x] debug_console
  • [x] dialog
  • [x] edit
  • [x] fixed_layout
  • [x] focus
  • [x] hello_world
  • [x] key_codes
  • [x] linear
  • [x] list_view
  • [x] logs
  • [x] lorem
  • [x] markup
  • [x] menubar
  • [x] mines
  • [x] mutation
  • [x] panels
  • [x] parse
  • [x] pause
  • [x] position
  • [x] progress
  • [x] radio
  • [x] refcell_view
  • [x] scroll
  • [x] select
  • [x] select_test cargo test --example select_test -- --nocapture
  • [x] slider
  • [x] status
  • [x] status_bar_ext
  • [x] stopwatch
  • [x] tcp_server
  • [x] terminal_default
  • [x] text_area
  • [x] theme
  • [x] theme_editor
  • [x] theme_manual
  • [x] themed_view
  • [x] user_data
  • [x] vpv
  • [x] window_title

[correabuscar]

Question: how would you like to handle the case when that text &str there as the arg to print_at and print_at_rep, contains a \0 (null byte) as part of the string, which is when CString::new from ncurses-rs side will return early with an Err for each of those 3 function calls in the code section below: ncurses::mvaddstr, ncurses::addstr:

https://github.com/gyscos/cursive/blob/0a95c82c88c8a791d7fd983e7fb9f568b77551a8/cursive/src/backends/curses/n.rs#L396-L405

I guess the options are:

• [ ] ignore them as:
  • [ ] leave them as they are as warnings
  • [ ] fix the warnings so prefix with let _ =
• [ ] panic via calling .unwrap() on the functions
• [ ] change print_at* signatures to gracefully pass the error to caller
  • [ ] presumably this? -> Result<(), std::ffi::NulError>
  • [ ] not this? -> Result<(i32), std::ffi::NulError>
• [x] replace all \0 from the text &str, by creating the resulting String on heap like let text=&text.replace('\0'," ");
• [ ] something else ?

EDIT: I will do the selected one, as it seems somewhat of a good compromise, until you tell me to change it, or, preferably, you can change it after the PR is in, preferably :)

[gyscos]

Hi, and thanks a lot for the work! Sorry for the delay.

My main concern is, as you mentioned, the unconditional re-allocation. The fact that ncurses re-allocates as well is not an excuse, because:

• It's still an extra re-allocation, making something non-ideal even worse.
• We could (should?) one day switch to using addnstr which might not need to be zero-terminated (though the current ~rest~ rust wrapper still re-allocates it).
• ~Other backends might not re-allocate.~ (Nevermind, I somehow missed that this was all ncurses-backend-specific.)

Using a smarter "replace if" method that returns something like a Cow could be an option to avoid allocation in the general case, only paying the price when there's actually a \0.

Another option would be to only keep the string prefix up to that \0, ignoring the rest (this is what would have happened using the C library directly).

[correabuscar]

Seems like, there's no need to use .unwrap() in some places(in this PR) just to get rid of the unused warnings, especially since the results were ignored before also, because counting on .unwrap() to not panic depends on the implementation (of ncurses-rs) at the time, but if this changes in the future and it can return new errors for other reasons, then .unwrap() will panic.

I was too noob to realize that a let _ = would be enough.

Making the changes...done

[correabuscar]

I'm not sure how to disable those checks above(well they're below now) which seem to happen after every push, but the PR is not supposed to work/compile until this ncurses-rs PR gets in: https://github.com/jeaye/ncurses-rs/pull/218 (if ever), so it's a waste of resources to even try to 'check'

EDIT: ok this is good, I didn't notice it was this way before whoops:

 1 workflow awaiting approval
This workflow requires approval from a maintainer. Learn more about approving workflows. 

[correabuscar]

Now this PR is clean(ish), we're waiting for one of these two ncurses-rs PRs to get in:

• https://github.com/jeaye/ncurses-rs/pull/218 (if building properly on more than just fedora/ubuntu/nixos is desired)
• or just the bare minimum PR: https://github.com/jeaye/ncurses-rs/pull/220

Keeping the PR in draft until then. Maybe even have the optional pancurses one(PR) get in, even though cursive works without that one in.

PS: that CI failure is due to TERM not being set or something(that PR https://github.com/jeaye/ncurses-rs/pull/218 would actually tell us about it or any other(but not all) weird errors more precisely).

[gyscos]

PS: that CI failure is due to TERM not being set or something(that PR https://github.com/jeaye/ncurses-rs/pull/218 would actually tell us about it or any other(but not all) weird errors more precisely).

Do you mean the TERM variable is not set at compile time? I think that shouldn't be required - it's not uncommon to build without a terminal at all (from CI, or even from some IDEs), and lying about a TERM doesn't sound like a very clean option. Might be our only option at this point, which is a bummer.

[correabuscar]

Do you mean the TERM variable is not set at compile time? I think that shouldn't be required - it's not uncommon to build without a terminal at all (from CI, or even from some IDEs), and lying about a TERM doesn't sound like a better option.

Sounds reasonable to not need it, but it's true that because at build.rs time in ncurses-rs, some piece of code (that someone else made and I just carried over without giving it any thought, if I'm being honest here) will attempt to init ncurses to find whether some macros are defined. Well, it's right here: https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/src/genconstants.c#L9

most likely needed due to that comment

lying about a TERM doesn't sound like a very clean option

and btw, has to exist in terminfo database, which is why building via ssh while inside alacritty for example, on openbsd or freebsd(forgot which), will fail the build due to TERM=alacritty with the same CI failure, because alacritty isn't in terminfo database (on one of those, probably openbsd)

[correabuscar]

Closing this to allow someone else in the future to come along and do things better whenever things are ready in ncurses-rs, and also because I've already closed the prerequisite PR for ncurses-rs and without it this PR has no point:) Also, I may or may not be here in the future to provide support for these changes, as I had originally intended.

Thank you so much @ gyscos for the reviews and replies that helped me understand rust code better! Very much appreciated! All the best!

[correabuscar]

Reopening because https://github.com/jeaye/ncurses-rs/pull/220 got in, so this PR might work now, bare with me while I re-test everything in OP...

[correabuscar]

I don't have the virtual machines anymore, so I've only tested Gentoo... PR uses v6.0.1 of ncurses crate which was just published.

EDIT: I should note that for cursive to fully use the ncurses crate v6 instead of v5, cursive would have to use a pancurses version that uses v6 which isn't yet published (waiting for this PR to get in, but hopefully it would be bumped to v 0.18.0 and published then cursive can point to that version), else both v6 and v5 would be used with the ncurses-backend feature.

[gyscos]

Good job here, and glad it paid off :)