Bill-Gray / PDCursesMod

Public Domain Curses - a curses library for environments that don't fit the termcap/terminfo model, modified and extended from the 'official' version
https://www.projectpluto.com/win32a.htm
340 stars 73 forks source link

ncurses extension define_key not implemented #214

Open adv-sw opened 3 years ago

adv-sw commented 3 years ago

Required to builld lldb curses implementation using this library.

Have stubbed it as follows to get something running.

#ifdef __PDCURSES__
// pdcurses doesn't implement define_key so we stub for the time being.
int define_key(const char *definition, int keycode)
{
   return keycode; // no idea if this is correct :)
}
#endif
GitMensch commented 3 years ago

That's definitely NOT correct.

A bit better would be to check for the define key function (has do be done at configure/cmake time), maybe a define for that already exists in lldb?

#ifndef HAVE_DEFINE_KEY
#define define_key(x,y) /* do nothing */
#endif

If not then you may still want to check if you can use that piece to define the function "away" for an easy compile (that can be even done unconditionally on the command line). If the return code is used: according to the man page define_key returns OK if everything is fine, ERR otherwise so you may want to always return the later in this case.

Note: It is unlikely that that change above gets in though, we had a discussion over 2 years ago on that matter:

keyok() and define_key() are written here as non-functional stubs, which I've been trying to remove from PDCurses where I can. Further, the things these functions are meant to do are again kind of meaningless outside of a termcap / terminfo environment.

Originally posted by @wmcbrine in https://github.com/wmcbrine/PDCurses/issues/59#issuecomment-490419007

[it is still good to have it here for reference]

Ideally you could patch the code in lldb that actually uses define_key - would that be possible?

Bill-Gray commented 3 years ago

Looking at the discussion @GitMensch links to, I see that William remarked that define_key() might be meaningfully defined for the VT port. He's right. That port contains a built-in list of key definitions for the Linux console and xterm-compatibles. If you encountered definitions that were missing from that table, you'd want to be able to use define_key() to add them. (It would also be relevant for the new Linux framebuffer port, which essentially "borrows" the VT code.)

In every other port, they are indeed meaningless. @adv-sw, if you're using the VT or framebuffer ports, and have a situation where key definitions are being added and need to actually be recognized ("this is the definition for Ctrl-Alt-Left Arrow and the key code I want getch() to return when that combination is hit"), I'll proceed from there (basically create an 'errata' table to which such entries will be added, extending the above-linked list in vt/pdckbd.c.)

If you're using it with any other port, or if you don't actually need the key in question, then @GitMensch's proposed solution (have it do nothing) is probably best. I'd have it return ERR, though, so that the application knows that nothing really happened.

adv-sw commented 3 years ago

yeah, return ERR seems most sensible for the time being. then app can know request failed. got lldb running with pdcursesmod. just - not as interactive as it should be yet but a start.

thanks for quick response.

could patch the app. thinking that's not necessarily the way as this is generic function (or stub)

adv-sw commented 3 years ago

Going with following for the time being :

// pdcurses doesn't implement define_key so we stub for the time being. int define_key(const char *definition, int keycode) { return ERR ; // not implemented. }

GitMensch commented 3 years ago

Is anyone against moving that issue to "add define_key() and keyok() for vt and framebuffer port"? We'd be then back to one of:

Bill-Gray commented 3 years ago

I don't actually expect to implement define_key() or keyok() soon. I might do it if somebody had an actual use case. Otherwise... not on the to-do list.

If we did do it, something resembling your third option would seem reasonable. I wouldn't want people to think these functions were actually doing something on ports where they are actually meaningless. (The overall concept that curses.h would be built dynamically tempts me; we might move the current curses.h to, say, curses0.h, and if you then built the code with WIDE=Y, DLL=Y, 32-bit chtypes, for the SDL1 platform, then we'd make a curses.h with the relevant #defines added. Users of the code and resulting library would not have to set any of those things; they'd be found within curses.h.)

GitMensch commented 3 years ago

I don't actually expect to implement define_key() or keyok() soon.

Then we may keep this issue open for PDCursesMod - but I suggest that you adjust the title of this issue or create a new issue to track this.

The overall concept that curses.h would be built dynamically tempts me [...], then we'd make a curses.h with the relevant #defines added. Users of the code and resulting library would not have to set any of those things; they'd be found within curses.h.

The point that this currently is not the case in PDCurses (it is since years with ncurses) is - for me personally - the biggest issue about building with PDCurses (I think XCurses with its configure hopefully doesn't have this issue). We also had a lot of misleading bug reports in GnuCOBOL because of this (and now ask the library if it actually matches what the compile said it does, which is good for library swapping after building in any case).

For my last ~rant about missing curses.h.in~ suggestion about how to nicely create curses.h see #133. You may want to use the "reference in new issue". Ideally we would find a way that @wmcbrine likes and could include too, I'm not sure if/where we had discussions about that.

adv-sw commented 3 years ago

Unsure of requirement as decided personally to bail to imgui based interface.

Status here : https://bugs.llvm.org/show_bug.cgi?id=51954

GitMensch commented 3 years ago

Also if you build PDCursesMod as DLL, you'll need to patch #include as follows.

#define PDC_DLL_BUILD 1
#include "curses.h"

that's one more thing that should be defined within the generated curses.h ...

Bill-Gray commented 3 years ago

@adv-sw, thank you; that gives me a bit more insight...

The actual usage is at lines 7720-7721 here, to get ncurses to recognize two keys it wouldn't normally recognize. All platforms of PDCurses would return those as KEY_BTAB and ALT_ENTER, so lines 95-95 of your source would have to be revised to read

#ifdef __PDCURSES__
 #define KEY_SHIFT_TAB    KEY_BTAB
 #define KEY_ALT_ENTER   ALT_ENTER
#else    /* for ncurses */
 #define KEY_SHIFT_TAB (KEY_MAX + 1)
 #define KEY_ALT_ENTER (KEY_MAX + 2)
#endif

and use of define_key() #ifdeffed out for PDCurses.

Also, at least on xterm on my Xubuntu 20.04 box, the control sequence for Alt-Enter is \033\012, not \033\015. (Usually, Alt-(key) will have the control sequence \033(key). The Enter key is an exception.) Not that it matters for the Windows ports; none of them use control sequences anyway.

However, all this would only explain why Shift-Tab and Alt-Enter wouldn't work; it doesn't explain why you're "erroring out". I suspect @GitMensch is either correct about PDC_DLL_BUILD being the culprit, or perhaps some other bits and bobs such as PDC_WIDE or PDC_FORCE_UTF8. As he mentions in his "rant", it'd be best if these were handled by modifying curses.h accordingly. (curses.h mentions, near the top, six items to "define before inclusion (only those needed)." You have to get all six the same between the library build and your project build.)

adv-sw commented 3 years ago

not a rant :) agree PDC_DLL_BUILD in the autogenerated fille is one less thing that can mess up :)

& yeah, that isn't going to be cause of all input messing up. lldb gui needs debugging to figure out cause of regression.

best I can do is inform for now. got other stuff I have to work on. must focus. apologies can't take it further.

the erroring out was with pdcurses, not yours & was when I was first experimenting & had it patched incorrectly.

so might be non-issue. I know yours runs (thanks), input failing ... reason unclear without diving into it.