-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add bare-minimum help dialog #58
Conversation
1b042db
to
b2626bf
Compare
Thanks for implementing!
We should probably add some kind of currently-focused-element Unfortunately, I used to use
This is concerning. What if the terminal is too small by nature? Can you add a test with a very small height viewport to make sure that it doesn't crash?
Another simple solution could be to add f-key shortcuts to toggle the menu bar items: #44 (comment). Those would be easy to add to the button text itself, and naturally expose the entire shortcut interface. |
If you let me know which function keys you'd like I'm happy to add them. Maybe F2+, reserving F1 for help? I think I agree with the idea of a "currently focused" enum and getting rid of the full combinatorial nature of the mega switch. I didn't know how much time I've got to contribute to this so if that's a blocker I can't commit to landing this. The crash was valid and a programming error -- it was an unsatisfiable constraint because I asked to display a block that was both 90% of my screen tall and required to be less than 20% of my screen. I'm afk, but I bet if you tried to quit when you have a screen that's less than 5 lines tall you'd get the same crash. |
Yeah confirmed it exists with the current code: Screen.Recording.2024-07-07.at.16.09.09.movIt's smaller than I expected, I'll change mine to be one line too to make it exactly as unlikely as the current problem. Although it is actually worse/more obvious with a narrow terminal: Screen.Recording.2024-07-07.at.16.13.00.movBacktrace on current main
|
Pressing question mark pops up a dialog that tells you you can use the mouse to click menu items to do things and view mouse shortcuts. Obviously this is not perfect, but it would have saved me a bunch of frustration, and it does a fair amount of the grunt work towards a better help box. One thing that made me stop here is that the main event loop should probably be split up to do different things depending on if any given dialog is open. Adding the HelpDialog using the current paradigm would have involved approximately tripling the size of the match for no benefit.
Actually, looking at the code and it feels like a rabbit hole that I would rather not dive into unless you agree with one of the following:
With any of these I probably won't get around to it until next weekend, and I can't super commit to doing it, I thought it would be less work to do when I said I'd be happy to. |
For now the lightest weight thing to do would be to make the top-level match 3 functions instead and do something like: if self.help_dialog.is_some() {
self.handle_help_event(event, ...)
} else if self.quit_dialog.is_some() {
self.handle_quit_event(event, ...)
} else {
self.handle_base_event(event, ...)
} but clearly that wouldn't help for when this app has a multi-pane view and I'm not sure if things are intertwined enough currently that that refactoring wouldn't be a pain. |
b2626bf
to
3ffb304
Compare
Oof, that's unfortunate. Thanks for investigating and for the PR! |
Pressing question mark pops up a dialog that tells you you can use the mouse to click menu items to do things and view mouse shortcuts.
Obviously this is not perfect, but it would have saved me a bunch of frustration, and it does a fair amount of the grunt work towards a better help box.
One thing that made me stop here is that the main event loop should probably be split up to do different things depending on if any given dialog is open. Adding the HelpDialog using the current paradigm would have involved approximately tripling the size of the match for no benefit.
I tried to add all the shortcuts to the help dialog, but that (A) crashed because the dialog has a reasonable hard limit on vertical size and I didn't want to investigate scrolling -- a motivated coder could probably add that easily now. And (B) ideally this would maybe use the the existing Menu to build out the shortcuts, but that requires fancier stuff and has actual code design implications that folks would probably like to talk about it and I'm just trying to be a tiny bit helpful.
This is an incremental step towards #25
scm-record-help.mov