WIP: Add menu panel (cheatsheet) #234

dawidd6 · 2018-08-28T18:20:23Z

My proposition is to add 2 new fields to Binding struct. One for human readable key (i don't know how to pretty print current Key field value) and the other one simply for description of what this key is responsible for. Then we can straightforwardly generate a cheatsheet on the fly. Possible in the future we'll be able to simply export this cheatsheet to separate file and for example put it on github as a replacement for manually written Keybindings.md.

Panel is currently set to maximum width and height, i think it's convenient.

Closes: #237

golangci · 2018-08-28T18:20:44Z

pkg/gui/help_panel.go

+}
+
+func (gui *Gui) handleHelpClose(g *gocui.Gui, v *gocui.View) error {
+	g.SetViewOnBottom(v.Name())


Error return value of g.SetViewOnBottom is not checked

golangci · 2018-08-28T18:20:45Z

pkg/gui/help_panel.go

+	helpView, _ := g.SetView("help", 0, 0, maxX-1, maxY-2, 0)
+	helpView.Title = strings.Title(gui.Tr.SLocalize("help"))
+
+	gui.renderHelpOptions(g)


Error return value of gui.renderHelpOptions is not checked

golangci · 2018-08-28T18:20:45Z

pkg/gui/help_panel.go

+		}
+	}
+
+	helpView.Write([]byte(content))


Error return value of helpView.Write is not checked

golangci · 2018-08-28T18:20:45Z

pkg/gui/help_panel.go

