tview icon indicating copy to clipboard operation
tview copied to clipboard

Documenting that app keys are not overridable by widgets

Open abitrolly opened this issue 3 years ago • 10 comments

In CustomKeys wiki page it is not mentioned that keys set by Application.SetInputCapture are always active and can not be overridden by widgets. As discussed in https://github.com/rivo/tview/issues/662#issuecomment-968158591.

EDIT: Here is where the key processing starts in tview.

https://github.com/rivo/tview/blob/9994674d60a85d2c18e2192ef58195fff743091f/application.go#L309

abitrolly avatar Mar 22 '22 09:03 abitrolly

It looks like nothing can be overridden by widgets. I add Modal to Pages and set input capture for both of them. When the modal is active, the key is first handled by parent Pages widget and then passed to Modal.

        // 2. modal with detailed info
        infobox := tview.NewModal().
                AddButtons([]string{"Quit", "Cancel"}).
                SetText("Lorem Ipsum")
        // 3. layout with two pages (second page is needed to show modal)
        pager := tview.NewPages().
                AddPage("list", list, Resize, Visible).
                AddPage("infobox", infobox, Resize, Hidden)
        rootpager.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
                // Key is a code
                if event.Key() == tcell.KeyESC {
                        log.Println("ESC pressed in root widget")
                        //app.Stop()
                }
                // Key is a character
                if event.Key() == tcell.KeyRune {
                        if event.Rune() == 'q' {
                                app.Stop()
                        }
                        if event.Rune() == 'i' {
                                rootpager.ShowPage("infobox")
                        }
                        if event.Rune() == 'o' {
                                rootpager.SwitchToPage("list")
                        }
                }
                return event
        })

        infobox.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
                if event.Key() == tcell.KeyESC {
                        log.Println("ESC pressed")
                        //pager.HidePage("infobox")
                        //return nil
                }
                return nil //event
        })

Pressing 'i' to show modal and then Esc gives two messages.

ESC pressed in root widget
                          ESC pressed

This makes it impossible to override shortcuts in a child widget. I expected the keys to be processed by child widgets first. With current behavior the parent widget needs to know about all keys used by child to skip handling them if the child is active.

The defaults override mentioned in CustomKeys work only to change default widget behavior, but not to change the function of keys in the parent widget.

I think that widget compositing vs inheritance is currently not covered.

abitrolly avatar Mar 23 '22 11:03 abitrolly

Basically the scenario I expected.

  1. Root widget gets key event
  2. If the key is overridden by focused child, it waits until the child processes the event
  3. If the child returns nil, the root widget skips processing the key
  4. If the child returns event, the root widget processes the event

abitrolly avatar Mar 23 '22 13:03 abitrolly

For the scenario you described, I think you could make use of the HasFocus() and GetInputCapture() calls appropriately, i.e. you could check if infobox is focused in the rootpager's InputCapture, and process the keyevent appropriately.

For example:

        infobox.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
               if event.Key() == tcell.KeyESC {
                        log.Println("ESC pressed")
                        //pager.HidePage("infobox")
                        //return nil
                }
                return nil //event
        })  

        rootpager.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
               // Key is a code
               // Root widget gets key event
               if event.Key() == tcell.KeyEscape {
                           // If the key is overridden by focused child, it waits until the child processes the event
                           if infobox.HasFocus() {
                              infoEvent := infobox.GetInputCapture()(event)

                              // If the child returns nil, the root widget skips processing the key
                              if infoEvent == nil {
                                    return nil
                              }

                              // If the child returns event, the root widget processes the event
                              //app.Stop()
                              return event
                        }
               }

                // Key is a character
                if event.Key() == tcell.KeyRune {
                        if event.Rune() == 'q' {
                                app.Stop()
                        }
                        if event.Rune() == 'i' {
                                rootpager.ShowPage("infobox")
                        }
                        if event.Rune() == 'o' {
                                rootpager.SwitchToPage("list")
                        }
                }
                return event
        })

darkhz avatar Apr 16 '22 05:04 darkhz

@darkhz thanks for the snippet. It solves the problem, but the burden is that all parent widgets should be modified to account for all shortcuts in child widgets. It could work if there is a way to forward event to child widget tree without explicitly mentioning them.

abitrolly avatar Apr 16 '22 06:04 abitrolly

Here is the diagram of how tview currently processes key event. There is no event forwarding beyond first root widget, so it is up to the root widget to forward the event further, manage focus etc. app input doesn't do focus management or any logic for forwarding events between widgets.

flowchart LR
    A("Application.Run()") -->|event| ICSET{a.inputCapture}
    ICSET -- nil --> CtrlC{event}
    CtrlC -- KeyCtrlC --> Q(Quit)

    ICSET --> AICE{"a.inputCapture(event)"}
    AICE --> CtrlC

    CtrlC --> ARIH{a.root.inputHandler && hasFocus}
    ARIH --> ARIHE("a.root.inputHandler(event, setFocus)")
    AICE -- nil --> C(continue)
    ARIH -- nil --> C

    ARIHE --> ARICE("a.root.inputCapture(event, setFocus)")
    
    click A "https://github.com/rivo/tview/blob/9994674d60a85d2c18e2192ef58195fff743091f/application.go#L309" _blank
    classDef link color:blue
    class A link

Markdown source for the diagram.

abitrolly avatar Jun 04 '22 18:06 abitrolly

Signature of application.inputCapture is different from widget.inputCapture. Widget handlers are passed setFocus handler. Not sure how it is supposed to be used. Looks like widgets don't have a reference to main app object to pass focus to another control, and that there is global SetFocus function.

abitrolly avatar Jun 05 '22 05:06 abitrolly

Yes, it is a role of root widget to forward keyboard event to the next. So the next widget decides what of its children have focus to react to.

https://github.com/rivo/tview/blob/9994674d60a85d2c18e2192ef58195fff743091f/grid.go#L266

https://github.com/rivo/tview/blob/9994674d60a85d2c18e2192ef58195fff743091f/pages.go#L308-L310

So it looks like only the widget knows what element holds the focus, and there is no way to track where focus is globally, like from Application. Or do I miss something?

abitrolly avatar Jun 05 '22 06:06 abitrolly

So for every widget there are two handlers.

  • widget.GetInputCapture - empty by default, set by user as described in CustomKeys
  • widget.InputHandler - implements widget keys

Now the problem is that widget.InputHandler doesn't return anything, so it is impossible to tell if a widget processed the key. So my scenario where event of forwarding unprocessed events to parents doesn't work.

abitrolly avatar Jun 05 '22 13:06 abitrolly

Another problem is that app.GetFocus() returns Primitive, which doesn't have GetInputCapture method. So I am blocked with my top-down event processing idea.

https://github.com/rivo/tview/blob/9994674d60a85d2c18e2192ef58195fff743091f/application.go#L726

abitrolly avatar Jun 05 '22 14:06 abitrolly

The top-down implementation was made based on #421 and I have to agree it should be possible to intercept key presses at any level of the widget hierarchy regardless of what's happening further down.

Going back to the original question, I'm trying to understand your reasoning for giving a key a different function in a higher-up "container" class such as Pages. It looks like you want Esc to close the application but not if the infobox is open. I have a feeling such a context-dependent redefinition of a key can lead to frustrations of the user (e.g. accidentally closing the application) — but let's go with it for now. Why do you process Esc in Pages then instead of in Application?

rivo avatar Jun 10 '22 15:06 rivo

@abitrolly Is this issue still relevant?

rivo avatar Dec 17 '22 18:12 rivo