Skip to content
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

gh-81163: Add support for extended color functions in ncurses 6.1 #17536

Merged
merged 30 commits into from
Aug 4, 2020
Merged

gh-81163: Add support for extended color functions in ncurses 6.1 #17536

merged 30 commits into from
Aug 4, 2020

Conversation

hpjansson
Copy link
Contributor

@hpjansson hpjansson commented Dec 9, 2019

This is a rebase of @websurfer5's #13534 against master, since that PR no longer applies cleanly and has not been updated since May. It now appears to pass the check that was failing in #13534 (review).

The original description from @websurfer5's PR follows.


ncurses 6.1 adds extended color functions to support terminals with 256 colors (e.g. xterm-256color). The extended functions use color-pair values that are signed integers because the existing functions only use signed short integer values that are too small to support the 65,536 color-pair values needed for a 256-color terminal.

The new extended color functions are used transparently by curses.color_content(), curses.init_color(), curses.init_pair(), and curses.pair_content() when available, and the original functions are used when they aren't. The module functions validate their parameters as signed integers when using the new extended functions and as signed short integers when using the original library functions to match the underlying ncurses function parameters. The original behavior remains unchanged when Python is built with a curses library that does not contain the new extended color functions.

A new function, curses.has_extended_color_support(), indicates whether extended color support is provided by the underlying ncurses library.

This PR also fixes issue #36630.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@hpjansson

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@hpjansson
Copy link
Contributor Author

I signed the CLA earlier today, hopefully it shows up in the system soon.

@dnastase
Copy link

dnastase commented May 4, 2020

Hi,

May I ask what is the status of this fix ?
This issue affects a lot of ncurses based projects.

Thanks.

Doc/whatsnew/3.8.rst Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
@hpjansson
Copy link
Contributor Author

Thanks for the review. I've addressed the issues brought up so far and a few more I found, and rebased it on master.

@ned-deily
Copy link
Member

ned-deily commented Jul 1, 2020

Alas, soon after the PR was updated, a large change across a number of modules relating to argument processing was merged by @serhiy-storchaka (578c395 for bpo-37999) that now causes merge conflicts for this PR. It would also be nice to merge this to 3.9 but there seem to be other merge conflicts there. Perhaps after fixing the merge conflicts here, a 3.9 backport PR could be prepared.

websurfer5 and others added 17 commits July 1, 2020 21:51
…t using the extended color functions in ncurses 6.1 when available.

color_content
color_number
init_color
init_pair
…ons to make them simpler and more maintainable
…ndicate whether extended colors are suported by the underlying ncurses library.
Use preconditions to ensure it's in range.
@serhiy-storchaka serhiy-storchaka self-requested a review July 3, 2020 05:34
This got lost during the rebase. Also change versionadded from 3.8
to 3.10.
@hpjansson
Copy link
Contributor Author

I have made the requested changes; please review again.

@websurfer5 Since this is your work originally, I gave you access to my fork -- feel free to change it as you like.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

Thanks for adding the doc change.

I built and ran test_curses on macOS with both the system ncurses 5.x and with a current ncurses 6.2 and verified that the current test_curses fails with 6.2 and that, with the PR applied, test_curses passes with both versions of ncurses and that has_extended_color_support() returns the expected values.

@ned-deily ned-deily merged commit da4e09f into python:master Aug 4, 2020
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 4, 2020
@dnastase
Copy link

dnastase commented Aug 4, 2020

Nice ! thank you all for your work and efforts !

@christianbundy
Copy link

Thanks for fixing this!

@ambv ambv added the needs backport to 3.9 only security fixes label Aug 10, 2020
@miss-islington
Copy link
Contributor

Thanks @hpjansson for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @hpjansson and @ned-deily, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker da4e09fff6b483fe858997da5599c25397107ca1 3.9

@ned-deily ned-deily removed the needs backport to 3.9 only security fixes label Aug 11, 2020
@ned-deily ned-deily removed their assignment Aug 11, 2020
@ned-deily
Copy link
Member

@ambv and I discussed backporting this for 3.9.0 but I had forgotten that, because of other changes already in 3.10, backporting this to 3.9 is not trivial so I can't recommend trying to throw it in just prior to 3.9.0rc1.

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020
@hpjansson hpjansson deleted the bpo36982 branch August 25, 2020 01:05
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@terryjreedy terryjreedy changed the title bpo-36982: Add support for extended color functions in ncurses 6.1 gh-81163: Add support for extended color functions in ncurses 6.1 May 18, 2024
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.