Codecov Report

Merging #117 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #117   +/-   ##
=======================================
  Coverage   23.41%   23.41%           
=======================================
  Files          13       13           
  Lines        1089     1089           
=======================================
  Hits          255      255           
  Misses        821      821           
  Partials       13       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cca69e6...abc54e0. Read the comment docs.

Codecov Report

Merging #117 into master will decrease coverage by 2.57%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   26.12%   23.55%   -2.58%     
==========================================
  Files          13       13              
  Lines        1129     1091      -38     
==========================================
- Hits          295      257      -38     
  Misses        821      821              
  Partials       13       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8970352...1b5afd2. Read the comment docs.

@jesseduffield and anyone interested

I'm in a bit of a pickle, keybindings.ParseAll() returns both the an object(containing the key, the modifier and the tokens), and an error.
How should i go about setting the keybindings in keybinding.go?
I was thinking about a for loop, but that seems like a lot of work for a not so pretty solution.
I'm now thinking of a helper function that panics if it contains an error and having a recovery function to catch that, but even with that i'd have to refactor a lot

The keybinding struct (now having []interface{} instead of interface{})

type Binding struct {
  ViewName    string
  Handler     func(*gocui.Gui, *gocui.View) error
  Key         []interface{} // FIXME: find out how to get `gocui.Key | rune`
  Modifier    gocui.Modifier
  Description string
}

The recovery

defer func() {
  if r := recover(); r != nil {
    fmt.Println("Recovered in f", r)
  }
}()

The keybinding def

{
  ViewName: "",
  Key:      filterParser(keybinding.ParseAll("ctrl+A, ctrl+B, CTRL+ALT+C")),
  Modifier: gocui.ModNone,
  Handler:  gui.quit,
},

The helper function

func filterParser(binds []keybinding.Key, err error) []gocui.Key {
  var keys []gocui.Key
  if err != nil {
    panic(err)
  }
  for _, bind := range binds {
    keys = append(keys, bind.Value)
  }
  return keys
}

@glvr182 I think we should put the default KeyConfig on the UserConfig struct in app_config.go. We can then have our filterParser method in that file and it can handle all of our defaults on startup.

In UserConfig:

Keybindings KeyConfig `yaml:"keybindings,omitempty"`

In GetDefaultConfig

		Keybindings: KeyConfig{
			Quit: []string{"q"},
                         ...
		},

Then in keybindings.go

		{
			ViewName: "",
			Keys:     parsedKeybindings(gui.Config.UserConfig.Keybindings.Quit),
			Modifier: gocui.ModNone,
			Handler:  gui.quit,
		},

and then in the same file:

func parsedKeybindings(binds []string) []gocui.Key {
  var keys []gocui.Key
  for _, bind := range binds {
    keys = append(keys, keybindings.MustParse(bind)) // MustParse panics on failure
  }
  return keys
}

you'll need each keybinding to support an array of keys, so then you'll be doing this in keybindings():

	for _, binding := range bindings {
		for _, key := range binding.keys {
			if err := g.SetKeybinding(binding.ViewName, key, binding.Modifier, binding.Handler); err != nil {
				return err
			}
		}
	}

I think it's worth adding a MustParse() function to the keybindings library, which panics on failure, so that we don't need to worry about the error that could be returned. As you say we can recover from it.

Hopefully the fact we now need to use an []interface{} isn't too bad, though I have had bad experiences with interface{} arrays in the past.

What are your thoughts?

I think having a must parse function in the lib is a good idea, I shall make that function tomorrow, (or if you have time be my guest).

I'm not the biggest fan of interfaces either, i prefer having an actual type, but maybe we find a way to resolve that along the road. (might try that in this pr).

I think with the MustParse function I'll be able to finish this soon.

Thanks for your input!

Just finished the mustParse functions, will use them now

@jesseduffield I'm again in a bit of a pickle, keybinding library uses awesome-gocui/gocui, lazydocker uses jesseduffield/gocui, what do you recon would be the best

@glvr182 that is a good question. I would like for the two gocuis to be one-and-the-same, but I haven't really put the effort in to port my stuff over to awesome-gocui/gocui when I add something to my own.

What are your thoughts on defining the gocui keys in the keybindings package and casting them in the consuming package? I'm assuming they are all integers under the hood? Otherwise, if we think that it's going to be too much work to get the two gocuis synced up, I could fork the keybindings package and get it to point at my own gocui.

@jesseduffield They are as far as i know uint16, I don't really like the idea of casting, for obvious reasons, but as you said we would have to update awesome-gocui/gocui a lot. Maybe we could focus on that, as others might have use for it aswell.

agreed

I'm settings the defaults right now, it's taking longer than i hoped, thanks for the patience

Rebased from master

@jesseduffield might it be possible to lock the app_config.go and keybindings files for the time being? Just untill the pr is done

sure, how long do you think you'll need?

Well once i set all the defaults, i can test it. So how about this weekend?

Apart from the errors, i think you can have a look at the keybinding configs @jesseduffield

Seems to be an issue with parsing characters to keys, will have to do more investigation
Might end up merging keybindings into gocui

Hit a bug while setting up the parser, hope to get stuff working tomorrow

@glvr182 how is it going? Maybe need some halp?

Yea well I back then hit a bug that had to do with some casting stuff. Haven't been able to fix that, but i was working on merging jesse's gocui with the awesome-gocui/gocui one to fix it. I'll post some errors tonight.