Ncurses Extended Color Pairs incorrectly display as copies of lower 256 color pairs

[cmang]

Bug report

Bug description:

When using ncurses extended color pairs, introduced in Python 3.10 in PR #17536 and issue #81163, the color pairs past 256 don't seem to work. When accessed, pairs 256+ repeat the behavior of pairs 0-255 instead of the colors set by curses.init_pair().

In my use case, curses.has_extended_color_support() == True and curses.COLORS == 256.

I can use curses.init_pair() to initialize color pairs past 255. It lets me set them without any error. But when I go to display the colors, color pair 256 displays the same as color pair 0, 257 displays the same as 1, and so on.

I created a small test program to reproduce the issue by initializing, displaying, and letting you scroll through the extended color pairs, examining pairs 0-65536: https://gist.github.com/cmang/7d366f677cd067c00e58a9d9c97141c5

The test program initializes 256 * 256 (65536) color pairs, using every FG and BG combination for FG and BG colors 0-255. It then lets you use the left/right arrow keys to scroll through the pairs. It requires an xterm-256color capable terminal.

In the following screenshot, the test program shows each color pair number in its given color and you can see that the pairs repeat. The expected behavior is that pairs 256-512 should have a different background color set (red), as should the next 256 pairs (green), etc.

Edit: If you get an error running the test program, make your terminal larger.

@hpjansson @websurfer5

Thanks!

CPython versions tested on:

3.10, 3.11, 3.12

Operating systems tested on:

Linux, macOS

[hpjansson]

The addstr and addch functions take an attribute value, but attributes can only store a color pair in the range 0..255. COLOR_PAIR() applies a bitmask so it can be combined with other attributes. I think we need new API to allow users to pass in the color pair separately and use wattr_set() internally instead of the legacy wattrset(). Ref. attr(3NCURSES).

[EvanCarroll]

@hpjansson why do we have to maintain an older api with wattrset() if wattr_set() can do all the same things, and more. And, we never expose that to the user directly can't we just update the binding to use wattr_set() under the hood and fix this?

[grymmjack]

Would love to know the status of this!

[picnixz]

why do we have to maintain an older api with wattrset() if wattr_set() can do all the same things,

Because not everyone has access to the new API. So we need to maintain whatever is still being used in the wild.

I think we need new API to allow users to pass in the color pair separately and use wattr_set() internally instead of the legacy wattrset().

I can think of this. Or we can call this one when we pass a color attribute internally so that we don't have a new public API. I'll need to think about it.

[EvanCarroll]

Because not everyone has access to the new API. So we need to maintain whatever is still being used in the wild.

When you say "new API" here you mean a new enough version of ncurses? If so, can't we just provide different definitions for the same function -- one which uses the new ncurses API and one which uses the old one? That's pretty routine for C. Then if you have the new API, the advanced color functionality just works because wattr_set() is called under the hood.

[picnixz]

When you say "new API" here you mean a new enough version of ncurses?

Yes, according to the docs, if you want to have more than 256 color pairs, you need ncurses ABI 6 even if you use wattr_set. But maybe we could just make the detection of extended pairs better (namely, if the ABI is < 6 and wattr_set does not exist => we say we cannot support extended pairs).

[cmang]

maybe we could just make the detection of extended pairs better (namely, if the ABI is < 6 and wattr_set does not exist => we say we cannot support extended pairs).

I reckon that since the documentation already hides both ABI versions behind one Python interface, then this would be a nice way for reality to reflect the documentation, if possible.

To me this seems preferable over making a new interface on the Python side, as people are already programming based on the documentation. It's also less work for people using the interface, if a bit more work behind-the-scenes on the Cpython side.

This approach deviates somewhat from the C ncurses patterns to be more Pythonic, though. I think that is cool if it works, but bad if it doesn't. Right now it doesn't.

Dumb question, but will this have implications for other Python implementations? Are there any non-cpython issues to take into consideration?

Is there any way to ask Python which Ncurses ABI it is using? I kind of assumed that's what curses.has_extended_color_support() would do, by proxy. (At least, it should always be False when using the Version 5 ABI.)

[picnixz]

will this have implications for other Python implementations

This is an extension module, entirely optional, and I believe that PyPy and others just ships the ncurses module. The only thing we touch is the curses API calls which are independent of how Python is implemented. Unless other implementations decideed to reimplement the curses module (namely provide their own shared library), then there won't be any impact. If they do, there will be a difference between our and the other implementations but that's their job if they want to backport the new feature.

This approach deviates somewhat from the C ncurses patterns to be more Pythonic, though

Actually, that's what we already do internally. When we can, we use new curses ABI calls internally and when we can't then we don't. For instance, for addstr:

The biggest difference is that the Python interface makes things simpler by merging different C functions such as addstr(), mvaddstr(), and mvwaddstr() into a single addstr() method.

So we're already unifying multiple kind of calls like this. In addition, we don't expose specific wattrset and wattr_set, so we're already saying "whatever attribute you want, we'll handle it ourselves".

---

More generally, I don't think we wanted the user to make the distinction between whether the attribute is passed as a 8-bit integer or not. That's what we also expose a way to create color attributes. I therefore think it's kind of a bug on our side that.

[hpjansson]

@hpjansson why do we have to maintain an older api with wattrset() if wattr_set() can do all the same things, and more. And, we never expose that to the user directly can't we just update the binding to use wattr_set() under the hood and fix this?

I agree this would be the best solution, as long as it can be done without breaking existing code. I was also worried that it'd break the thin wrapper pattern of the ncurses module, but as @picnixz pointed out, we're already merging calls, so that's probably not an issue.