wmcbrine / PDCurses

A curses library for environments that don't fit the termcap/terminfo model.
https://pdcurses.org/
1.04k stars 174 forks source link

Timing issue with keypad() #90

Closed gkriehn closed 1 year ago

gkriehn commented 4 years ago

There appears to be a timing issue when keypad(stdscr,TRUE); is used.

Without keypad(), the following code works just fine:

#define XCURSES

#include <curses.h>

int main(int argc, char **argv)
{
     Xinitscr(argc,argv);
     cbreak();
     nodelay(stdscr,FALSE);
     noecho();
     printw("Hello World\n");
     refresh();
     getch();
     endwin();

     return 0;
}

As expected, the program waits for a key press before exiting. However, when kepad() is added:

#define XCURSES

#include <curses.h>

int main(int argc, char **argv)
{
     Xinitscr(argc,argv);
     cbreak();
     keypad(stdscr,TRUE);
     nodelay(stdscr,FALSE);
     noecho();
     printw("Hello World\n");
     refresh();
     getch();
     endwin();

     return 0;
}

The program no longer waits to print to the screen or waits for a key press before the program ends. The window flickers on screen and the program immediately ends. However, if I add a loop to the code:

#define XCURSES

#include <curses.h>

int main(int argc, char **argv)
{
     int ch;

     Xinitscr(argc,argv);
     cbreak();
     keypad(stdscr,TRUE);
     nodelay(stdscr,FALSE);
     noecho();
     printw("Hello World\n");
     refresh();
     while(1)
     {
          ch = getch();
          if (ch == 'Q')
          {
               printw("Q Pressed, Press Any Key to Exit");
               refresh();
               break;
          }
     }
     getch();
     endwin();

     return 0;
}

Then the program works as expected. Is there some hidden timing issue that has popped up in recent versions of PDCurses? This behavior was not present in v3.4.

Bill-Gray commented 4 years ago

I had to add #include "curses.h" to your code and remove #define XCURSES (duplicate definition). I also had to change init argc to int argc. Here's a corrected and extended version. (Which runs Just Fine, both with William's fork and mine, and also works with your added lines to wait for Q removed.)

The most likely culprit would be different compilation flags. If you have a wide-char library and compile your code without PDC_WIDE or vice versa, the returned characters will be different and you can get this sort of problem. Note that with the new version, the library builds in wide-char mode by default, which it didn't do before.

