Async counter example text input broken

[JanKleine]

Hi,

Thanks for providing such a comprehensive template!

I tried to play around with the async counter example, but the text input doesn't work for me.

After some digging I found the issue and there is a quick semi-fix, but a somewhat more annoying issue remains, which also applies to the template and not just the counter example.

Quick Fix

The handle_key_events() function of the Home Component is never called. This is because the handle_events() function is overwritten with the handling of the mouse event. Changing the function name to handle_mouse_events() fixes the issue (with some minor other changes to make the compiler happy).

Remaining issue

Typing a lowercase q in the input field still makes the program quit, even in insert mode. This is because the key presses are passed through the key map and the resulting action is triggered regardless of component state. I think this is a bigger issue, as TUI applications usually quit on q and often require text input. So I think it's probably best to make the template easily compatible with such applications.

Maybe a possible solution would be to let component cancel actions, or to move the keybinding handling into the component (the keybindings are in the home section of the config, so maybe their logic should also be in the Home component so it can depend on the component state). The handling for the j,k,/,? keys is already inside the component so they can be handled better. But that raises the question about the case when multiple components need to handle the same key (e.g. two components with text inputs). Maybe full text input should be a first class citizen, that components can trigger, so it's handled outside of components all together?

These ideas are not very thought out and I'm not totally happy with them. I thought I'd first try to get some feedback as you likely have a better overview over how things are and should work.

I'm happy to help with a PR later

[joshka]

These ideas are not very thought out and I'm not totally happy with them. I thought I'd first try to get some feedback as you likely have a better overview over how things are and should work.

I'd definitely encourage you to share more feedback on any aspect of this template (including the docs). The async template is pretty much a single person effort (props to @kdheepak). But as a result it hasn't the same level of feedback as the more core parts of the library, and it needs it to help make it meet your use cases (or in case @kdheepak wins the lottery tomorrow).

[kdheepak]

Thanks for the feedback! Sorry for the frustrating experience. I did think of this issue when I was first added a text input field in the template but I think while adding features to the template along the way (e.g. reading key configuration from a file etc) I didn't test everything in the counter app and didn't account for this.

I've fixed this particular issue in the linked PR. The way I fixed it is the "intended way" of dealing with this kind of issue.

Specifically, every component can return a Option<Action> on handling an update, which allows it to modify the state of the application further. This is the mechanism I intended people can use to modify state anywhere else in the app from any component. The only disadvantage (imo) is that your Action enum can become bloated.

For this issue, I introduce a Mode::HomeInput which doesn't have any of the key bindings associated it and then all key inputs are only processed by the text input field. I also introduce a Action::EnterHomeInput and Action::EnterHomeNormal actions to switch the mode of the app state (renamed to Action::EnterModeHomeInput and Action::EnterModeHomeNormal in a subsequent PR).

I say "intended way" in quotes because there's so many ways to architect a ratatui application and this is just one way to do it. For example, I think another way to solve this be to introduce a layer in the key configuration that uses component name + mode:

{
  "keybindings": {
    "Home": {
      "Normal": {
          "<q>": "Quit", // Quit the application
          "<j>": "ScheduleIncrement",
          "<k>": "ScheduleDecrement",
          "<?>": "ToggleShowHelp",
          "</>": "EnterInsert",
          "<Ctrl-d>": "Quit", // Another way to quit
          "<Ctrl-c>": "Quit", // Yet another way to quit
          "<Ctrl-z>": "Suspend" // Suspend the application
      },
    }
  }
}

Generally, I want to say that this template is very opinionated + also not as well documented as I'd like. It's also a trivial example with complicated features to showcase the features.

I feel more strongly about moving this template to a different name, like component or opinionated, and putting together a different simpler template for async specifically: https://github.com/ratatui-org/templates/issues/36

We are discussing a rewrite to the tutorial documentation and I think when that happens I'll revisit this template as well and rework whatever example we use in the tutorial to use this "component"-based template.

[kdheepak]

I've reopened the issue to get feedback from you and see if you have any more ideas on how you might have gone about making a PR to sort this out.

[kdheepak]

I've made another PR where Mode is now a shared value across the Home component and the App, using Rc<RefCell<Mode>>.

This doesn't use the "intended way" of fixing this problem. Rather, this is what I should have done to begin with, i.e. Mode should be global across all components of the app, and each component could have its own modes or use a subset of the global mode.

This PR also lets you add key configurations per mode, for example:

    "Insert": {
      "<Ctrl-h>": "ToggleShowHelp", 
    },

I think there may be a bug somewhere related to key-events though, since I'm not able to get <Ctrl-?> to work. But that isn't probably related to this discussion so I'll look into that separately.

[JanKleine]

Thank you very much for the detailed answer and fix! I’ll check it out as soon as I can (which won’t be until mid/end of next week :/ ).

[CableSynth]

The counter was broken again when #45 was merged in. The fixes discussed in this issue were deleted.