+	helpView.Write([]byte(content))
+
+	g.Update(func(g *gocui.Gui) error {
+		g.SetViewOnTop("help")


Error return value of g.SetViewOnTop is not checked

golangci · 2018-08-28T18:20:45Z

pkg/gui/help_panel.go

+
+	g.Update(func(g *gocui.Gui) error {
+		g.SetViewOnTop("help")
+		gui.switchFocus(g, v, helpView)


Error return value of gui.switchFocus is not checked

jesseduffield · 2018-08-28T23:32:17Z

I like your proposition, but I think we could benefit from taking this one step further:

I've made a feature request denoting what I think would kill two birds with one stone:
#237

Effectively I want to have global keybindings displayed at the bottom at all times, and panel-specific keybindings available via a popup context menu that comes up when you press e.g. '?' or something similar. That way users don't even need to learn the keybindings, they can just use context menus. But the context menus will show the keybindings in each line aside the keybinding description, meaning if users want to use the popup as a cheatsheet they can too.

What are your thoughts on this?

dawidd6 · 2018-08-29T10:00:20Z

So if i understand right, you want to have ? key available globally but when for example focus is on commits panel then pressing ? will only display the keybindings for commits panel?

jesseduffield · 2018-08-29T10:04:57Z

yep :) It could be a better keybinding than '?' but yes that's the idea

dawidd6 · 2018-08-29T10:06:38Z

Also the cheatsheet should be selectable with space to execute specific handler?

jesseduffield · 2018-08-29T10:07:04Z

yep :)

dawidd6 · 2018-08-29T10:07:52Z

Okay will tinker with that.

jesseduffield · 2018-08-29T10:08:28Z

awesome lemme know if you have any questions

dawidd6 · 2018-08-29T11:04:53Z

@jesseduffield i'm currently thinking about size of the panel, do you have any desired dimensions?

jesseduffield · 2018-08-29T12:01:01Z

i think vertically it should just fit its contents, and horizontally maybe it should be 2/3 the screen width, as is currently the case with confirmation panels. If that is too wide we can shrink it down

dawidd6 · 2018-08-29T12:12:47Z

And could you tell me how to set it to fit its contents?

dawidd6 · 2018-08-29T12:29:25Z

Current state:

codecov-io · 2018-08-29T12:34:39Z

Codecov Report

Merging #234 into master will increase coverage by 1.89%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
+ Coverage   84.82%   86.72%   +1.89%     
==========================================
  Files           9        9              
  Lines        1364     1559     +195     
==========================================
+ Hits         1157     1352     +195     
  Misses        206      206              
  Partials        1        1

Impacted Files	Coverage Δ
pkg/i18n/dutch.go	`100% <100%> (ø)`	⬆️
pkg/i18n/polish.go	`100% <100%> (ø)`	⬆️
pkg/i18n/english.go	`100% <100%> (ø)`	⬆️

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 5af03b6...63e4006. Read the comment docs.

jesseduffield · 2018-08-30T09:53:07Z

@dawidd6 looking good! There is a function in confirmation_panel.go called getMessageHeight which takes a string and a width and tells you how tall your view needs to be to contain it. So I'd recommend building up your key array and the display string first, then using that to determine the dimensions of your view, rather than making the view first and then populating it.

dawidd6 · 2018-08-30T11:50:50Z

pkg/gui/keybindings.go

-		{ViewName: "status", Key: 'u', Modifier: gocui.ModNone, Handler: gui.handleCheckForUpdate},
+		{ViewName: "", Key: '?', Modifier: gocui.ModNone, Handler: gui.handleHelp},
+		{ViewName: "status", Key: 'e', Modifier: gocui.ModNone, Handler: gui.handleEditConfig, KeyReadable: "e", Description: "edit config"},
+		{ViewName: "status", Key: 'o', Modifier: gocui.ModNone, Handler: gui.handleOpenConfig, KeyReadable: "o", Description: "open config"},


@jesseduffield how do you feel about this? Should we indent the fields now since it is becoming long with 2 new fields?

yep I think we should

dawidd6 · 2018-09-01T13:16:48Z

@jesseduffield hey do we need space or other keys like this visible in help menu? I mean arrows, spaces or other special keys would be visible only at the bottom or do you want them to be also in help menu?

dawidd6 · 2018-09-01T15:33:06Z

Test failing fix: #250

jesseduffield · 2018-09-03T12:07:57Z

@dawidd6 I would include the space, but not the arrow keys. Navigation keys should stay at the bottom, but any key that triggers an action like space etc should be in the menu.

The exception is global keys which should just be shown at the bottom. I guess there is no harm in putting pull/push in the context menu for the files panel or branches panel, but for now I think's simpler just to show panel-specific keybindings in the context menu.

How does that sound to you?

dawidd6 · 2018-09-03T13:42:28Z

Okay. I will rebase this PR with latest master, include spaces and i think it will be all.

- delete scrolling ability - lines are now selectable - implemented handler execution when space is pressed - add example descriptions for status panel keybindings

dawidd6 · 2018-09-05T11:30:20Z

Also the generated cheatsheet currently looks like this:

Lazygit menu

Global

	P      push
	p      pull
	R      refresh

Status

	e      edit config file
	o      open config file
	u      check for update

Files

	c      commit changes
	C      commit changes using git editor
	space  toggle staged
	d      delete if untracked / checkout if tracked (aka go away)
	m      resolve merge conflicts
	e      edit file
	o      open file
	i      add to .gitignore
	r      refresh files
	S      stash files
	A      abort merge
	a      stage/unstage all
	t      add patch
	D      reset hard

Branches

	space  checkout
	c      checkout by name
	F      force checkout
	n      new branch
	d      delete branch
	D      delete branch (force)
	m      merge into currently checked out branch

Commits

	s      squash down
	r      rename commit
	R      rename commit with editor
	g      reset to this commit
	f      fixup commit

Stash

	space  apply
	g      pop
	d      drop

jesseduffield · 2018-09-05T12:24:23Z

@dawidd6 I was actually thinking we might make it even easier than that: perhaps we could use the tab key? But then, the tab key is already used for cycling through views and adding a newline in a commit message panel.

I was thinking 'm' (demoting the existing 'm' keybindings to shift+'m') but that's only easy to reach if you're using vim keybindings.

Enter is another good solution, but I think I still want to reserve that for focusing the main view (so that you can then scroll a diff with the arrow keys)

I would use 'h' but that's used for vim keybindings. What about 'g'? It's within reach for both regular keybindings and vim keybindings, and we'd only be demoting existing keybindings which don't semantically map well to 'g' in the first place (i.e. stash popping and resetting to a commit, which we can replace with 'x').

jesseduffield · 2018-09-05T12:24:42Z

btw awesome work on the automatically generating cheatsheet :)

dawidd6 · 2018-09-05T13:24:59Z

Thanks for kind words 🎉.

Maybe better to not break currently established set of keybindings and use x for menu?

EDIT: changed it already anyway, can revert ofc.

jesseduffield · 2018-09-05T23:33:30Z

yep x sounds good

jesseduffield · 2018-09-05T23:38:34Z

pkg/gui/menu_panel.go

+	for _, binding := range bindings {
+		if key := gui.GetKey(binding); key != "" && (binding.ViewName == v.Name() || binding.ViewName == "") && binding.Description != "" {
+			if binding.ViewName != current {
+				content += "\n"


could we move global keybindings to the bottom of the menu? I think that most people will just use the keybindings for these, and would want to see panel-specific menu items first

jesseduffield · 2018-09-05T23:45:17Z

pkg/gui/menu_panel.go

+	padWidth := gui.GetMaxKeyLength(bindings)
+
+	for _, binding := range bindings {
+		if key := gui.GetKey(binding); key != "" && (binding.ViewName == v.Name() || binding.ViewName == "") && binding.Description != "" {


minor gripe but we can flatten this a bit by saying

key := gui.GetKey(binding) if key == "" || binding.Description == "" || (binding.ViewName != v.Name() && binding.ViewName != "") { continue } ...

panel keybindings are now on top and global keybindings are below separated with empty newline

this is fmt number x+1

dawidd6 · 2018-09-07T12:42:48Z

Done @jesseduffield

Oh man, merge conflicts again.

glvr182 · 2018-09-08T12:05:19Z

@dawidd6 How far are you on the PR, soon i'm going to rework the GUI package. I'll start on this when you're done with this PR. No rush just curious.

dawidd6 · 2018-09-08T12:24:36Z

@glvr182 well, i would say it is done already. Waiting for further review.

glvr182 · 2018-09-08T12:27:47Z

@dawidd6 A perfect, would also love to see this all in action, its really usefull! I guess we have to wait for @jesseduffield

dawidd6 · 2018-09-08T12:41:34Z

@glvr182 what kind of rework are you planning?

glvr182 · 2018-09-08T13:20:14Z

@dawidd6 Well i talked to @jesseduffield shortly about it as well and bascly, rewriting functions that can be made smarter, remove double code, better error handling / logging, dont pass useless parameters to functions, improve readability, and comments + formatting

glvr182 · 2018-09-08T13:20:27Z

so yea it's gonna be a big update

dawidd6 · 2018-09-08T15:30:07Z

I agree, after spending some time in the codebase i can tell it is in need for some rework.

glvr182 · 2018-09-08T16:09:11Z

@dawidd6 Ill start with it as soon as i can, if you have some ideas about possible changes you can just send me a message or if you want implement them in the branch ill be working in (to be created)

jesseduffield · 2018-09-09T03:03:55Z

@dawidd6 are you okay to look at these merge conflicts?

conflicts resolved

dawidd6 · 2018-09-09T08:44:45Z

@jesseduffield done, i think it's alright.

jesseduffield · 2018-09-09T23:43:22Z

LGTM, fantastic work man :)

dawidd6 mentioned this pull request Aug 28, 2018

Show keybinding cheatsheet in app #222

Closed

golangci reviewed Aug 28, 2018

View reviewed changes

dawidd6 commented Aug 30, 2018

View reviewed changes

dawidd6 added 10 commits September 3, 2018 17:52

move bindings to getKeybindings()

741e28d

initial help panel

2416f58

change key from 'H' to '?'

6a99d36

apply fmt

77623db

update help panel

28a9594

- delete scrolling ability - lines are now selectable - implemented handler execution when space is pressed - add example descriptions for status panel keybindings

more error checks

8a01d11

help panel size from getConfirmationPanelDimensions

653d590

make '?' key visible on every panel

cc3fa4b

move descriptions to i18n

9ceaf5b

indent keybindings

7ec5b6c

update pl translation

4188786

dawidd6 added 2 commits September 5, 2018 15:45

workaround to include menu keybinding in cheatsheet

08395ae

change menu keybinding from ? to x

db2e216

jesseduffield reviewed Sep 5, 2018

View reviewed changes

dawidd6 added 3 commits September 7, 2018 14:19

rewrite some of menu panel logic

ba6dedf

panel keybindings are now on top and global keybindings are below separated with empty newline

ladies and gentlemen...

9cef98f

this is fmt number x+1

satisfy golangci

e80371f

Merge branch 'master' into feature/help

6f7de83

conflicts resolved

shorter english string

63e4006

dawidd6 mentioned this pull request Sep 9, 2018

Can't scroll the bottom "hints" #26

Closed

jesseduffield merged commit f8ab4f4 into jesseduffield:master Sep 9, 2018

dawidd6 deleted the feature/help branch September 10, 2018 10:38

WIP: Add menu panel (cheatsheet) #234

WIP: Add menu panel (cheatsheet) #234

Conversation

dawidd6 commented Aug 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesseduffield commented Aug 28, 2018

dawidd6 commented Aug 29, 2018

jesseduffield commented Aug 29, 2018

dawidd6 commented Aug 29, 2018

jesseduffield commented Aug 29, 2018

dawidd6 commented Aug 29, 2018

jesseduffield commented Aug 29, 2018

dawidd6 commented Aug 29, 2018

jesseduffield commented Aug 29, 2018

dawidd6 commented Aug 29, 2018

dawidd6 commented Aug 29, 2018

codecov-io commented Aug 29, 2018 • edited

Codecov Report

jesseduffield commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidd6 commented Sep 1, 2018

dawidd6 commented Sep 1, 2018

jesseduffield commented Sep 3, 2018

dawidd6 commented Sep 3, 2018

dawidd6 commented Sep 5, 2018 • edited

Lazygit menu

Global

Status

Files

Branches

Commits

Stash

jesseduffield commented Sep 5, 2018

jesseduffield commented Sep 5, 2018

dawidd6 commented Sep 5, 2018 • edited

jesseduffield commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawidd6 commented Sep 7, 2018

glvr182 commented Sep 8, 2018

dawidd6 commented Sep 8, 2018

glvr182 commented Sep 8, 2018

dawidd6 commented Sep 8, 2018

glvr182 commented Sep 8, 2018

glvr182 commented Sep 8, 2018

dawidd6 commented Sep 8, 2018

glvr182 commented Sep 8, 2018

jesseduffield commented Sep 9, 2018

dawidd6 commented Sep 9, 2018

jesseduffield commented Sep 9, 2018

dawidd6 commented Aug 28, 2018 •

edited

codecov-io commented Aug 29, 2018 •

edited

dawidd6 commented Sep 5, 2018 •

edited

dawidd6 commented Sep 5, 2018 •

edited