awesome-gocui / gocui

Minimalist Go package aimed at creating Console User Interfaces.
BSD 3-Clause "New" or "Revised" License
350 stars 39 forks source link

[BUG] Parse() doesn't parse Alt, Shift modifiers and converts Ctrl to a key #91

Open abitrolly opened 3 years ago

abitrolly commented 3 years ago

Parse() fails process Alt+ and Shift+ combinations. See the code below.

package main

import (
    "github.com/awesome-gocui/gocui"
    "github.com/kr/pretty"
)

func main() {
    pretty.Println(gocui.Parse("q"))
    pretty.Println(gocui.Parse("ctrl+q"))
    pretty.Println(gocui.Parse("shift+q"))
    pretty.Println(gocui.Parse("alt+q"))
}

This outputs,

int32(113) gocui.Modifier(0) nil
gocui.Key(17) gocui.Modifier(0) nil
nil gocui.Modifier(0) &errors.errorString{s:"no such keybind"}
nil gocui.Modifier(0) &errors.errorString{s:"no such keybind"}

Another strange behavior is that https://pkg.go.dev/github.com/awesome-gocui/gocui#Parse should return modifier separately from Key, but it for Ctrl+ there is no Modifier and it is mixed into the key.

To Reproduce

https://play.golang.org/p/UPWMipkgA9U

Expected behavior

Key('q') Modifier() nil
Key('q') Modifier('ctrl') nil
Key('q') Modifier('shift') nil
Key('q') Modifier('alt') nil
KoditkarVedant commented 2 years ago

Hi @mjarkk @glvr182, I was looking to contribute to this project. I'm new to Go lang but I do have experience in other language. So I was going thought this bug description. I wonder is this really a bug because looking at the code in Parse method it is behaving as expected.

mjarkk commented 2 years ago

@KoditkarVedant You are correct the codes doesn't seem to support this tough i think you expect this kind of input to work.

You are welcome to add support for this!

KoditkarVedant commented 2 years ago

@mjarkk What are your views on using the https://github.com/awesome-gocui/keybinding instead of maintaining the separate implementation in this repo.

mjarkk commented 2 years ago

I think we should not use it to keep the amount of dependencies low.

abitrolly commented 2 years ago

@mjarkk how many new dependencies does it add?

dankox commented 2 years ago

I'm not really sure what is the purpose of the keybinding library. It doesn't provide any meaningful functionality when used alone and it is not a CLI (which I thought it is) as it only provides functions to translate into gocui keys which can be used only by gocui (which looks kinda similar to original gocui parsing functions with some additions).

I think that parsing of keybinds should be done in gocui as it's used by it so it would make more sense.
Regarding keybinding library, that could be maybe changed (or rather newly created) into CLI which would prompt for keypresses and some comments and create a Go file with all the function calls for the specific keys (probably some template function).

Anyway, just my two cents :) Regarding the original question, I'm with @mjarkk on this... there is no need to add another dependency on a file which was extracted from this library and put into separate library just to add it back to this library by a another dependency.
Also, I'm not sure if it's even possible, because that library is dependent on this library, creating circular dependency ;) I know it's a problem in one project between packages, but not sure how Golang would act when it's between modules.

abitrolly commented 2 years ago

For better portability across different frameworks, it would be nice to have some reference keybinding lib. Having it in separate repo at least should make it clear what approach is used, which constants are provided, how keys are referenced from code and from an application config.

A demo that allows to test different keys and combination would be useful. Especially for different language layouts.

dankox commented 2 years ago

I see, for such a case maybe the keybinding library makes sense. Although I guess it could be created as a submodule in this repo instead so the parser is only in one place, or maybe the parser in keybinding should use the parser from this library. Or users could just use this library to parse the keys because this library will be downloaded anyway.

Nevertheless, you can't really make dependency in this library on keybinding, because it is a circular dependency and keybinding uses gocui for the keys to produce them.

abitrolly commented 2 years ago

Ideally both should be merged into https://pkg.go.dev/golang.org/x/term instead of adding another layer to the cake. At least key codes and handling logic for them could be universal.

dankox commented 2 years ago

Hmmm... I guess we are talking about two different things here. keybinding or gocui keybinds are not based on golang.org/x/term library.
The keys used here are obtained from underlaying tcell library, which handles consoles/terminals across multiple platforms and have its own specific way of dealing with it.

golang.org/x/term doesn't really fit in at all, as it only provides support for classic shell type terminal with prompt and have no special key handling available to user from what I remember (only readline or something like that). It doesn't even have key codes publicly available, so using it as a third-party library to translate those keys into gocui can't be made even if one would want to.

On the other hand tcell is terminal UI library which provides full control of a terminal it runs on with cell based view and all kinds of other stuff.

So these are really totally different libraries and that idea is not applicable here at all. But this is probably going already a bit off-topic. If you have a specific use case in mind which would bring some light to the problem you see, I think it might be good to open a new issue and describe it there exactly with specific examples.