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.

Sign up for GitHub

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

Jump to bottom

bpo-37738: Fix curses addch(str, color_pair) #15071

Merged
merged 1 commit into from
Aug 14, 2019
Merged

bpo-37738: Fix curses addch(str, color_pair) #15071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2019-08-01-17-11-16.bpo-37738.A3WWcT.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix the implementation of curses ``addch(str, color_pair)``: pass the color
pair to ``setcchar()``, instead of always passing 0 as the color pair.
18 changes: 15 additions & 3 deletions Modules/_cursesmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,18 @@ static char *screen_encoding = NULL;

/* Utility Functions */

static inline int
color_pair_to_attr(short color_number)
{
return ((int)color_number << 8);
}

static inline short
attr_to_color_pair(int attr)
{
return (short)((attr & A_COLOR) >> 8);
}

/*
* Check the return code from a curses function and return None
* or raise an exception as appropriate. These are exported using the
Expand Down Expand Up @@ -606,7 +618,7 @@ _curses_window_addch_impl(PyCursesWindowObject *self, int group_left_1,
if (type == 2) {
funcname = "add_wch";
wstr[1] = L'\0';
setcchar(&wcval, wstr, attr, 0, NULL);
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if we are supposed to remove the color pair from attr passed to setcchar()?

Copy link

Choose a reason for hiding this comment

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

According to http://pubs.opengroup.org/onlinepubs/7908799/xcurses/setcchar.html, setcchar() takes an attr_t as the third argument.

According to http://pubs.opengroup.org/onlinepubs/7908799/xcurses/curses.h.html, attr_t is a bit set that can have these constants ORed:

WA_ALTCHARSET Alternate character set
WA_BLINK Blinking
WA_BOLD Extra bright or bold
WA_DIM Half bright
WA_HORIZONTAL Horizontal highlight
WA_INVIS Invisible
WA_LEFT Left highlight
WA_LOW Low highlight
WA_PROTECT Protected
WA_REVERSE Reverse video
WA_RIGHT Right highlight
WA_STANDOUT Best highlighting mode of the terminal
WA_TOP Top highlight
WA_UNDERLINE Underlining
WA_VERTICAL Vertical highlight

Meanwhile, waddch() takes a chtype, which contains a character, a color pair, and a bunch of attributes from this set:

A_ALTCHARSET Alternate character set
A_BLINK Blinking
A_BOLD Extra bright or bold
A_DIM Half bright
A_INVIS Invisible
A_PROTECT Protected
A_REVERSE Reverse video
A_STANDOUT Best highlighting mode of the terminal
A_UNDERLINE Underlining

There's no guarantee that, for example, A_BOLD == WA_BOLD. I'm afraid a more elaborate translation may be necessary here to be 100% correct.

Copy link

@mgedmin mgedmin Aug 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

curses.h on my system has

/*
 * X/Open attributes.  In the ncurses implementation, they are identical to the
 * A_ attributes.
 */
#define WA_ATTRIBUTES	A_ATTRIBUTES
#define WA_NORMAL	A_NORMAL
#define WA_STANDOUT	A_STANDOUT
#define WA_UNDERLINE	A_UNDERLINE
#define WA_REVERSE	A_REVERSE
#define WA_BLINK	A_BLINK
#define WA_DIM		A_DIM
#define WA_BOLD		A_BOLD
#define WA_ALTCHARSET	A_ALTCHARSET
#define WA_INVIS	A_INVIS
#define WA_PROTECT	A_PROTECT
#define WA_HORIZONTAL	A_HORIZONTAL
#define WA_LEFT		A_LEFT
#define WA_LOW		A_LOW
#define WA_RIGHT	A_RIGHT
#define WA_TOP		A_TOP
#define WA_VERTICAL	A_VERTICAL

#if 1
#define WA_ITALIC	A_ITALIC	/* ncurses extension */
#endif

so maybe we're fine? As long as we know we're using ncurses, and not some other curses library?

Copy link

Choose a reason for hiding this comment

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

I googled around to see whether there are curses implementations that use different bit values for these constants, and yes:

change ncurses-examples to use attr_t vs chtype to follow X/Open documentation more closely since Solaris xpg4-curses uses different values for WA_xxx vs A_xxx that rely on attr_t being an unsigned short. Tru64 aka OSF1, HPUX, AIX did as ncurses does, equating the two sets.

--- from https://www.gnu.org/software/ncurses/

setcchar(&wcval, wstr, attr, attr_to_color_pair(attr), NULL);
if (coordinates_group)
rtn = mvwadd_wch(self->win,y,x, &wcval);
else {
Expand Down Expand Up @@ -2621,7 +2633,7 @@ _curses_color_pair_impl(PyObject *module, short color_number)
PyCursesInitialised;
PyCursesInitialisedColor;

return PyLong_FromLong((long) (color_number << 8));
return PyLong_FromLong(color_pair_to_attr(color_number));
}

/*[clinic input]
Expand Down Expand Up @@ -3644,7 +3656,7 @@ _curses_pair_number_impl(PyObject *module, int attr)
PyCursesInitialised;
PyCursesInitialisedColor;

return PyLong_FromLong((long) ((attr & A_COLOR) >> 8));
return PyLong_FromLong(attr_to_color_pair(attr));
}

/*[clinic input]
Expand Down