-
Notifications
You must be signed in to change notification settings - Fork 245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make cursive
work with ncurses
v6.0.1
#778
make cursive
work with ncurses
v6.0.1
#778
Conversation
cursive
work with ncurses-rs
v6
This comment was marked as resolved.
This comment was marked as resolved.
8680a89
to
feeaa6c
Compare
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
Using a smarter "replace if" method that returns something like a Another option would be to only keep the string prefix up to that |
fc4b68d
to
61fc866
Compare
otherwise, nothing would get printed, silently. for print_at() and print_at_rep() only! why delete \0 instead of replace? read this: gyscos#778 (comment) this also fixes warnings about unused Result
61fc866
to
fa81cd2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
...before sending the &str to ncurses backend, this is done for cursive's print_at() and print_at_rep() only! otherwise, nothing would get printed, silently. Why delete \0 instead of replace with eg. space? this explains it best: gyscos#778 (comment) This also fixes warnings about unused Result. Closes: gyscos#780
fa81cd2
to
d00c1b0
Compare
...before sending the &str to ncurses backend, this is done for cursive's print_at() and print_at_rep() only! otherwise, nothing would get printed, silently. Why delete \0 instead of replace with eg. space? this explains it best: gyscos#778 (comment) This also fixes warnings about unused Result. Closes: gyscos#780
d00c1b0
to
487c63e
Compare
as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
cac901a
to
baedacb
Compare
Now this PR is clean(ish), we're waiting for one of these two
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 |
Do you mean the |
This comment was marked as resolved.
This comment was marked as resolved.
newterm https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/src/lib.rs#L1023-L1029 CString::new https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new bubble up this newterm error as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com> preserve original error in the panic report otherwise, we'd not know why ncurses-rs newterm errored directly include the variable name in the format! expression as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
580aa85
to
1a9bee7
Compare
newterm https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/src/lib.rs#L1023-L1029 CString::new https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new bubble up this newterm error as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com> preserve original error in the panic report otherwise, we'd not know why ncurses-rs newterm errored directly include the variable name in the format! expression as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
1a9bee7
to
29670e5
Compare
newterm https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/src/lib.rs#L1023-L1029 CString::new https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new bubble up this newterm error as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com> preserve original error in the panic report otherwise, we'd not know why ncurses-rs newterm errored directly include the variable name in the format! expression as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
29670e5
to
e68fd4f
Compare
newterm https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/src/lib.rs#L1023-L1029 CString::new https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new bubble up this newterm error as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com> preserve original error in the panic report otherwise, we'd not know why ncurses-rs newterm errored directly include the variable name in the format! expression as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
e68fd4f
to
d46a9a4
Compare
newterm https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/src/lib.rs#L1023-L1029 CString::new https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new bubble up this newterm error as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com> preserve original error in the panic report otherwise, we'd not know why ncurses-rs newterm errored directly include the variable name in the format! expression as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
d46a9a4
to
c48ab05
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Reopening because jeaye/ncurses-rs#220 got in, so this PR might work now, bare with me while I re-test everything in OP... |
cursive
work with ncurses-rs
v6cursive
work with ncurses
v6.0.1
which means that ncurses-rs already has the needed changes in this PR: jeaye/ncurses-rs#220 and/or from this PR: jeaye/ncurses-rs#218
newterm https://github.com/jeaye/ncurses-rs/blob/3aa22bc279e4929e3ab69d49f75a18eda3e431e9/src/lib.rs#L1023-L1029 CString::new https://doc.rust-lang.org/std/ffi/struct.CString.html#method.new bubble up this newterm error as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com> preserve original error in the panic report otherwise, we'd not know why ncurses-rs newterm errored directly include the variable name in the format! expression as suggested here: gyscos#778 (comment) Co-authored-by: Alexandre Bury <alexandre.bury@gmail.com>
c48ab05
to
0bd373d
Compare
I don't have the virtual machines anymore, so I've only tested Gentoo... EDIT: I should note that for cursive to fully use the |
Good job here, and glad it paid off :) |
but first:ncurses-rs
needs the changes in this PR: jeaye/ncurses-rs#220 (or ideally the ones in this PR jeaye/ncurses-rs#218 , depending on which PR gets merged I'll (re)test the below)ncurses
crate v 6.0.1 is published and enough.However
pancurses
doesn't need these changes looks like, forcursive
to work with it, so0.17
seems good (butcursive
's examples are not tested with0.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
cargo build --features="ncurses-backend"
cargo test --features="ncurses-backend"
cargo build --features="ncurses-backend" --all-targets
cargo test --features="ncurses-backend" --all-targets
... on each of these target environments:
Legend:
❌ = known not to work
= untested
= works
Gentoo 2.15 default/linux/amd64/23.0/split-usr/no-multilib (stable)
-won't build but it would've if Makes the build more robust and fixes compile errors and warnings jeaye/ncurses-rs#218 was merged.NixOS (if
PKG_CONFIG_PATH
is set to the dir containingncurses.pc
file, or usednix-shell
command incursive
repo dir)Fedora 39
Ubuntu Desktop 22.04.4 LTS (needs
libncurses-dev
)pkg-config
andpkgconf
installed.pkg-config
installed.FreeBSD 14.0-RELEASE
OpenBSD 7.5 GENERIC
LC_CTYPE=en_US.UTF-8
orLANG=en_US.UTF-8
to be set otherwise it looks as if it doesn't have wide chars support socursive
andpancuses
examples look pretty broken.MacOS Mojave v10.14.6 (with this)
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
TODO:
cargo test
) withpancurses
(dep)0.17
but this means it usesv5.101.0
ofncurses
crate forpancurses
and v 6.0.1 forcursive
, seecargo tree --all-features | less
(search forncurses
)cargo test
) withpancurses
main
branch from github with this PR in.cursive
need version bump in this PR? ie.version = "0.21.0"
or shall this be left to be done by the repo owner ?!ncurses-rs
version inCargo.toml
after it gets published, ie. so it's not lower than that version, because then it would break compilation ofcursive
. Note thatpancurses
doesn't need a version bump (see above), although the examples I'm running are through the PR-edpancurses
only.pancurses
inCargo.toml
too, after it gets its PR for the v6ncurses-rs
usage.cargo test --example select_test -- --nocapture
in addition to the suggested--bin
which doesn't work.ncurses-rs
v6)warning: attribute should be applied to a foreign function or static
#[link_name="box"]
not a foreign function or static
, this is onncurses-rs
warning: unused
Resultthat must be used
, (for all 3 cases) happens due toCString::new
This function will return an error if the supplied bytes contain an internal 0 byte.
ncurses::newterm
.unwrap()
is safe here due to usingNone
as the first arg tonewterm
ncurses::addstr
\0
is already handled bycursive
(due to PR Adds a buffer before the backend #786 and followups) so it won't reach theaddstr
call,unless in the future a dep gets updated to include this unicode-rs/unicode-width@4efb180#diff-2ad10836ccce5ac2056d5679cc92449d9ff9094d4ff5c5803f65b5dd1d52ef19R224this is handled too now.cargo clippy
and fix:cargo clippy --all-targets
cargo clippy --all-targets --all-features
cargo test --features="ncurses-backend,ansi,toml"
cursive
commits.cargo run --features="ncurses-backend" --example NAME
:cargo run --features="ncurses-backend,ansi" --example ansi
cargo run --features="ncurses-backend,builder" --example builder
(but looks better withcrossterm-backend
which is now the default I noticed)crossterm-backend
has no flickering on bottom line which is great)select_test
viacargo test --features="ncurses-backend" --example select_test -- --nocapture