Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scroll keybindings for mac #423

Closed
sudo-suhas opened this issue Apr 9, 2019 · 7 comments · Fixed by #443
Closed

scroll keybindings for mac #423

sudo-suhas opened this issue Apr 9, 2019 · 7 comments · Fixed by #443

Comments

@sudo-suhas
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Right now, the scroll keybindings shown in the default screen on running lazygit as well as the /docs/keybindings/Keybindings_en.md file show this:

  PgDn: scroll down
  PgUp: scroll up

However these keys do not exist on mac(right?). I came across the correct keybindings in #25.

Describe the solution you'd like
Ideally, the keybindings shown at the bottom on running lazygit would be OS aware. Furthermore, the Keybindings.md file should call out the different keybinding on mac. I'd be happy to work on these if you could give me some pointers on where the changes would need to be done.

@mjarkk
Copy link
Contributor

mjarkk commented Apr 9, 2019

This is not a mac specific issue, A lot of laptops also don't have PgDn and PgUp. Beside that macOS supports the keys just fine and there are keyboard for macOS that have those keys.
Personally i like the Ctrl + u and Ctrl + d over the PgDn and PgUp

Isn't it better to completely replace all the documentation and tips with the Ctrl + u/d

@xdjinnx
Copy link
Contributor

xdjinnx commented Apr 9, 2019

On mac you can use Fn + Up and Fn + Down! 😃

@jesseduffield
Copy link
Owner

This isn't the first time somebody has had this conundrum, suggesting the docs could be clearer.

@sudo-suhas if you want to help make this clearer to the user, the place to go would be lazygit/pkg/gui/keybindings.go and add an Alternative key to the Binding struct, and maybe for the case of pgup do:

		}, {
			ViewName:    "",
			Key:         gocui.KeyPgup,
			Modifier:    gocui.ModNone,
			Handler:     gui.scrollUpMain,
			Alternative: "fn+up",
		}, {

Then in lazygit/scripts/generate_cheatsheet.go's formatBinding function you could add the alternative value to the end of the description in parentheses like 'Scroll up (fn+up)`.

I'm open to suggestions on how better to do this given that this approach wouldn't be particularly DRY if we end up with multiple places where we see the gocui.KeyPgup key being used (we'd need to set the alternative for each of those instances which isn't very clean).

@sudo-suhas
Copy link
Contributor Author

Is it possible to represent fn+up as a Key? If we could, build tags or runtime.GOOS can be used to tailor the keybindings to the OS.

@sudo-suhas
Copy link
Contributor Author

For example, in gui.renderGlobalOptions:

func (gui *Gui) renderGlobalOptions() error {
	scrollKeys := "PgUp/PgDn"
	if runtime.GOOS == "darwin" {
		scrollKeys = "FnUp/FnDn"
	}
	return gui.renderOptionsMap(map[string]string{
		scrollKeys: gui.Tr.SLocalize("scroll"),
		"← → ↑ ↓":  gui.Tr.SLocalize("navigate"),
		"esc/q":    gui.Tr.SLocalize("close"),
		"x":        gui.Tr.SLocalize("menu"),
	})
}

@jesseduffield
Copy link
Owner

The problem is that PgUp and PgDn are actually valid keys on a mac keyboard that includes a number pad (as mine does) but they don't appear on mac keyboards e.g. on a macbook. So it's actually not a matter of the OS being different, it's a combination of OS and hardware

@sudo-suhas
Copy link
Contributor Author

The problem is that PgUp and PgDn are actually valid keys on a mac keyboard that includes a number pad

@jesseduffield I did not know that. I will be doing the changes as per your suggestions in #423 (comment). One question I have is whether we should show the alternatives to keybindings shown in the default screen. And if this should be done only for mac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants