PDCursesMod icon indicating copy to clipboard operation
PDCursesMod copied to clipboard

Making SCREEN private

Open Bill-Gray opened this issue 3 years ago • 1 comments

(Note that this is food for thought/a possible future improvement. It applies to both PDCurses and PDCursesMod.)

curses.h says :

/* Avoid using the SCREEN struct directly -- use the corresponding
 functions if possible. This struct may eventually be made private. */

Which seems reasonable, and makes me wonder why it isn't private?

The only demo that accesses SCREEN elements is testcurs, where SP->visibility is accessed, but the current visibility can be determined in a generic, non-PDCurses-specific manner, avoiding direct access to SP elements.

A more serious obstacle occurred when I tried to make a getstr() replacement. There is no way, at least in PDCurses or ncurses, to get the current break and echo states without either accessing structure elements from SP (a PDCurses* method only) or just keeping track of your calls to cbreak() and echo().

If SCREEN became private, we'd need PDCurses-specific functions to access those structure elements (the current direct access of structure elements is already PDCurses-specific, so I don't see that as so terrible.) I've added such functions; see commit 5da7c96fe883e9cb4092.

Thus far, I don't see other instances where "privatizing" SCREEN would cause problems. If anyone knows of such instances, please let me know; I'd add further PDC_get_SCREEN element() functions as needed.

Still, actually making it private will have to wait for the next major version increase, since it (at least theoretically, and perhaps in some actual cases) breaks existing code.

Bill-Gray avatar Jun 18 '22 21:06 Bill-Gray

Commit 1d32280adaa7d266c13 implements this (in a new opaque_screen branch so as not to break existing code), and also makes SP internal. Commit 2259f29d5f0be70fae077 addressed the one place where SP was still being accessed.

Bill-Gray avatar Jul 13 '22 21:07 Bill-Gray

That leaves the question (maybe @wmcbrine?): Do we want to possibly break existing programs which access the screen directly now, during compile time by making it private instead of breaking them later, also at runtime if/when the "don't access directly" struct changes in the future?

GitMensch avatar Aug 26 '22 06:08 GitMensch

An interesting point. At present, we need not break anything. Commit 0d5e4dc5b45b5108f48e "re-purposed" an existing void pointer in the SCREEN structure to point to an opaque structure (defined/used internally to the PDCursesMod library). Because of its opacity, we can modify it without causing incompatibilities; outside code couldn't see what that pointer pointed to before, and it can't now.

As a result, I was able to move a couple of external variables into that opaque structure, ones that I'd wanted to move for some time but couldn't without breaking compatibility. Still more can be done in that direction.

So I think we can have it both ways : we can make changes inside the SCREEN structure (or at least the opaque part of it) without breaking things either at compile or run-time. I would still strongly urge not accessing SCREEN members directly, simply because any such code locks you in to PDCurses*. But you can do it.

Bill-Gray avatar Aug 26 '22 15:08 Bill-Gray

Agreed. With the referenced change we do not need to ever break anything, even if people access parts of the screen structure, which they should not. There is still broken run-time possible if someone overwrites the complete structure (for example with a backup), no?

GitMensch avatar Aug 26 '22 15:08 GitMensch

Hmmm... you mean if they did something like

const SCREEN old_scr = *SP;

(do various things,  then decide you want to recover the old screen)
*SP = old_scr;

? Yes, that would break, but it would do so anyway. SCREEN contains several pointers, so you may be restoring a pointer to memory that has moved or been freed. Anybody doing this already has crashes going on.

Incidentally, I see that ncurses makes the SCREEN structure opaque, and the WINDOW structure can be opaque.

Bill-Gray avatar Aug 26 '22 15:08 Bill-Gray

I've meant a copy of its content via memcpy sizeof(SCREEN), but the effect is what you've described.

GitMensch avatar Aug 26 '22 15:08 GitMensch

So "close as not planned"?

GitMensch avatar Aug 26 '22 15:08 GitMensch

I think "close as not planned until we do something that breaks the ABI anyway." If we do that, I'd take the opportunity to make the SCREEN, and possibly WINDOW, structs private.

Bill-Gray avatar Sep 12 '22 00:09 Bill-Gray