Refactoring the key handling #3916
Replies: 7 comments 6 replies
-
|
Quoting from #3913 as it is directly related:
So I took km_dokey_event() -> km_dokey_event_line(), made it accept Keymap *map (I kept mtype for now). I know it's not much for now, but I need to know if I'm doing this right, or I'm completely off the course with this. PS. Most of the business logic went to km_dokey_event2() |
Beta Was this translation helpful? Give feedback.
-
Yes, you sound like you understand things :-)
One of the hardest problems in Computer Science :-)
OK. I think we could simplify things. Change to a fixed: If it returns a real event, then handle it. However, if it returns
A better long-term solution might be:
I look forward to it. |
Beta Was this translation helpful? Give feedback.
-
|
Staring for too long at km_dokey_event() got me really confused. Is it related to the fact that we need to lookup several MENUS at once to try to find the correct opcodes. Like what we're about to do for mw_get_field(), Or is there a bigger picture in key opcode translation, that somehow I miss ? |
Beta Was this translation helpful? Give feedback.
-
Don't worry, NeoMutt's code has that effect on people :-)
First, we don't need to fix anything. You added You can stop reading here :-) |
Beta Was this translation helpful? Give feedback.
-
|
However, it's a good opportunity to examine how these functions work and,
Yes. Some Dialogs rely on up to four different The Pager has:
These match the four "domains" of actions. If you now look at The first step is to change |
Beta Was this translation helpful? Give feedback.
-
Looking much further ahead, I want to move the event loop out of the Dialog. We'd need to split Some thought is needed on how keybindings will work. A Dialog would define which domains it accepts: The event loop of many Dialogs is quite small: do
{
op = km_dokey(MENU_ALIAS);
rc = alias_function_dispatcher(dlg, op);
if (rc == FR_UNKNOWN)
rc = menu_function_dispatcher(menu->win, op);
if (rc == FR_UNKNOWN)
rc = global_function_dispatcher(NULL, op);
} while (rc != FR_DONE);If we've defined our domains, then this loop can be eliminated and replaced with a universal function. Eventually, we'd have standardised Dialogs. The caller would: struct Dialog *dlg = alias_dialog_create(args);
int rc = dlg_run(dlg);
dlg_free(&dlg); |
Beta Was this translation helpful? Give feedback.
-
So struct MenuFuncOp will act as a haystack, and probably KeyMap (MenuType right now) will act as the needle. However I'm unsure about cleaning up MenuFuncOp structures like OpAlias, OpPager, etc. eg: I'm sure this mess will cleanup in the 'much further ahead' phase where we will have DomainMENU like structures. Come to think of it right now you we're saying the same thing in reply 2 and 3 (in reply 1 you're telling me to mind my own business - but I ignored that :) ) On the matter of keybindings in the later reply, keybindings is a matter that's bugging me for a while. Come to think of it we could probably alter the order of the elements in haystack to give higher Right before your reply I had another reply prepared but I dropped it in fear I was completely off course, Anyhow I'm still thinking about all this. You do realize if we keep this up we'll end up rewriting NeoMutt from the ground up in one single PR :) PS. Thanks for taking the time to post really long replies, I figured I could do the same ;) |
Beta Was this translation helpful? Give feedback.
-
No problem.
Yes
I have some ideas, but they're not fully-formed yet. OpGeneric might be the starting point.
Yes, I've thought the same thing, can we create a combined struct: { "previous-entry", OP_PREV_ENTRY, "<up>" },The catch is that some functions have multiple keybindings, e.g. { OP_FIRST_ENTRY, "<home>" },
{ OP_FIRST_ENTRY, "=" },
Yes and no. I mentioned moving the event loops out of the dialogs... The user hits a key 'X' and NeoMutt asks the focussed window if it can handle it. The Sidebar would be a self-contained unit that observes events and updates its display.
I'm grateful that you're working on the bigger picture,
Yes, we don't need to alter them yet, but we can start thinking about overlaps.
The main difference is that the Dialogs expect an opcode and mw_get_field() needs both opcodes and raw keys.
Hmm... yes
It's MENU_SIDEBAR now, but in the future we'll want to pass in other menus.
I think we should leave this alone.
Yes, exactly
That's a possibility, but we'd still need logic to tell us which of MENU_ALIAS or MENU_GENERIC comes first.
That's great, thanks!
I'm still pretty confident the plan will solve a lot of my/our problems.
hehe |
Beta Was this translation helpful? Give feedback.
-
|
Here's an example of Menu splitting OpGenericIt seems that there are almost no generic functions in OpGeneric.
|
Beta Was this translation helpful? Give feedback.
-
|
NeoMutt Key Bindings: https://gist.github.com/flatcap/fc84519ad7361f6e093eadda039aa0eb |
Beta Was this translation helpful? Give feedback.
-
|
A few words about my dungeon, err .. that's km_dokey_event(). I was set to do something in the "much further ahead" phase, but it turns out I do not fully understand the km_dokey_event() behavior yet. Instead Ive been trying to add multiple keymap / menufuncsop, and it gave the chance to debug km_dokey_event() even further to understand its behavior. What I came up with was: hs=haystack KeymapSeq, MenuFuncOps are structs composed of struct Keymaps, MenuFuncOp. I call km_dokey_event the following way: Things are working, mostly, there are still things needing to be handled here and there. Last time we were discussing the possibility of joining KeyBindings and MenuFunc.
I don't necessary see a problem with this. I think it will work, but we will mostly invalidate Keymap stuff, I believe that's probably used for macros too. we would do a single traversal of the structure and see if it's a keybinding (business as usual) or a MenuFuncOp. Better get something debugged. Oh, on the same note, how does one debug km_dokey_event() like functions? It's so re-entrant and so used everywhere Lastly, I've had to raise() on if (mtype == 10 && event.op == OP_MAIN_CHANGE_FOLDER) and use gdb to catch the caller, it was OpIndex from dlg_index() obviously, but didn't know that back then. |
Beta Was this translation helpful? Give feedback.
-
I tend to start with a lot of tail -F ~/.neomuttdebug0 | sed 's/.*\]<.> [a-z0-9_]\+() //'GNU's My other debugging tips... Use the command line to get to the start of your test: # Start Neomutt and compose an email
./neomutt -e "push 'mjim@example.com<enter>hello<enter>'"If NeoMutt is running and "at the right place", you can attach a debugger from another window: gdb --pid $(pidof neomutt)(or cgdb which is my preferred wrapper)
gdb lets you set conditional breakpoints One final timesaver I like is breakpoint commands. When the breakpoint is hit, gdb prints the values of two params automatically. I'll reply to the rest of your post when I've understood it :-) |
Beta Was this translation helpful? Give feedback.
-
That looks good
Another problem, that I've just noticed is that some MenuOpSeq have const struct MenuOpSeq AliasDefaultBindings[] = { /* map: alias */
{ OP_TAG, "<space>" },
};Also note that the
Sorry, you've lost me there. |
Beta Was this translation helpful? Give feedback.
-
This is every function involved in key handling.
On the far right is the curses function
getch(), and every left-hand leaf is a consumer.A few consumers, like
mutt_what_key()use the keycode directly, but the majority expect an opcode, e.g.OP_DELETE.Source: graphviz, svg
Key:
mutt_getch()has three possible sources for input.In order of priority:
UngetKeyEvents: a buffer of keys that have been pushed back.There are some functions that need to peek at the next character, but might not need it.
They can
mutt_unget_ch()to return it.MacroEvents: a buffer of pending macro keystrokes.When a macro is encountered, it's keystrokes are pushed into this buffer, then
mutt_getch()can be called as normal.This can be temporarily ignored by setting the global
OptIgnoreMacroEvents.getch(): an ncurses function that actually checks the keyboard.km_dokey_event()is responsible for the key --> opcode translations.It's also responsible for calling
imap_keep_alive()on timeouts.km_dokey_event()takesMenuTypeparameter, e.g.MENU_ALIAS.It uses the global
KeyMapsto get a list of key bindings / macros (struct Keymap).If it can't find a matching binding in the given menu, it tries the Generic bindings.
If they fail too, it searches all the menus for a match then ungets the function as a string
<name>.km_dokey_event()includes business logic to provide special behaviour for the Editor, Pager and Generic menus.Beta Was this translation helpful? Give feedback.
All reactions