panel_hidden conflicts with NetBSD and ncurses

[GitMensch]

while the portability list says "portable", it actually isn't.

This is PDC+PDCMs definition:
https://github.com/Bill-Gray/PDCursesMod/blob/a5f13c248c5cca3fd3ccc29d91385b0ee1161567/pdcurses/panel.c#L431C1-L438C2

with this doc:

panel_hidden() returns OK (0) if pan is hidden and ERR (-1) if it is not.

while ncurses says:

returns TRUE (1) if the panel is in the panel stack, FALSE (0) if it is not. If the panel is a null pointer, return ERR.

Note: while not documented, PDCM+PDC return ERR as expected - but specially for PDCM only if asserts are not enabled, which breaks further compatibility (this line should be dropped).

I'll create a PR for a fix, but I'm not sure if this should be applied as-is, because this will break programs written explicit for PDC/PDCM, using this function and checking for ERR explicit (but on the other hand, it may not be that often used and instead checked for non-zero directly - and even if it is checked for OK or non-zero that would be good).

It would be interesting to get input from @wmcbrine as well as this is a really old portability issue.

[Bill-Gray]

A very odd bug!

Whence came the ncurses docs you cite above? The ncurses documentation here (search for 'panel_hidden') says

panel_hidden(pan) returns FALSE if the panel pan is in the panel stack,
and TRUE if it is not.  If the panel is a null pointer, it returns ERR.

...which flips the meaning of the return value (for non-NULL pointers). That matches what I'd intuitively expect; panel_hidden() should return TRUE if the panel is hidden, and FALSE if it it's visible. ("Is the panel hidden?" is a true/false question, not an OK/error one.) Otherwise, you get

   if( panel_hidden( pan) == TRUE)
      /* panel is not actually hidden */

SVR4 documentation for the function is here, but is ambiguous on the point.

I've modified the test_pan.c demo (click here for the modified version) to show the return value of panel_hidden() on-screen; you can see how it changes as you toggle panels on/off. With ncurses, that return value is indeed 1 if the panel is hidden and 0 if it's visible. With PDCurses*, those values are 0 and -1, respectively.

As to the assert( pan) line : about four years ago, I added a lot of such assert()s for functions which can theoretically accept NULL pointers and return error conditions. My reasoning was that they are all functions for which the passing of a NULL parameter indicates a bug. I'd be willing to remove any of them, if a case can be made that there might actually be a case where a NULL pointer passed to them wouldn't actually be a bug.

However, I added them four years ago; if they've actually signalled an error falsely, nobody mentioned it to me. (Admittedly, it does happen that people don't report even blatant bugs, either because they're busy or assume I already must know about them. And the very bug that is the subject of this issue is an example of one that's gone unreported for a couple of decades. As you suggest, this is a rarely-used function.)

[GitMensch]

I was confused - the difference is only between OK = 1 and ERR = -1, not switched around

[Bill-Gray]

True confession time : I must have noticed this bug four years ago and simply coded around it. I've no memory of doing that... I probably saw it, thought "that's weird, I'll have to look into that further later", and then didn't.

It'd be good if any fix could be coordinated with @wmcbrine. Going by the SVR4 documentation for panel_hidden(), we definitely have got this wrong.

[GitMensch]

Thanks for the note - I've pushed a new version of the commit that drops this check in the panel demo (and adds to the changelog [also including the missing Unicode update, that started last January])

[GitMensch]

The possible reason is that ncurses had a long standing bug in its documentation which was fixed 20230819.

Older versions say

returns TRUE if the panel is in the panel stack, FALSE if it is not. If the panel is a null pointer, return ERR.

While the current one says:

panel_hidden(pan) returns FALSE if the panel pan is in the panel stack, and TRUE if it is not. If the panel is a null pointer, it returns ERR.

[GitMensch]

... rechecked:

The official doc https://invisible-island.net/ncurses/man/panel.3x.html#h3-panel_hidden has it correctly - because that is ncurses 6.5 (2024)

the release notes for 6.5 say that the doc was wrong. Checking a mirror of older versions shows that since ncurses 5.7 the code wasn't changed and this change as previous versions was not changing the return values; ncurses 5.0 from 26 years ago already returned TRUE if the panel is hidden...

but: ncurses 4 (27 years ago) indeed had the same bug we currently inspect here

[Bill-Gray]

There was a brief thread on the bug-ncurses in which the bug in documentation was pointed out; the docs were fixed immediately afterward. As both you and that thread note, the code behaved correctly. (Unless you go back 27 years.)

For this change, we would bump PDCursesMod's PDC_VER_MINOR up from 4 to 5, so that our PDC_BUILD would be raised from 4400 to 4500 [0], If William makes a similar fix to PDCurses, and increments PDC_BUILD for that library from 3907 to 3908, then the code to check for "correct" implementation of panel_hidden() would look like

#if (defined( PDCURSES) && (PDC_BUILD < 3908)) || (defined( __PDCURSESMOD__) && (PDC_BUILD < 4500))
    /* code assuming our 'old' mis-implementation */
#else                   
   /* code that assumes the standard SVR4 version of panel_hidden() */
#endif

If William doesn't do anything, then that first line becomes

#if (defined( PDCURSES) && (!defined( __PDCURSESMOD__) || PDC_BUILD < 4500))

[0] This originally said 'from 4400 to 4401'. Corrected following @GitMensch`s reply below.

[GitMensch]

I'm confused, if we change PDC_VER_MINOR that way then PDC_BUILD will be 4500, no?
Would you like to release 4.4.1 without that change before (I don't mind if we directly go to 4.5.0, just asking)?

[GitMensch]

I've wrapped this up in a separate branch for now, please do a review (even if it may stay as-is for a bunch of days), allowing me to further adjust as needed.

[Bill-Gray]

Looks pretty good to me. Minor changes :

• In HISTORY.md, replace 'Current PDCursesMod - 2024 Dec 31' with 'PDCursesMod 4.5.0 - 2024 December 31'. (I agree this ought to be a PDC_VER_MINOR increment, not a PDC_VER_CHANGE increment, such that PDC_BUILD will become 4500, not 4401.)
• I sometimes use test_pan with PDCurses, usually when digging into if something is a "just me, or PDCurses* in general" issue. It's less likely that I'll be using the 4.5 test_pan.c with the 4.4 or earlier PDCursesMod library, but I suppose it could happen. So the test_pan.c patch should read

#if (defined( PDCURSES) && (!defined( __PDCURSESMOD__) || PDC_BUILD < 4500))
    if( panel_hidden( curr_top) == OK)    /* old,  non-standard behavior */
#else            
    if( panel_hidden( curr_top) == TRUE)   /* correct behavior according to SVR4 */
#endif

(same test as in my previous message, but with the correct PDCBUILD < 4500 check. Also, this version doesn't make assumptions about when/if/how @wmcbrine will patch this in upstream PDCurses. If he does fix it, we can revise this test, and also your warning about incompatibility in panel.c.

• Which makes me think : in that panel.c note about portability, I'd replace check for the PDCurses version number / date with check for the PDCursesMod build (PDC_BUILD < 4500). Or something of that ilk.