Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Add new feature: Vim mode #368

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Add new feature: Vim mode #368

wants to merge 34 commits into from

Conversation

steewbsd
Copy link

This feature adds a working vim mode, some bugs are already known and will try to fix them. For example, getting inside shortcuts menu inside vim mode will render focus unusable and require an app reset (quit with q and open again).

@steewbsd steewbsd closed this Nov 16, 2020
@steewbsd steewbsd reopened this Nov 16, 2020
Copy link
Author

@steewbsd steewbsd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test whether the ESC key works on a binding. Change the VimNormalMode from "-" to tcell.KeyEscape.

@Bios-Marcel
Copy link
Owner

The method addVimKey doesn't exist, you probably forgot to commit the file. You also added multiple unused dependencies to the go.mod.

@steewbsd
Copy link
Author

Cannonical imports are preventing the package from building. I need some help managing them.

go.mod Outdated
@@ -1,3 +1,5 @@
replace github.com/Bios-Marcel/cordless => github.com/0xSteeW/cordless v0.0.0-20201117104521-a2cc7de4b6dd
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be upstream, it shouldn't be necessary at all.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will not build either way. It was a quick test to try to get appveyor to recognize my files

@steewbsd steewbsd mentioned this pull request Nov 17, 2020
@Bios-Marcel
Copy link
Owner

I hit alt+dot and this happened:

marcel@marcel-pc /m/d/c/cordless (0xSteeW-vim)> go run . --account=forkwatch
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x850ed9]

goroutine 1 [running]:
github.com/Bios-Marcel/cordless/tview.(*Application).Run.func1(0xc0000c17c0)
	/mnt/data/code/cordless/tview/application.go:168 +0x82
panic(0xd088c0, 0x165e470)
	/snap/go/6728/src/runtime/panic.go:969 +0x166
github.com/Bios-Marcel/cordless/shortcuts.(*Shortcut).Equals(0xc0000b7720, 0xc000c9b800, 0xc000c99e00)
	/mnt/data/code/cordless/shortcuts/shortcuts.go:440 +0x9
github.com/Bios-Marcel/cordless/ui.(*Window).handleGlobalShortcuts(0xc000130c60, 0xc000c9b800, 0xc0000c17c0)
	/mnt/data/code/cordless/ui/window.go:2145 +0xe4
github.com/Bios-Marcel/cordless/tview.(*Application).Run(0xc0000c17c0, 0x0, 0x0)
	/mnt/data/code/cordless/tview/application.go:256 +0xa4e
main.main()
	/mnt/data/code/cordless/main.go:74 +0x4cd
exit status 2

@Bios-Marcel
Copy link
Owner

I can't hit a single key with the app crashing actually ^^

@steewbsd
Copy link
Author

Noticed it yesterday, only happening after merging your last commit. Can you try building my last commit (before merging)?

)

type VimHelp struct {
vimMode *vim.Vim
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is doubly tracked, the config already knows about this, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config only knows about VimEnabled, which is a bool only used when instantiating app.VimMode. This is needed to directly enable or disable vim interactively in the command line for this session.

}

// NewApplication creates and returns a new application.
func NewApplication() *Application {
func NewApplication(vimEnabled bool) *Application {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does tview need to know about vim mode?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Application holds the pointer to vim.Vim that will be shared to the entire program, with app.vimMode.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is a cordless thing, not a tview thing, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a cordless thing, but the best place as it seemed to me to share the global pointer to vim.Vim was in window.app and derived. So NewApplication needs to know if vim is enabled (as it does not have access to the config)

@@ -67,17 +68,17 @@ type Application struct {
// stop the application.
screenReplacement chan tcell.Screen

// Defines whether a bracketed paste is currently ongoing.
pasteActive bool
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needs a merge with master, this shouldn't be an outgoing deletion!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to re-merge after reverting commits.

@@ -5,5 +5,5 @@ package shortcuts
import tcell "github.com/gdamore/tcell/v2"

func addDeleteLeftShortcut() *Shortcut {
return addShortcut("delete_left", "Delete left", multilineTextInput, tcell.NewEventKey(tcell.KeyBackspace, rune(tcell.KeyBackspace), tcell.ModNone))
return addShortcut("delete_left", "Delete left", multilineTextInput, tcell.NewEventKey(tcell.KeyBackspace, rune(tcell.KeyBackspace), tcell.ModNone),nil)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the last parameter nil? Does this need explanation or do we need to change it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows build for some reason wont compile, it does not find anything declared inside shortcuts for some reason, so I can not use shortcuts.addVim or NullVimEvent. It's pretty much a HACK to make it pass the build.

go.mod Outdated
@@ -9,6 +9,7 @@ require (
github.com/Bios-Marcel/shortnotforlong v1.1.1
github.com/alecthomas/chroma v0.7.3
github.com/atotto/clipboard v0.1.2
github.com/bunyk/gokeybr v0.0.0-20201019133936-f9e4ed3fdc5d // indirect
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Installed some things while working on this, thought I had reverted go.sum and go.mod. Will fix

ui/window.go Outdated
window.vimStatus.Content = fmt.Sprintf("Vim: %s",window.app.VimMode.CurrentModeString())
return nil
} else if shortcuts.VimVisualMode.Equals(event) {
window.app.VimMode.Visual()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious what the methods do, as there's no verb in the names.

@@ -2117,6 +2141,35 @@ func (window *Window) handleGlobalShortcuts(event *tcell.EventKey) *tcell.EventK
return nil
}

if window.app.VimMode.CurrentMode != vim.Disabled {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the login view, I assume that's not handled with this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Login is not handled.

util/vim/vim.go Outdated
// InsertMode : every key is sent to the program as normal,
// such as input boxes.
InsertMode // 1
// TODO
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs fix

util/vim/vim.go Outdated
NormalMode int = iota // 0
// InsertMode : every key is sent to the program as normal,
// such as input boxes.
InsertMode // 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The //0 and //1 comments are kinda unnecessary.

util/vim/vim.go Outdated
VisualMode // 2

// Default: disabled
Disabled = -1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need disabled? Isn't this what VimEnabled: false in the config is for?
And if I missunderstand, why isn't this 0? You might also want to create a type alias type VimMode int.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config's VimEnabled is only used when instantiating vim.Vim. This struct then holds a value, CurrentMode, that will be checked everywhere vim mode needs to replace normal bindings.

@bobbbay
Copy link

bobbbay commented Jan 2, 2021

+1 this would make and break for myself, and many others. I can finally moderate my vim server with vim keybindings!

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

Successfully merging this pull request may close these issues.

3 participants