I'd assume you're getting a false character input at startup. keypad(stdscr, TRUE) enables the return of certain function keys, and in this case, you're getting a spurious one you wouldn't normally receive. Thus the added lines in my code to say, "if the input character is not Q, show its value." I'll bet that when you run the code, it will show an input character right off the bat, even though you've not touched the keyboard. (Actually, it seems even more likely that it's a wide-char library linked to code compiled without -DPDC_WIDE.)

gkriehn commented 4 years ago

Yes, I left out some of the boiler plate portions of the code like #include (I'm using an include flag with the compiler, along with pointing to the library with -L), and init argc was a typo. I've fixed the original comment. #define XCURSES is needed because I'm launching an X11 window. The point though is I don't have to touch anything on the keyboard for the program to end pre-maturely.

I've tried re-compiling with and without -DPC_WIDE (./configure --disable-widec), and adjusting the compilation flags. I was able to see the character width change, but there was no change to the issue.

But, testing your tweaked code with the else{} pointed me to the problem. It is a spurious key press. When the window launches it appears to capture screen position. (546 Pressed). If I drag the window with the mouse, or re-size the window, the same event is repeatedly captured by getch().

So if I launch the window and call two getch() functions in a row, the problem goes away.

Not sure how to deal with this other than a flushinp() call after Xinitscr(argc,argv). Seems like a hack, but it does solve this issue.

gkriehn commented 4 years ago

resize_term() is effectively two functions: When called with nonzero values for nlines and ncols, it attempts to resize the screen to the given size. When called with (0, 0), it merely adjusts the internal structures to match the current size after the screen is resized by the user. On the currently supported platforms, SDL, Windows console, and X11 allow user resizing, while DOS, OS/2, SDL and Windows console allow programmatic resizing. If you want to support user resizing, you should check for getch() returning KEY_RESIZE, and/or call is_termresized() at appropriate times; if either condition occurs, call resize_term(0, 0). Then, with either user or programmatic resizing, you'll have to resize any windows you've created, as appropriate; resize_term() only handles stdscr and curscr.

I'm resizing the window (which is why argc and argv are needed). Guess it's an issue of RTFM -- but this issue only popped up after v3.4.

Thanks for your patience.

Bill-Gray commented 4 years ago

Well, that's interesting... and prompted me to further revise my test code. I don't get the spurious KEY_RESIZE events, until I start moving the window around on the screen (no resizing involved). Then I get a string of "Resized 80x24" messages as the window moves. You really shouldn't get resize messages when the window hasn't changed size. Fortunately, the relevant logic in x11/pdcscrn.c to say "if the new size is same as the old size, don't tell the user the screen has been resized" is quite straightforward (see commit dd098c00c5bd in my fork). The same change could be made to the wmcbrine fork. Perhaps the spurious KEY_RESIZE messages didn't happen in previous versions? The latest PDCurses version massively revised the X11 code, getting rid of a fork() and associated message-passing, memory-sharing, and general confusion. Could be a bug crept in amongst all of this otherwise most welcome improvement.

gkriehn commented 4 years ago

I can only assume the spurious KEY_RESIZE message didn't happen in previous versions.

If you remove the getch(); line in your test code just before while(1), then the else if( ch == KEY_RESIZE) triggers when the window first pops up, even if you don't move the window around.

This is, I assume, because I am passing -lines 48 -cols 96 into the program when it is initially launched, meaning the window is resized from the very beginning.

So the great code cleanup must have fixed(?) the initial KEY_RESIZE event.

wmcbrine commented 4 years ago

I already dealt with this post-3.9, in 43ca5474ad9e9b630db53f888984771f98abdb21 , and then 7eb7598115c6442db1653f98fff32ab48c923a3c .

gkriehn commented 4 years ago

Yeah, pulling the latest snapshot did the trick. Problem resolved.

Thanks.

Bill-Gray commented 4 years ago

Errmmm... well, partly resolved and dealt with. 43ca547 and 7eb7598 fix the initial "false resize". (I'm using the latest snapshot, and therefore never saw that particular problem.) It does not resolve the false resizes you get when moving the window around the screen. The problem is that we're responding to a ConfigureNotify event. As the Xlib documentation states, you get those for "actual changes to a window's state, such as size, position, border, and stacking order". So we have to ignore the non-resizing events, as per dd098c0. Doing so also removes the need for 43ca547 and 7eb7598.

gkriehn commented 4 years ago

True. The development choices here are obviously a bit tricky because terminals were never meant to be re-sized or moved when computer terminals were just that -- terminals -- not the windowed terminal emulators we have now.

Browsing through the PDCurses documentation, I'm not necessarily seeing support to detect when the X11 (or any other) terminal window has been "physically" moved. If position portions of the ConfigureNotify event are going to be outright ignored, it would be nice for the documentation to explicitly state that, or for additional functionality to be added in at some point.

The PDCurses implementation supports moving characters, panels, virtual windows, etc. It would be nice to at least support a detection event of when the terminal itself has also been moved, but agree that getch() is not the place for it. I'd argue that getch() is a poor choice to detect any of these events, but if they are interpreted as input characters, it becomes a grey area. Keyboard inputs are clearly character inputs. Mouse events and changing terminal window events are clearly inputs, but I'd argue are not necessarily character inputs, even though that's the implementation. So a different event detection system would seem more appropriate. But that's just me.

Bill-Gray commented 4 years ago

You've lost me here. Why would you want to know that a PDCurses window had been moved? Resized, yes, obviously. But I can't think of what one would do within a PDCurses window because of it having been moved. Agreed, it is a little strange that you get "mouse events" and "resize events" returned as keyboard events. It does make the coding a little easier, though, and (of course) we're kinda stuck with it now anyway, after roughly 30 years.

gkriehn commented 4 years ago

Since PDCurses has been slowing gaining better X11 and SDL support, multiple simultaneous terminal screens comes to mind. You would then need position information about each of the screen windows if they were to loosely interact with each other. I have 3 monitors, 2 of which are vertical, 1 of which is horizontal, so I just can't resize one terminal screen to fit all of my real estate. But if you could launch multiple terminal graphics screens, I could place a separate screen window on a different monitor, and knowing the position could eventually lead to placing them or interacting with them in a deterministic manner. It's an edge case, for sure though.

Bill-Gray commented 4 years ago

In theory, the X11, SDLn, and WinGUI ports could all support multiple terminal screens. (The others only can in the sense that you can open another terminal and run WinCon or VT in it; I don't see how it would work for DOS or OS/2 or DOSVGA.) You would use newterm() to initialize your new terminal, set_term() to select that terminal (i.e., say that subsequent curses commands should apply to that terminal), and then perhaps set_term() back to the initial terminal. Each set_term() would swap out all the global variables such as SP, 'Mouse_status`, etc.

You could do it, but (a) it would only work for X11, SDLn, and WinGUI; (b) it would be a lot of work for each platform. The first isn't a deal-breaker. You can't resize a DOS terminal, nor get 16 million colors on it; there are many features that are platform-dependent. (b) might be an obstacle, though.

(Note that I previously suggested a new function to accomplish all this. Dunno why I did that; the existing set_term() function has enabled this for æons.)

wmcbrine commented 1 year ago

5b5c3d349b9948cc37da39c985d65f395059e833